On Wed, 13 Nov 2019 21:53:13 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier <fkirma...@openjdk.org> > wrote: > >> The pull request has been updated with additional changes. >> >> ---------------- >> >> Added commits: >> - 5cd96a56: JDK-8200224 >> >> Changes: >> - all: https://git.openjdk.java.net/jfx/pull/25/files >> - new: https://git.openjdk.java.net/jfx/pull/25/files/85c87628..5cd96a56 >> >> Webrevs: >> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.02 >> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.01-02 >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >> Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod >> Patch: https://git.openjdk.java.net/jfx/pull/25.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 > > I see that when you made your earlier comment regarding `System.exit`, you > really meant that the existing swing test was calling `Platform.exit`, which > isn't the same thing; it does shut down the JavaFX runtime, which is intended. > > The problem you are running into is that gradle runs multiple tests in the > same VM and in an undefined, and unpredicable, order. This means that tests > need to take care not to alter or rely on global state such as threads, > static fields, global JavaFX state, and the running (or lack thereof) of the > JavaFX runtime. The swing tests violate this rule and are therefore > inherently unstable. The only reason this hasn't been a problem up to now is > that the javafx.swing module contains a single test class. I will file a new > test bug to address it -- probably by moving that test to the `systemTests` > project. > > You will need to move your test into the `systemTests` project under the > `tests/system/` directory. Such tests are valid in the system tests project > because we run each test in that project in its own VM. Once your proposed > test is robust (meaning consistently catches the bug without your fix and > consistently passes with your fix) on all platforms without using Robot, then > `tests/system/src/test/java/test/javafx/embed/swing/` would be the best place > to put your new test. > > Regarding the test itself, it still doesn't fail for me on Windows without > your fix. I ran the test program attached to the bug and I see something that > might help explain this. That test program creates a pair of JFXPanel objects > and adds both to the JPanel. If I first click on the first one, then it only > shows a single click. Every time after that, if I click on a new JFXPanel, > then I get 2 clicks. If, instead, I click on the second JFXPanel right after > starting the program, I get 2 clicks the first time. With that in mind, I > modified your test to add a dummy JFXPanel to the JPanel before the one you > are sending the mouse pressed event to, and then it then does what I expect: > it catches the bug without your fix and passes with your fix. That might help > you come up with a more robust test. I added some specific comments on the > test as well. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java > line 46: > >> 45: >> 46: >> 47: public class SwingFXUtilsTest { > > Your proposed fix for this test class is not the right fix, and needs to be > reverted. It highlights an existing bug with the swing tests, which I will > address in general comments. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java > line 100: > >> 99: jpanel.add(fxPnl); >> 100: jframe.setContentPane(jpanel); >> 101: jframe.setVisible(true); > > You should call `jframe.pack()` here. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java > line 128: > >> 127: >> 128: Thread.sleep(100); // there should be no pressed event after >> the initial one. Let's wait for 0.1s and check again. >> 129: assertEquals(1, pressedEventCounter[0]); > > I recommend using 500 msec so we don't risk missing a failing click on slower > systems. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java > line 60: > >> 59: public static void doSetupOnce() { >> 60: if (setupIsDone) return; >> 61: Platform.startup(() -> { > > This is not needed if you revert the changes to the other test class. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java > line 130: > >> 129: assertEquals(1, pressedEventCounter[0]); >> 130: } >> 131: } > > You will want to add a cleanup method, annotated with `@AfterClass` to call > `Platform.exit` and dispose of the JFrame. > > modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java > line 106: > >> 105: Scene scene = new Scene(new Group()); >> 106: scene.getRoot().requestFocus(); >> 107: > > I don't think requesting focus is necessary (or desired). > > ---------------- > > Changes requested by kcr (Lead). It's now removed done I've kept the logic of the other JFXPanelTest. Should i change it? removed I've removed it PR: https://git.openjdk.java.net/jfx/pull/25
