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