On Thu, 5 Jun 2025 09:36:37 GMT, Alice Pellegrini <d...@openjdk.org> wrote:

>> The implemented solution modifies the `OutputBuffer` implementation instead 
>> of the `OutputAnalyzer` implementation.
>> This is because the **OutputBuffer implementation which handles processes** 
>> (LazyOutputBuffer) starts a thread in its constructor, so we would need to 
>> add a strange additional constructor parameter to the 
>> `OutputBuffer.of(Process, Charset)` static method, while the printing 
>> through to stdout (and stderr) only makes sense for LazyOutputBuffer.
>> 
>> I believe changing the config option from `outputanalyzer.verbose` to 
>> `output buffer.verbose` would make it cleaner, and avoid referencing the 
>> OutputAnalyzer in the OutputBuffer implementation.
>
> Alice Pellegrini has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into 
> 8356438-outputanalyzer-optional-print
>  - Update test/lib/jdk/test/lib/process/OutputBuffer.java
>    
>    Co-authored-by: Chen Liang <li...@openjdk.org>
>  - Initial working solution

> 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 
> ...

I see a few tests that do that, nearly all of which are network tests. It is 
not something I was familiar with. But it doesn't seem right to me that a 
particular test would use that property to change the behaviour of 
`OutputAnalyzer`. I also don't think the test should have to opt-in to this 
kind of behaviour rather than it being directly controlled by the person 
running the tests. To that end I would see `OutputAnalyzer` examining a  
specific property, e.g. `OutputAnalyzer.printToStdStreams` to control this.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/25587#issuecomment-2982650756

Reply via email to