On Mon, 29 Jan 2024 15:39:32 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java >> line 65: >> >>> 63: } >>> 64: >>> 65: waiter.await(GRACE_PERIOD, TimeUnit.SECONDS); >> >> I don't think this will do what you want. This will return almost >> immediately, before any of the animation has run (as soon as the runnable >> that sets the uncaught exception handler finished). What I think you need is >> something more like this: >> >> >> Platform.runLater(() -> registerExceptionHandler()); >> assertTrue(waiter.await(GRACE_PERIOD, TimeUnit.SECONDS)); >> >> // start the 10 threads... >> >> Util.sleep(GRACE_PERIOD); >> >> >> The await ensures that the uncaught exception handler has been registered >> before starting the threads. The sleep then runs the threads for a fixed >> amount of time. > >> I don't think this will do what you want. This will return almost >> immediately, before any of the animation has run (as soon as the runnable >> that sets the uncaught exception handler finished). > > I observed that this setup works correctly for the `AnimationTimer` test. The > test does wait for `GRACE_PERIOD` seconds when there is no exception and then > succeeds, or catches and fails immediately when there onw is thrown. The > `Animation` test was more finicky, but I think that is because I'm not > simulating a processing load properly. > >> The await ensures that the uncaught exception handler has been registered >> before starting the threads. > > While technically true, the handler register in a negligible amount of time > compared to the grace period. Because exceptions are thrown continuously > )it's not a one-time event), the handler will never miss a failure. > > I will change it for correctness sake. I misread it. I see now that you only countdown when there is a failure. In that case, it seems fine as you have it (unless you also want to have a second latch that ensures that the uncaught event handler is in place prior to submitting the runnables). You might want to add a comment as to the purpose of the latch, since it wasn't initially obvious to me. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469812959
