On Tue, 15 Nov 2022 18:00:59 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> 1. Introduced the following utility methods:
> - Util.launch
> - Util.shutdown
> - Util.waitForLatch
> 2. Fixed the out-of order calls to Stage.hide() and Platform.exit() in many
> tests' shutdowns.
> 3. Replaced local waitForLatch copies with Util.waitForLatch
I left a couple quick comments on the new utility methods in `test.util.Util`.
Have you run a full test (including robot) on all three platforms?
> Fixed the out-of order calls to Stage.hide() and Platform.exit() in many
> tests' shutdowns.
I am not aware of any out of order calls. Perhaps you are referring to the
following pattern:
Platform.runLater(() -> stage.hide());
Platform.exit();
That is actually valid, since `Platform.exit()` can be called on any thread.
The FX runtime doesn't exit until all pending runnables are run. Doing the exit
on the FX application thread is fine, too.
tests/system/src/test/java/test/util/Util.java line 317:
> 315: * @param args - command line arguments
> 316: */
> 317: public static <T extends Application> void launch (
This looks like a useful utility. I think it would be helpful to either remove
the timeout parameter entirely, or else have a variant of this that doesn't
take the timeout value, and use a default timeout value. Most tests use 10 or
15 seconds so standardizing on 15 seems reasonable.
Also, many of the tests use `Platform.startup` since they don't need to launch
an application. Have you looked at providing a similar utility method for those
cases?
tests/system/src/test/java/test/util/Util.java line 338:
> 336: public static void shutdown(Stage... stages) {
> 337: // why isn't runAndWait() exposed as a public API?
> 338: PlatformImpl.runAndWait(() -> {
Not providing a public `runAndWait` was a deliberate choice. In any case, tests
or utilities that need to wait should use the `runAndWait` method in this Util
class in preference to `PlatformImpl`. For one thing it will throw exceptions
back to the caller where as the `PlatformImpl` method will not.
I think `Platform.runLater` should be sufficient here, but as long as the tests
are still stable when using `runAndWait`, I don't have a strong objection.
-------------
PR: https://git.openjdk.org/jfx/pull/950
