Skip to content

8356438: Update OutputAnalyzer to optionally print process output as it happens #25587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+8 −4
Open

8356438: Update OutputAnalyzer to optionally print process output as it happens #25587

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b51f0d4
Initial working solution
friedbyalice Jun 2, 2025
db559d2
Update test/lib/jdk/test/lib/process/OutputBuffer.java
friedbyalice Jun 4, 2025
7cfbec2
Merge remote-tracking branch 'origin/master' into 8356438-outputanaly…
friedbyalice Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions test/lib/jdk/test/lib/process/OutputBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.io.ByteArrayOutputStream;
import java.io.InputStream;
import java.io.StringReader;
import java.io.OutputStream;
import java.nio.charset.Charset;
import java.time.Instant;
import java.util.List;
Expand Down Expand Up @@ -111,10 +112,12 @@ private static class StreamTask {
private final Future<Void> future;
private final Charset cs;

private StreamTask(InputStream stream, Charset cs) {
private StreamTask(InputStream stream, Charset cs, OutputStream additionalStream) {
this.buffer = new ByteArrayOutputStream();
this.cs = cs;
this.future = new StreamPumper(stream, buffer).process();
StreamPumper pumper = new StreamPumper(stream, buffer);
if (additionalStream != null) pumper.addOutputStream(additionalStream);
this.future = pumper.process();
}

public String get() {
Expand Down Expand Up @@ -144,8 +147,9 @@ private final void logProgress(String state) {
private LazyOutputBuffer(Process p, Charset cs) {
this.p = p;
logProgress("Gathering output");
outTask = new StreamTask(p.getInputStream(), cs);
errTask = new StreamTask(p.getErrorStream(), cs);
boolean verbose = Boolean.getBoolean("outputanalyzer.verbose");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting a system property at this level of the code kind of hides the functionality.

An alternative solution would be to have OutputAnalyzer constructor(s) that takes a boolean argument. That boolean could be set as needed by individual tests using the test.debug property already used by a lot of tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't it the person running the tests that wants to set this, not an inherent property of a test itself? Or are you envisaging enabling it at the test-level so the person running the tests doesn't have to do so? But then how does that affect the overall jtreg log content. ??

Copy link
Author

@friedbyalice friedbyalice Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add on this, I should mention that I noticed a lot of tests are using OutputAnalyzer indirectly, returned as a result of a utility function, for example ProcessTools.execute{Process,Command}

git grep -E "ProcessTools\.execute((Process)|(Command))" | wc -l
     351

Instead of calling one of OutputAnalyzer's constructors (700 invocations, but many of them are with the Eager, string based and not process based, constructor, which we don't care about)

I'm not sure adding additional parameters also to that code is an ideal solution, I'd much prefer adding the command line parameter to the docs of the OutputAnalyzer class, so that one knows to enable it when running the single test through jtreg

That said, if there are multiple OutputAnalyzers going in a single test, then it makes perfect sense to have (or at least use) a constructor argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't it the person running the tests that wants to set this, not an inherent property of a test itself?

I'm not sure if I completely understand your question. A lot of tests use the test.debug system property to enable more verbose test output. It's enabled when someone runs the test e.g., jtreg -Dtest.debug=true ...

As you pointed out, a lot of tests may not care if the subprocess's output is cached or not, but for those that do, having to specify a second property could be onerous and using the same test.debug property inside OutputBuffer could result in unexpected output. If the OutputAnalyzer ctor took a boolean , a test could enable (or not) with something like new OutputAnalyzer(childProc, Boolean.getBoolean("test.debug"))

I'm not sure adding additional parameters also to that code is an ideal solution,

There are two constructors that take Process objects as arguments. I was thinking that you could overload them to take the extra argument.

public OutputAnalyzer(Process process, Charset cs, boolean flushOutput)
public OutputAnalyzer(Process process, boolean flushOutput)

None of the existing tests would be affected and those tests that could benefit from inline output could specify it as needed. You're right that the use of OutputAnalyzer is usually indirect so that would cause some other code to be changed as well, but it only has to change when it becomes necessary to enable it. (And it can be updated in the same way i.e., overloading the methods to take an extra argument.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"verbose" is not really the right word here either. This does not change the amount of output only where it also gets sent to, and that is hard-wired to stdout/err so the name should reflect that e.g. printToStdStreams ?

I'm not at all sure how to expose this to the top-level API's though.

outTask = new StreamTask(p.getInputStream(), cs, verbose ? System.out : null);
errTask = new StreamTask(p.getErrorStream(), cs, verbose ? System.err : null);
}

@Override
Expand Down