8336332: Rework tests to avoid unrelated stderr output #1897

andy-goryachev-oracle · 2025-09-11T22:09:10Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8336332: Rework tests to avoid unrelated stderr output (Enhancement - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1897/head:pull/1897
$ git checkout pull/1897

Update a local copy of the PR:
$ git checkout pull/1897
$ git pull https://git.openjdk.org/jfx.git pull/1897/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1897

View PR using the GUI difftool:
$ git pr show -t 1897

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1897.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-11T22:10:17Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-11T22:10:58Z

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8336332: Rework tests to avoid unrelated stderr output

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

8f4d7a0: 8368375: Skip intermittently failing SWTCursorsTest
405450d: 8367991: Update RegionBackground tests to use ScreenCaptureTestWatcher utility
4ac4ff6: 8366986: [Win] Incorrect position and size after hiding iconified stage
... and 2 more: https://git.openjdk.org/jfx/compare/7a488327beb574c84bd68e620d97c2bbe24e335b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-09-12T16:06:46Z

Webrevs

kevinrushforth · 2025-09-12T17:51:54Z

I see that you focused on javafx.base, which is where many of the problems are. Do you plan to update this PR for the other modules and for system tests (the latter also has several exceptions and lots of noise), or will you file a follow-up issue to take up after this one is done?

andy-goryachev-oracle · 2025-09-12T18:00:01Z

good point.
no, this PR is only for the headful tests as they cause the most grief when analyzing the failures.

mstr2 · 2025-09-12T18:16:08Z

The ErrorLoggingUtility class manages static state, and requires users to follow a protocol to avoid messing up its internal state. Have you considered using a try-with-resources scope to make the API easier to use, for example:

try (var util = ErrorLoggingUtility.suppressStdErr()) {
    // test something
    // ...
    //

    util.checkWarning(NullPointerException.class);
} // internal state is restored at the end of the block

andy-goryachev-oracle · 2025-09-12T18:24:08Z

Good point. It already has a static state (error log record), and on top of that try with resources will not work in the bulk use case of @BeforeEach and @AfterEach

Plus, it's a test utility.

I did try to explain the usage in javadoc - maybe that can be further clarified?

mstr2 · 2025-09-12T18:47:40Z

Good point. It already has a static state (error log record), and on top of that try with resources will not work in the bulk use case of @BeforeEach and @AfterEach

Why wouldn't it work? If you really need to, you can store the AutoClosable in @BeforeEach and then call the close method in @AfterEach. But I would also prefer to not scatter around code in before-each and after-each methods, but to have each test be as stand-alone as possible, especially when it comes to scoping.

Whether ErrorLoggingUtility has static state is also immaterial (that would only be important if tests were to be run in parallel), what matters is the API. Using an auto-closable scope has the following advantages:

Using the tool is easy, as you can only call methods that are applicable for the state of the test. If you want to check the log output, you need to have an active scope instance.
It can't be misused, as you can't call methods in the wrong order.
The static state will always be well-defined.

Yes, it's only a test utility, but the API is really suboptimal no matter how I look at it.

andy-goryachev-oracle · 2025-09-12T20:02:49Z

These are all valid points, but keep in mind that System.setErr() is static to begin with. All the system and headful tests are single threaded by design, and this is rather unlikely to change.

Whether to use BeforeEach/AfterEach or standalone test is up to the test developers (this PR contains both cases actually). I would say re-designing the existing tests to be standalone (SelectBindingTest for example) is out of scope for this PR.

kevinrushforth · 2025-09-16T18:09:30Z

Two general comments before I start looking at the PR:

Since you are only addressing the javafx.base tests in this PR, please update the JBS title (and PR title) to reflect that.
In the description you wrote:

This PR removes unrelated stderr output in the headful test logs

These are "headless" tests, not headful.

andy-goryachev-oracle · 2025-09-16T19:17:24Z

Since you are only addressing the javafx.base

so that's where it gets complicated: turns out the utility gets used in more tests (graphics, fxml, see #1905 ), so two things came up:

the utility needs to be moved to another package
the utility probably needs to be decoupled from Logging, as it creates unneeded dependency with --add-exports javafx.base/com.sun.javafx.binding=ALL-UNNAMED in 8367567: Rework system tests to suppress unrelated stderr output #1905
perhaps the two PRs need to be merged into one larger one

What do you think?

kevinrushforth · 2025-09-16T20:14:15Z

Since you are only addressing the javafx.base

so that's where it gets complicated: turns out the utility gets used in more tests (graphics, fxml, see #1905 ), so two things came up:

the utility needs to be moved to another package

the utility probably needs to be decoupled from Logging, as it creates unneeded dependency with --add-exports javafx.base/com.sun.javafx.binding=ALL-UNNAMED in 8367567: Rework system tests to suppress unrelated stderr output #1905

Yes, I think this seems best. Rather than extend the existing javafx.base ErrorLoggingUtility, which is dependent on the (IMO rather hacky) Logging class in javafx.base itself, a separate test utility that just focuses on capturing and parsing stderr and has no dependencies on logging seems best.

perhaps the two PRs need to be merged into one larger one

Maybe that would be best, although I think it would be OK to keep them separate as long as you do a proof of concept that other modules can use it without needing changes to the exports, etc.

kevinrushforth

I left a couple quick comments in line. I won't do a thorough review until you decide whether to create a new utility class (which would have the advantage of removing a number of unrelated changes in this PR).

kevinrushforth · 2025-09-16T18:50:13Z

modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java

+
+    private static PrintStream stderr;
+    private static AccumulatingPrintStream stderrCapture;
+    private static int checked;


This is unused (only ever written) and can be removed.

kevinrushforth · 2025-09-16T19:29:52Z

modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java

+            if(!errors.equals(exp)) {
+                stderr.println("Mismatch in thrown exceptions:\n  expected=" + exp + "\n  observed=" + errors);
+                stderr.println(text);
+            }


A mismatch should fail the test.

kevinrushforth · 2025-09-16T20:14:55Z

modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java

+        "(?:" +
+            // catches lines starting with things like "Exception in thread "main" java.lang.RuntimeException:"
+            "^" +
+            "(?:" +


This might be easier to read and maintain with text blocks.

it's a complete gibberish as continuous text. not an easy thing to parse mentally.

andy-goryachev-oracle · 2025-09-16T20:30:41Z

thanks for suggestions! I'll revert back to DRAFT since there will be more structural changes, then we'll see if we can separate the two PRs or combine them.

kevinrushforth

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" :)

kevinrushforth · 2025-09-18T15:21:24Z

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java

    @BeforeEach
    public void setUp() throws Exception {
+        OutputRedirect.suppressStderr();
+        ErrorLoggingUtiltity.reset();


Moving the call to reset to a @BeforeEach method is an unrelated change. It seems safer to restore the @BeforeAll method with just the call to reset.

kevinrushforth · 2025-09-18T15:24:32Z

modules/javafx.base/src/test/java/test/javafx/binding/BindingsCreateBindingTest.java

+    @BeforeEach
+    public void beforeEach() {
+        OutputRedirect.suppressStderr();
        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.

kevinrushforth · 2025-09-18T15:25:53Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+    }
+
+    /// Returns the captured output, if any, or `null`.
+    /// @return the captured output string, or `null`


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

this method is not used anymore, will remove.

kevinrushforth · 2025-09-18T15:30:14Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

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


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

kevinrushforth · 2025-09-18T15:33:50Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+                        m.put(name, Integer.valueOf(v + 1));
+                    }
+                } else {
+                    throw new IllegalArgumentException("must specify either Class<? extends Throwable>: " + c);


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

kevinrushforth · 2025-09-18T15:40:41Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+        return null;
+    }
+
+    // should I leave this test here?  to test the test?


Seems reasonable.

Although, I would remove or reword the comment asking whether you should leave it if you've decided to do so.

kevinrushforth · 2025-09-18T15:47:23Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+    private static Map<String, Integer> toMap(Object... expected) {
+        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.

No, I need the counts.
JDK has neither MultiMaps nor counting sets.

Oh, nm. I missed that the value is a count of that particular exception class.

kevinrushforth · 2025-09-18T15:53:52Z

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

-        doTestCommon(implicitExit, reEnableImplicitExit, stageShown,
-                ThrowableType.NONE, appShouldExit);
+        doTestCommon(implicitExit, reEnableImplicitExit, stageShown, ThrowableType.NONE, appShouldExit);


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

kevinrushforth · 2025-09-18T15:58:34Z

tests/system/src/test/java/test/launchertest/ModuleLauncherTest.java

    private final int testExitCode = ERROR_NONE;

-    private void doTestLaunchModule(String appModulePath, String testAppName) throws Exception {
+    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.

andy-goryachev-oracle · 2025-09-18T16:09:53Z

regex: I can try commenting what each part means

kevinrushforth

Last batch of comments. Initial testing looks good.

kevinrushforth · 2025-09-18T17:28:05Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

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


Oh, nm. I missed that the value is a count of that particular exception class.

kevinrushforth · 2025-09-18T17:29:18Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+                } else {
+                    throw new IllegalArgumentException("must specify Class<? extends Throwable>: " + c);
+                }
+            } else if(x instanceof String) {


Minor: space after if

kevinrushforth · 2025-09-18T17:44:30Z

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java

    @BeforeAll
-    public static void setUpClass() {
-        System.err.println("SelectBindingTest : log messages are expected from these tests.");
+    public static void setUpClass() throws Exception {


Minor: this method previously didn't have throws Exception and probably doesn't need it.

Are you going to revert the "throws Exception" ?

kevinrushforth · 2025-09-18T17:45:24Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+        return null;
+    }
+
+    // should I leave this test here?  to test the test?


Although, I would remove or reword the comment asking whether you should leave it if you've decided to do so.

kevinrushforth · 2025-09-18T17:48:29Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+            forEach((c) -> {
+                if (c != null) {


Minor suggestion: filter for non-null Strings before the forEach rather than checking for non-null in the forEach ?

my version is more efficient :-)

I doubt it. Anyway, it doesn't really matter all that much.

off topic, but it is more efficient - check out

https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/research/MoreEfficient.java

It might be interesting to see micro-benchmark results, but yeah, off topic.

kevinrushforth · 2025-09-18T17:50:42Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+    ///
+    /// `java.lang.NullPointerException: ...`
+    private static final Pattern EXCEPTION_PATTERN = Pattern.compile(
+        "(?:" +


Looks reasonable to me. :)

kevinrushforth · 2025-09-18T18:38:03Z

tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java

+            } finally {
+                OutputRedirect.checkAndRestoreStderr(ClassCastException.class);
+            }


This fails for me, which is not surprising since the snapshot is asynchronous. You probably want to move everything (the suppressStderr and the try/finally with the checkAndRestoreStderr) out of the runAndWait, but even that might not be sufficient without a delay after latch.getCount().

you are right, I missed the runAndWait()

kevinrushforth

Shapshot1Test still fails for me. This time the exceptions are being captured in the print stream, but the parsing of the output reports 2 exceptions instead of the expected 1:

    Mismatch in thrown exceptions:
      expected={java.lang.ClassCastException=1}
      observed={java.lang.ClassCastException=1, Exception=1}

kevinrushforth · 2025-09-18T20:23:32Z

tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java

+            assertEquals(0, latch.getCount());
+        } finally {


Since the ClassCast exception happens after the latch countdown, I recommend a sleep (of, say, 100 msec) after checking that the latch count is 0.

kevinrushforth · 2025-09-18T20:24:06Z

tests/system/src/test/java/test/javafx/scene/Snapshot1Test.java

+            assertEquals(0, latch.getCount());
+        } finally {


Same here: a short sleep after the assertEquals.

andy-goryachev-oracle · 2025-09-18T20:32:24Z

"Exception is snapshot callback" is coming from
Node:2421 and Scene:1612

Why in the world do we have unsuppressible System.err.println() in there??

andy-goryachev-oracle · 2025-09-18T20:43:24Z

I decided to remove Snapshot1Test from this PR: there is enough value added already, and there is more work needed to remove stderr output coming from CssParser anyway, so let's postpone it until the next test sprint.

follow up:
JDK-8368023
[TestBug] Rework tests to clean up stderr

kevinrushforth

Looks good. My testing shows that there are no more "failed" messages for a successful build and that the number of Exception messages are reduced. The remainder can be handled by the follow-on bug you filed.

kevinrushforth · 2025-09-18T20:53:12Z

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java

    @BeforeAll
-    public static void setUpClass() {
-        System.err.println("SelectBindingTest : log messages are expected from these tests.");
+    public static void setUpClass() throws Exception {


Are you going to revert the "throws Exception" ?

kevinrushforth · 2025-09-19T14:14:54Z

modules/javafx.base/src/test/java/test/javafx/util/OutputRedirect.java

+            forEach((c) -> {
+                if (c != null) {


It might be interesting to see micro-benchmark results, but yeah, off topic.

kevinrushforth · 2025-09-19T15:29:22Z

"Exception is snapshot callback" is coming from Node:2421 and Scene:1612

Why in the world do we have unsuppressible System.err.println() in there??

We have many such places in the JavaFX runtime. We already have a task filed to examine these: JDK-8320266.

kevinrushforth · 2025-09-22T13:26:17Z

@arapte Can you be the second reviewer?

arapte

LGTM+, The log looks good now.

andy-goryachev-oracle · 2025-09-23T18:09:06Z

thank you for reviewing!

/integrate

openjdk · 2025-09-23T18:10:35Z

Going to push as commit f4b3d55.
Since your change was applied there have been 5 commits pushed to the master branch:

8f4d7a0: 8368375: Skip intermittently failing SWTCursorsTest
405450d: 8367991: Update RegionBackground tests to use ScreenCaptureTestWatcher utility
4ac4ff6: 8366986: [Win] Incorrect position and size after hiding iconified stage
... and 2 more: https://git.openjdk.org/jfx/compare/7a488327beb574c84bd68e620d97c2bbe24e335b...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-23T18:10:47Z

@andy-goryachev-oracle Pushed as commit f4b3d55.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

andy-goryachev-oracle added 4 commits September 11, 2025 13:45

suppress stderr

1b4efaf

fixed utility class name

923f1b4

2025

16638a4

revert intentionally failing test

a669287

whitespace

dd43ce9

andy-goryachev-oracle marked this pull request as ready for review September 12, 2025 16:01

openjdk bot added the rfr Ready for review label Sep 12, 2025

andy-goryachev-oracle added 7 commits September 15, 2025 11:12

Merge remote-tracking branch 'origin/master' into 8336332.failed

7b228d4

cleanup

cd1b946

moved utility

12e0f6a

fxml

1a60d2e

add exports

2b435a8

whitespace

fb1fd24

test.javafx.util

bbcc412

kevinrushforth reviewed Sep 16, 2025

View reviewed changes

andy-goryachev-oracle marked this pull request as draft September 16, 2025 20:29

openjdk bot removed the rfr Ready for review label Sep 16, 2025

kevinrushforth reviewed Sep 18, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits September 18, 2025 09:29

review comments

3afa270

static

4399362

kevinrushforth reviewed Sep 18, 2025

View reviewed changes

review comments

3ed8135

kevinrushforth reviewed Sep 18, 2025

View reviewed changes

run and wait

734c261

kevinrushforth reviewed Sep 18, 2025

View reviewed changes

revert snapshot 1 test

25695f5

kevinrushforth approved these changes Sep 19, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits September 19, 2025 08:32

Merge remote-tracking branch 'origin/master' into 8336332.failed

a9abd5a

throws

3d90049

kevinrushforth approved these changes Sep 19, 2025

View reviewed changes

andy-goryachev-oracle requested a review from kevinrushforth September 19, 2025 15:36

arapte reviewed Sep 23, 2025

View reviewed changes

arapte approved these changes Sep 23, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Sep 23, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 23, 2025

openjdk bot closed this Sep 23, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Sep 23, 2025

andy-goryachev-oracle deleted the 8336332.failed branch September 23, 2025 18:12

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

8336332: Rework tests to avoid unrelated stderr output #1897

8336332: Rework tests to avoid unrelated stderr output #1897

Uh oh!

Conversation

andy-goryachev-oracle commented Sep 11, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How To

Changes

Miscellaneous

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 11, 2025

Uh oh!

openjdk bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Sep 12, 2025

Uh oh!

andy-goryachev-oracle commented Sep 12, 2025

Uh oh!

mstr2 commented Sep 12, 2025

Uh oh!

andy-goryachev-oracle commented Sep 12, 2025

Uh oh!

mstr2 commented Sep 12, 2025

Uh oh!

andy-goryachev-oracle commented Sep 12, 2025

Uh oh!

kevinrushforth commented Sep 16, 2025

Uh oh!

andy-goryachev-oracle commented Sep 16, 2025

Uh oh!

kevinrushforth commented Sep 16, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Sep 16, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

andy-goryachev-oracle commented Sep 11, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 11, 2025 •

edited

Loading

mlbridge bot commented Sep 12, 2025 •

edited

Loading

andy-goryachev-oracle Sep 18, 2025 •

edited

Loading