On Mon, 3 Feb 2025 21:47:11 GMT, Andy Goryachev <[email protected]> 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
