On Thu, 18 Sep 2025 15:26:27 GMT, Andy Goryachev <ango...@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
