On Fri, 12 Mar 2021 16:29:35 GMT, Kevin Rushforth <[email protected]> wrote:
>> 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.
I've made all the changes in the comments, and also changed the names of the
tickets and the PR.
For some reason, I can't interact with the two unclosed comments.
I've removed the unreachable code, and the print-statements are also removed.
> 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.
Yes, I've fixed the method names!
-------------
PR: https://git.openjdk.java.net/jfx/pull/421
