On Wed, 10 Mar 2021 14:17:57 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> The idea behind the fix is good. A few changes are needed: >> >> 1. The check should be split into two separate `if` statements, each with >> their own error message (see inline). >> >> 2. This should be documented in the API docs for the Application::launch >> method as an additional case that will throw an IllegalStateException. It >> will need a reasonably trivial CSR since we are specifying a new case that >> will cause an exception to be thrown. >> >> 3. The system test need to be broken up into separate files, one for each >> `@Test` method, since application launching tests need to run in their own >> JVM. If you want to share any code, you could refactor it into a common >> class that has methods (not annotated with `@Test`) to perform the testing, >> and separate classes that will subclass the common class, each with a single >> `@Test` method that simply calls into the method in the common class to do >> the testing. See any number of examples in >> `tests/system/src/test/java/test/com/sun/javafx/application/` (which is also >> a better location for your new test). You will want to add a cleanup method >> annotated with `@AfterClass` that calls `Platform.exit()`. I think three >> tests would be good: >> * Initalize the FX runtime via Platform.startup and then launch an >> Application on another thread (should succeed) >> * Initalize the FX runtime via Platform.startup and then launch an >> Application ~~on another thread~~ the FX Application Thread (should throw >> ISE) >> * Initalize the FX runtime via Application.launch and then launch a >> second Application on another thread (should throw ISE). >> >> >> I note that when I run your test, it hangs (on Windows...I didn't try it on >> other platforms). >> >> gradle --info -PFULL_TEST=true cleanTest :systemTests:test --test >> InitializeJavaFXTest >> >> This might be resolved by ensuring each test is in its own JVM as requested >> above. >> >> >> Additional comments are inline. > >> * Initalize the FX runtime via Platform.startup and then launch an >> Application on another thread (should throw ISE) > > That should be: > > * Initalize the FX runtime via Platform.startup and then launch an > Application on the FX Application thread (should throw ISE) Contribution.MD states, that it's automatically checked for whitespace errors? Is this not true? As a clarification, this PR restores the previous behavior before Platform.startup. With this PR Application.launch and Platform.startup behaves the same. ------------- PR: https://git.openjdk.java.net/jfx/pull/421
