RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]
David Holmes
dholmes at openjdk.org
Mon Jun 9 04:06:55 UTC 2025
On Thu, 5 Jun 2025 09:36:37 GMT, Alice Pellegrini <duke at openjdk.org> wrote:
>> The implemented solution modifies the `OutputBuffer` implementation instead of the `OutputAnalyzer` implementation.
>> This is because the **OutputBuffer implementation which handles processes** (LazyOutputBuffer) starts a thread in its constructor, so we would need to add a strange additional constructor parameter to the `OutputBuffer.of(Process, Charset)` static method, while the printing through to stdout (and stderr) only makes sense for LazyOutputBuffer.
>>
>> I believe changing the config option from `outputanalyzer.verbose` to `output buffer.verbose` would make it cleaner, and avoid referencing the OutputAnalyzer in the OutputBuffer implementation.
>
> Alice Pellegrini has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
> - Merge remote-tracking branch 'origin/master' into 8356438-outputanalyzer-optional-print
> - Update test/lib/jdk/test/lib/process/OutputBuffer.java
>
> Co-authored-by: Chen Liang <liach at openjdk.org>
> - Initial working solution
Given jtreg buffers everything anyway, does this even help the outlined usecase?
To be blunt if I were debugging the process launched via ProcessTools, I'd be doing that directly without jtreg and the test libraries getting in the way. That said the proposed change doesn't seem to cause any harm so I'm not totally against it. Just not sure how effective it actually is, nor the best way for the person running the test to turn it on.
test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
> 148: this.p = p;
> 149: logProgress("Gathering output");
> 150: boolean verbose = Boolean.getBoolean("outputanalyzer.verbose");
"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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25587#pullrequestreview-2908864741
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2134992541
More information about the core-libs-dev
mailing list