On Thu, 5 Jun 2025 14:58:40 GMT, Matthew Donovan <mdono...@openjdk.org> wrote:
>> 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.
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. ??
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2134994847
