RFR: 8336332: Rework tests to avoid unrelated stderr output [v7]

[Kevin Rushforth]

On Thu, 18 Sep 2025 15:26:27 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> This PR removes unrelated `stderr` output in the headless test logs by redirecting it to an in-memory buffer.  Exceptions found in the buffer can be checked against the expected list.
>> 
>> In the case when any mismatch is detected, whether the type or the number of exceptions of particular type, the accumulated buffer gets dumped to `stderr` and the test fails.
>> 
>> ## How To
>> 
>> To redirect stderr and later check the exceptions, surround your code with
>> 
>> `OutputRedirect.suppressStderr()` and either `OutputRedirect.checkStderr()` or `OutputRedirect.checkAndRestoreStderr()` (ideally, in the `finally` block).
>> 
>> To simply undo redirection, without checking, call  `OutputRedirect.restoreStderr()`.
>> 
>> To add the check to all the tests in the file, one can call the above mentioned methods inside  `@BeforeEach` and `@AfterEach`.
>> 
>> ## Changes
>> 
>> - added `OutputRedirect` facility
>> 
>> ## Miscellaneous
>> 
>> `ErrorLoggingUtiltity` name will be fixed in a followup https://bugs.openjdk.org/browse/JDK-8367995
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup

Thanks for reverting the renaming. It's much easier to review now.

I left a few inline comments. I'll test it and then also finish my review. The regex is a bit of a head scratcher (as they always are), so I'll probably just do a quick "eh, looks OK as long as it works" :)

modules/javafx.base/src/test/java/test/javafx/binding/BindingsCreateBindingTest.java line 73:

> 71:     public void beforeEach() {
> 72:         OutputRedirect.suppressStderr();
> 73:         ErrorLoggingUtiltity.reset();

Same comment as in earlier class: it seems best to leave the reset in a `@BeforeAll` method, unless there is a compelling reason to change it.

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java line 73:

> 71: 
> 72:     /// Returns the captured output, if any, or `null`.
> 73:     /// @return the captured output string, or `null`

The empty string might be a better choice if there is no output.

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java line 103:

> 101:             Map<String, Integer> exp = toMap(expected);
> 102:             if (!errors.equals(exp)) {
> 103:                 stderr.println("Mismatch in thrown exceptions:\n  expected=" + exp + "\n  observed=" + errors);

This should cause the test to fail. Set `err = true` here.

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java line 141:

> 139: 
> 140:     private static Map<String, Integer> toMap(Object... expected) {
> 141:         HashMap<String, Integer> m = new HashMap<>();

Suggestion: Given how they are used, you might consider using a `Set` (which could be created as either a `LinkedHashSet` or `TreeSet`) instead. This would simplify the logic a bit.

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java line 153:

> 151:                     }
> 152:                 } else {
> 153:                     throw new IllegalArgumentException("must specify either Class<? extends Throwable>: " + c);

Typo: remove the "either" since there isn't another one in this case.

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java line 221:

> 219:     }
> 220: 
> 221:     // should I leave this test here?  to test the test?

Seems reasonable.

tests/system/src/test/java/test/com/sun/javafx/application/SingleExitCommon.java line 141:

> 139:             boolean appShouldExit) {
> 140: 
> 141:         doTestCommon(implicitExit, reEnableImplicitExit, stageShown, ThrowableType.NONE, appShouldExit);

Minor: This is an unrelated formatting change in a method that is otherwise untouched.

tests/system/src/test/java/test/launchertest/ModuleLauncherTest.java line 60:

> 58:     private final int testExitCode = ERROR_NONE;
> 59: 
> 60:     private void doTestLaunchModule(String appModulePath, String testAppName, Object ... expected) throws Exception {

No caller of this method passes anything for the newly added `Object ... expected` varargs parameter. I recommend reverting the addition of the parameter.

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

PR Review: https://git.openjdk.org/jfx/pull/1897#pullrequestreview-3240454716
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359883325
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359889900
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359910671
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359994625
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359928622
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2359960040
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2360026251
PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2360048846