On Fri, 12 Mar 2021 14:49:41 GMT, Florian Kirmaier <fkirma...@openjdk.org>
wrote:
>> Fixing deadlock when calling Application.launch in the FXThread after
>> Platform.startup
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8263322
> Seperated the unit-tests into seperate files
Thanks for splitting up the tests. I ran them with the fix and all pass, I then
ran them without the fix and only the one I expected to fail did.
Before approving this, can you change the title of the JBS bug and CSR issue to
match (after doing that you will also need to update this PR title) as
recommended in the CSR? My suggestion is something like:
Calling Application.launch on FX thread should throw IllegalStateException, but
causes deadlock
Another thing I recommend is to comment out or remove the print statements in
the tests. They are useful while debugging, but in production we prefer tests
to only print something as a warning or error.
I also left a few inline comments.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXBase.java
line 35:
> 33: public class InitializeJavaFXBase {
> 34:
> 35:
Minor: there is an extra blank line here
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunchBase.java
line 22:
> 20: Application.launch(InitializeApp.class);
> 21: }).start();
> 22: appLatch.await(5, TimeUnit.SECONDS);
You should `assertTrue` the return value of `await`.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXStartupBase.java
line 15:
> 13: latch.countDown();
> 14: });
> 15: latch.await(5, TimeUnit.SECONDS);
You should `assertTrue` the return value of `await`.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch1Test.java
line 41:
> 39:
> 40: @Test (timeout = 15000)
> 41: public void testStartupThenLaunchInFX() throws Exception {
Would something like `testLaunchThenLaunchInFX` be a better name? As it is, it
uses the same name as the test that really does use `Platform.startup` which I
found confusing.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch2Test.java
line 41:
> 39:
> 40: @Test (timeout = 15000)
> 41: public void testStartupThenLaunch() throws Exception {
Would something like `testLaunchThenLaunch` be a better name?
-------------
PR: https://git.openjdk.java.net/jfx/pull/421
