On Wed, 10 Mar 2021 22:35:31 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
> updated the javadoc to mention the new case.
The fix looks good. I have a couple comments on the tests, mainly about making
them more robust by having only a single `@Test` method in each file.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXStartupTest.java
line 46:
> 44:
> 45: @Test (timeout = 15000)
> 46: public void testStartupThenLaunch() throws Exception {
It will be more robust for these two test methods to be in separate files.
Otherwise they might interfere with each other. If `testStartupThenLaunchInFX`
runs first, then an error in that test (e.g., if the fix isn't applied and it
times out) will cause the `testStartupThenLaunch` to fail. Conversely, if
`testStartupThenLaunch` runs first, then the system is in a state where the
Application has already been started by the time `testStartupThenLaunchInFX`
runs. It will still pass, but won't be a solid test of the fix, since that case
would fail today.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunchTest.java
line 46:
> 44:
> 45: @Test (timeout = 15000)
> 46: public void testStartupThenLaunch() throws Exception {
It will be more robust for these two test methods to be in separate files.
Otherwise they might interfere with each other.
tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXBase.java
line 80:
> 78: System.out.println("Finished launch!");
> 79: Assert.fail("Error: No Exception was thrown - expected
> IllegalStateException");
> 80: } catch (IllegalStateException e) {
Maybe add a comment that this exception is expected?
-------------
PR: https://git.openjdk.java.net/jfx/pull/421
