RFR: 8356438: Update OutputAnalyzer to optionally print process output as it happens [v2]

[Matthew Donovan]

On Tue, 10 Jun 2025 07:47:29 GMT, Alice Pellegrini <duke at 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