On Tue, 17 Jun 2025 17:33:31 GMT, Matthew Donovan <mdono...@openjdk.org> wrote:
>> 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
>
>> 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.)
As noted above, it is not usual for test to create an instance of
`OutputAnalyzer` directly, so the extra constructor argument would seem of
little general value. But okay, it might be useful in some case.
In terms of the existing PR to control the behaviour via System property, I'll
repeat my comment that "verbose" is not really the right word here. 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
?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2220910786
