On Tue, 7 Apr 2020 09:51:18 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8241840 >> Some code cleanups > > tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 133: > >> 132: >> 133: Assert.assertNull(weakReference.get()); >> 134: } > > This assert check call should be added to the @Test method. > I would recommend to refer [this > method](https://github.com/openjdk/jfx/blob/364c64a2e55b561df4ca1afc85c91054b339eea8/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java#L1998) > from ListViewTest. Unifying memory-tests is something I discussed in my last PR. This code is based on my previous pull request: https://github.com/openjdk/jfx/pull/71 I would like to keep this version which I've used there. But it really has to be a library/utility-class which is used across the project instead of reinventing it on every test. > tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 84: > >> 83: counter += 1; >> 84: testLeakOnce(); >> 85: } > > If the test has constant behavior on every run, then only one run should be > done. done > modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 354: > >> 353: } >> 354: if(Window.focusedWindow == this) { >> 355: Window.focusedWindow = null; > > Please correct format by adding space after `if`. done > tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 113: > >> 112: }); >> 113: >> 114: Assert.assertTrue("Timeout, waiting for runLater. ", >> leakLatch.await(15, TimeUnit.SECONDS)); > > Is it really required to nest the `Platform.runLater()` calls ? Can you > please check if this code can be simplified by > making the calls sequential, Consider using `Util.runAndWait()` done > tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 53: > >> 52: System.setProperty("monocle.platform","Headless"); >> 53: } >> 54: > > The test will always run with Monocle. I see that if this static block is > removed then the test fails on Windows 10. > Can you please verify all the platforms and change the test such that it runs > on all platforms/ combinations. I wasn't able to reproduce the Problem on Window10 with VirtualBox. How should I do it? Add a second test class without the static code? Do you have a good recommendation on how to add it? ------------- PR: https://git.openjdk.java.net/jfx/pull/153
