RFR: 8348423: [TestBug] stress test Nodes initialization from a background thread [v8]

[Kevin Rushforth]

On Mon, 3 Feb 2025 21:47:11 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Created a test that validates various Nodes can be initialized in a background thread.
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   more jitter

Overall, this looks good. I think the framework is in a good enough place to integrate it now; we can always make improvements in the future if needed.

Here are a couple general observations based on my testing (mostly on Windows):

1. Once a test fails, subsequent tests are likely to be impacted. I've seen several instances of a timeouts in later tests after a failure (when I enable a test associated with a known bug). In at least one case, the visible stage froze and became non-responsive. This isn't surprising to me, given that all tests are run in the same process. If any of the exceptions hit the FX application thread it will likely cause this behavior.
2. Some of the disabled tests only fail infrequently -- meaning that either the payload for the test or the 5 second duration might not be enough to trigger the bug reliably (although race conditions are notoriously hard to test "reliably"). We can look at these individual cases when reviewing the bug fix that will enable the corresponding test.

I left a couple in-line questions and comments. Many would be questions for follow-up work (if at all). I'll re-approve if you decide to make any changes now.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 158:

> 156:     // for debugging purposes: setting this to true will skip working tests
> 157:     // TODO remove once all the tests pass
> 158:     private static final boolean SKIP_TEST = false;

I presume this is just for your convenience while developing this test as a way to run only the test you are interested in?

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 486:

> 484:             return c;
> 485:         }, (c) -> {
> 486:             c.setPannable(nextBoolean());

Do you think it's worth adding some scrolling here? That might be a good follow-on test enhancement.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 501:

> 499:         }, (c) -> {
> 500:             c.setEditable(nextBoolean());
> 501:             accessControl(c);

Is it worth adding code to increment / decrement the spinner?

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 712:

> 710:             TreeItem<String> root = new TreeItem<>(null);
> 711:             return root;
> 712:         };

`gen` is an unused local var.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 775:

> 773:         int threadCount = 1 + Runtime.getRuntime().availableProcessors() * 2;
> 774:         AtomicBoolean running = new AtomicBoolean(true);
> 775:         int additionalThreads = 2; // jiggler + tight loop

I wonder if it is worth having more than 1 "tight loop" thread? Maybe dedicate 1/2 of the threads to a tight loop (`generator` in the loop) and 1/2 to access (`generator` once and `operation` in the loop) This is something that you could consider for a future improvement,

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 782:

> 780:             new Thread(() -> {
> 781:                 try {
> 782:                     while (running.get()) {

Maybe exit early if there is a failure, e.g., `while (running.get() && !failed.get())`?

In order to be effective, the `UncaughtExceptionHandler` would need to interrupt the main thread (so that `sleep(DURATION)` terminates early), so we would need to register the main thread with the `UncaughtExceptionHandler`.

This could be a follow-on if we decide it is helpful (I'm not sure it's worth doing).

tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 53:

> 51:     /** Stage valid only during test */
> 52:     protected static Stage stage;
> 53:     protected static BorderPane content;

MInor: I think `root` , `contentRoot`, or `contentPane` would be a better name here (it's a container for content, not the content itself as further indicated by the `setContent` method).

tests/system/src/test/java/test/robot/testharness/RobotTestBase.java line 60:

> 58:         @Override
> 59:         public void start(Stage primaryStage) throws Exception {
> 60:             stage = primaryStage;

Maybe `stage.setAlwaysOnTop(true)`? We do this for most other robot tests.

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

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1690#pullrequestreview-2594237735
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1942002563
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943625091
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943625678
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943628584
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943631711
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943425979
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943446345
PR Review Comment: https://git.openjdk.org/jfx/pull/1690#discussion_r1943450309