On Mon, 9 Jun 2025 04:03:28 GMT, David Holmes <[email protected]> wrote:
>> 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. ??
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
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2137161838
