On Mon, 15 Sep 2025 19:25:37 GMT, Andy Goryachev <[email protected]> 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` (without failing the test). >> >> ## How To >> >> To redirect stderr and later check the exceptions, surround your code with >> >> `ErrorLoggingUtility.suppressStderr()` and either >> `ErrorLoggingUtility.checkStderr()` or >> `ErrorLoggingUtility.checkAndRestoreStderr()` (ideally, in the `finally` >> block). >> >> To simply undo redirection, without checking, call >> `ErrorLoggingUtility.restoreStderr()`. >> >> To add the check to all the tests in the file, one can call the above >> mentioned methods inside `@BeforeEach` and `@AfterEach`. >> >> ## Miscellaneous >> >> For reviewers' convenience, the first commit contains the main change, the >> second fixes the misspelt name of the utility class, the rest are trivial. >> >> ## Questions >> >> - should we fail the current test with `Assertions.fail()` in case of a >> mismatch? > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > moved utility 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). > > 1. 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: > > 1. the utility needs to be moved to another package > 2. 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](https://github.com/openjdk/jfx/pull/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. > 3. 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. modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java line 81: > 79: private static PrintStream stderr; > 80: private static AccumulatingPrintStream stderrCapture; > 81: private static int checked; This is unused (only ever written) and can be removed. modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java line 127: > 125: stderr.println("Mismatch in thrown exceptions:\n > expected=" + exp + "\n observed=" + errors); > 126: stderr.println(text); > 127: } A mismatch should fail the test. modules/javafx.base/src/test/java/test/util/ErrorLoggingUtility.java line 184: > 182: // catches lines starting with things like "Exception in > thread "main" java.lang.RuntimeException:" > 183: "^" + > 184: "(?:" + This might be easier to read and maintain with text blocks. ------------- PR Review: https://git.openjdk.org/jfx/pull/1897#pullrequestreview-3231265800 PR Comment: https://git.openjdk.org/jfx/pull/1897#issuecomment-3300210311 PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2353364648 PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2353453548 PR Review Comment: https://git.openjdk.org/jfx/pull/1897#discussion_r2353554564
