On Thu, 5 Jun 2025 09:36:37 GMT, Alice Pellegrini <d...@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 <li...@openjdk.org>
> - Initial working solution
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");
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2129066907
