On Tue, 10 Jun 2025 07:47:29 GMT, Alice Pellegrini <d...@openjdk.org> wrote:
>> 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
>
> ---
>
> 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.)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25587#discussion_r2152810288
