From a42ededd0a4f986397105ea5a3e835ac13c3777c Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 26 Jun 2018 11:20:55 +0800 Subject: [PATCH] Fix stalling HelpWindowTest When running `gradlew headless allTests`, gradle will sometimes stall on HelpWindowTest. While we do not know the exact cause of this stalling, we have narrowed it down to the following conditions[1] which must all be present for the stalling to occur: 1. The test must be running in headless mode. 2. There must be two stages created, one with a WebView, and one with no scene. 3. Both stages must be shown. Since it is pretty difficult and time-consuming to debug and find out the exact cause of this stalling, let's fix this issue by removing one of its trigger conditions. (1) obviously cannot be removed, since we want headless testing to work on our code. For (2), the HelpWindow needs to have a WebView to show the user guide, and so we can't remove the WebView. But really, stepping back a bit, we can see that the whole test setup which leads to (2) and (3) being present in the first place is pretty strange: // (a) guiRobot.interact(() -> helpWindow = new HelpWindow()); // (b) Stage helpWindowStage = FxToolkit.setupStage((stage) -> stage.setScene(helpWindow.getRoot().getScene())); // (c) FxToolkit.showStage(); helpWindowHandle = new HelpWindowHandle(helpWindowStage); [...] // (d) guiRobot.interact(helpWindow::show); In (a), we create an instance of HelpWindow, which has its own Stage which can be retrieved with HelpWindow#getRoot(). However, in (b) we transfer the HelpWindow's scene to the TestFX's primary stage, leaving the helpWindow with no scene. But: there is no reason to use the TestFX's registered primary stage when HelpWindow already has its own Stage. And in (c), we show the TestFX primary stage, and then later in (d) we show the helpWindow. However, given that helpWindow does not have a scene and is thus not showing anything, it would be deceptive to use it for tests which are meant to verify that the application works correctly. As such, let's fix the stalling by fixing the root of the problem: the bogus test setup. Rather than having two stages, let's just use HelpWindow's stage directly for all tests, and register it as the TestFX primary stage using FxToolkit#registerStage(...). This has the effect of removing trigger conditions (2) and (3), since we now only have one Stage, thus fixing the stalling. NOTE: Even with this fix, focus_helpWindowNotFocused_focused() still cannot be run in headless mode, although for a different reason compared to when this skip was introduced in 2cf5b98 (HelpWindowTest: skip test with buggy behavior in headless mode, 2018-04-21). Now, this test will fail due to a different JavaFX bug[2]. Clarify this by adding comments explaining why the test is skipped in headless mode. --- [1] Minimal reproducer for the stalling: package seedu.address.ui; import org.junit.Test; import org.testfx.api.FxRobot; import org.testfx.api.FxToolkit; import javafx.scene.Scene; import javafx.scene.web.WebView; import javafx.stage.Stage; public class HelpWindowTest { @Test public void test() throws Exception { FxToolkit.registerPrimaryStage(); FxRobot fxRobot = new FxRobot(); fxRobot.interact(() -> { Stage stage1 = new Stage(); stage1.setScene(new Scene(new WebView())); Stage stage2 = new Stage(); // Order in which stages are shown does not matter stage1.show(); stage2.show(); }); // May or may not stall here depending on your luck fxRobot.interact(() -> {}); } } [2] https://github.com/javafxports/openjdk-jfx/issues/50 --- .../java/seedu/address/ui/HelpWindowTest.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/test/java/seedu/address/ui/HelpWindowTest.java b/src/test/java/seedu/address/ui/HelpWindowTest.java index 0dfb9842284c..20d721cd0e45 100644 --- a/src/test/java/seedu/address/ui/HelpWindowTest.java +++ b/src/test/java/seedu/address/ui/HelpWindowTest.java @@ -23,13 +23,13 @@ public class HelpWindowTest extends GuiUnitTest { @Before public void setUp() throws Exception { guiRobot.interact(() -> helpWindow = new HelpWindow()); - Stage helpWindowStage = FxToolkit.setupStage((stage) -> stage.setScene(helpWindow.getRoot().getScene())); - FxToolkit.showStage(); - helpWindowHandle = new HelpWindowHandle(helpWindowStage); + FxToolkit.registerStage(helpWindow::getRoot); + helpWindowHandle = new HelpWindowHandle(helpWindow.getRoot()); } @Test - public void display() { + public void display() throws Exception { + FxToolkit.showStage(); URL expectedHelpPage = HelpWindow.class.getResource(USERGUIDE_FILE_PATH); assertEquals(expectedHelpPage, helpWindowHandle.getLoadedUrl()); } @@ -42,12 +42,20 @@ public void isShowing_helpWindowIsShowing_returnsTrue() { @Test public void isShowing_helpWindowIsHiding_returnsFalse() { - guiRobot.interact(helpWindow.getRoot()::hide); assertFalse(helpWindow.isShowing()); } @Test public void focus_helpWindowNotFocused_focused() throws Exception { + // TODO: This test skip can be removed once this bug is fixed: + // https://github.com/javafxports/openjdk-jfx/issues/50 + // + // When there are two stages (stage1 and stage2) shown, + // stage1 is in focus and stage2.requestFocus() is called, + // we expect that stage1.isFocused() will return false while + // stage2.isFocused() returns true. However, as reported in the bug report, + // both stage1.isFocused() and stage2.isFocused() returns true, + // which fails the test. assumeFalse("Test skipped in headless mode: Window focus behavior is buggy.", guiRobot.isHeadlessMode()); guiRobot.interact(helpWindow::show);