On Wed, 29 Jan 2025 23:54:44 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Created a test that validates various Nodes can be initialized in a > background thread. I like the overall structure of this test. I left several mostly "fit and finish" type comments. I will test it as part of finishing the review. What testing have you done? I recommend the following CI headful test runs: 1. A run using this branch "as is" to verify no failures on our headful systems 2. A run using a branch with all of the `@Disabled` tests enabled to see how well the test catches the bugs. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 136: > 134: * Tests Node initialization from a background thread, per > 135: * > 136: * https://openjfx.io/javadoc/23/javafx.graphics/javafx/scene/Node.html I prefer not to put external links to specific versions of the docs. I would just say "per the Node docs" or similar. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 146: > 144: * - WebView > 145: * > 146: * The test creates a visible node of a given type, and at the same > time, starts a number of background threads Suggestion: "creates a visible node" --> "creates a visible node on the JavaFX application thread" tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 272: > 270: return c; > 271: }, (c) -> { > 272: c.show(); // does not fail here, unlike DatePicker? Interesting. I still think we should disallow it as I mentioned in the `DatePicker` case. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 305: > 303: return c; > 304: }, (c) -> { > 305: c.show(); // fails here As mentioned in the JBS issue, I think we should consider filing an issue to update the spec of `ComboBoxPopupControl::show` to require calling it on the JavaFX app thread (throwing an exception if called off thread). Either way, we should ensure that the skins themselves never call show unless the control is showing. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 377: > 375: c.setPopupSide(Side.RIGHT); > 376: accessControl(c); > 377: c.show(); This is another case of a `show` method that we should consider changing to require access only on the FX app thread. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 745: > 743: @Override > 744: public void run() { > 745: try { Suggestion: use a lambda rather than an inner class here . tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 755: > 753: inFx(() -> { > 754: operation.accept(visibleNode); > 755: }); Does this need to be done for each thread? I can see why it might make sense to do that, as it preserves the relative frequency of operations on the FX app thread versus the frequency off thread regardless of how many threads you have. It would risk flooding the FX event queue if the number of background threads were huge, but you limit it based on the number of physical HW threads, so this seems OK. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationBackgroundThreadTest.java line 779: > 777: > 778: private static boolean nextBoolean() { > 779: return new Random().nextBoolean(); It is a best practice when using Random to share a single instance and not allocate a new one each time. It is more performant and more random. tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 42: > 40: > 41: /** > 42: * Similar to VisualTestBase, but more convenient. If you are going to compare it to VisualTestBase, you need a more nuanced statement than just "more convenient" which could mislead someone into thinking that this is a replacement. It isn't. VisualTestBase is specifically intended for screen capture tests, so creates stages as `UNDECORATED` and `alwaysOnTop`, and provides pixel capture and comparison utilities. I recommend describing this class on its own terms. tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 65: > 63: stage.setScene(scene); > 64: stage.setWidth(STAGE_WIDTH); > 65: stage.setHeight(STAGE_HEIGHT); Typically, best practice is to set the width / height of the Scene, rather than the Stage, especially for decorated stages, although there are some times you might want to set the stage. It might not matter for what you are using it for. tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 81: > 79: @AfterAll > 80: public static void teardownOnce() { > 81: Util.shutdown(); Should you pass "stage" here, or do you ensure that the stage is always closed some other way? tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 173: > 171: * @param r the code to execute > 172: */ > 173: public void inFx(Runnable r) { Minor: Maybe call this method "runAndWait" instead? That would make it clearer what it does. tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 176: > 174: Util.runAndWait(() -> { > 175: r.run(); > 176: }); Minor: This can be reduced to `Util.runAndWait(r)` tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 183: > 181: * @param milliseconds the number of milliseconds to sleep > 182: */ > 183: protected void sleep(int milliseconds) { Maybe use `long` rather than `int` to match `Thread.sleep` and `Util.sleep`? It probably doesn't matter in practice. ------------- PR Review: https://git.openjdk.org/jfx/pull/1690#pullrequestreview-2587514918 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937765444 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937767206 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937799226 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937798147 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937801455 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937772645 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937776420 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937848601 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937750555 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937753823 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937755797 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937758040 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937762531 PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1937759121
