Skip to content

Commit

Permalink
Fix stalling HelpWindowTest
Browse files Browse the repository at this point in the history
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] javafxports/openjdk-jfx#50
  • Loading branch information
pyokagan committed Jun 28, 2018
1 parent 8d2322b commit a42eded
Showing 1 changed file with 13 additions and 5 deletions.
18 changes: 13 additions & 5 deletions src/test/java/seedu/address/ui/HelpWindowTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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);

Expand Down

0 comments on commit a42eded

Please sign in to comment.