HelpWindowTest: fix failing test in non-headless mode #880

yamidark · 2018-04-16T12:53:05Z

HelpWindowTest#focus_helpWindowNotFocused_focused() test checks that
the HelpWindow#focus() method will cause the HelpWindow to be in focus
when called. The test first asserts that the HelpWindow is not in focus
first after it is shown, being calling this method and asserting that
the HelpWindow is now in focus.

When tests are run in non-headless mode, the HelpWindow will be in
focus immediately after it is shown, thus our first assertion is
incorrect, causing the test to consistently fail in non-headless mode.

Let's add a method GuiRobot#removeFocus(), and update the test to call
this method to remove focus from the HelpWindow after it is shown
to ensure the first assertion is correct in non-headless mode.

CanIHasReview-bot · 2018-04-16T13:14:38Z

v1

@yamidark submitted v1 for review.

[v1 1/1] HelpWindowTest: fix failing test in non-headless mode

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamidark · 2018-04-16T13:16:01Z

@vivekscl @Rinder5 for your review.

vivekscl

[1/1] Commit Message
1st para:
HelpWindowTest#focus_helpWindowNotFocused_focused() test checks that
the HelpWindow#focus() method will cause the HelpWindow to be in focus
when called. The test first asserts that the HelpWindow is not in focus
~~first after~~ when it is first shown ~~being calling this method and asserting that~~ and then asserts that
the HelpWindow comes into focus after calling HelpWindow#focus().

2nd para:
When tests are run in non-headless mode, the HelpWindow will be in
focus ~~immediately after~~ when it is first shown, thus rendering our initial assumption is
false and causing the test to ~~consistently fail in non-headless mode~~ fail.

3rd para:
Let's add a method GuiRobot#removeFocus(), and update the HelpWindowTest#focus_helpWindowNotFocused_focused() to call
this method to remove focus from the HelpWindow ~~after~~ when it is first shown,
to ensuring that the initial assumption is true.

vivekscl · 2018-04-17T03:29:54Z

src/test/java/guitests/GuiRobot.java

+     * Removes focus from the current stage.
+     */
+    public void removeFocus() throws Exception {
+        FxToolkit.setupStage(dummyStage -> dummyStage.requestFocus());


Should there be a try-catch block instead of simply throwing the exception?

Hmm, do you mean something like what UiPartRule does:

catch (TimeoutException te) { throw new AssertionError("Timeout should not happen.", te); }

Since I will be inling the method directly into the test, should we still do a try-catch block in the test?

yamidark · 2018-04-17T06:41:35Z

Actually, should I add in the comments that the removeFocus() does not work in headless mode due to an issue with Monocle, and make the ~~test~~ method return immediately if in headless mode?

pyokagan · 2018-04-17T11:53:17Z

src/test/java/guitests/GuiRobot.java

@@ -39,6 +40,13 @@ public void pauseForHuman() {
        sleep(PAUSE_FOR_HUMAN_DELAY_MILLISECONDS);
    }

+    /**
+     * Removes focus from the current stage.


A more accurate description would be "focus on the registered TestFX stage". If the "current stage" is the registered TestFX stage, then this won't "remove focus" from it.

With that said, I think this is too oddly specific to be inside GuiRobot. We don't usually "remove focus", we just focus on another window instead.

Just inline this directly in the test?

pyokagan · 2018-04-17T12:02:21Z

@yamidark

Actually, should I add in the comments that the removeFocus() does not work in headless mode due to an issue with Monocle, and make the test method return immediately if in headless mode?

Actually, I'm not sure why the test is even run in headless mode? For headless mode to be useful its results need to match the real-world head-full mode, otherwise its test results are meaningless since the fact that a test passes/fails in headless mode does not mean that it would pass/fail in the real-world.

If Monocle's window focusing behavior does not match the "real world" then there is no point trying to run focus tests with it. Better to be explicit about that by printing a gigantic "TEST SKIPPED IN HEADLESS MODE, RUN IT IN HEAD-FULL MODE INSTEAD" message.

pyokagan · 2018-04-17T14:06:41Z

If Monocle's window focusing behavior does not match the "real world" then there is no point trying to run focus tests with it.

Actually I take that back. There's still some value in the test if it fails if requestFocus is not called for whatever reason.

pyokagan · 2018-04-17T17:34:24Z

@yamidark

Sorry, I re-assert again that the test should not be run in headless mode :-). Its relying on undefined behavior, and I think that is part of the reason why HelpWindowTest is stalling our builds.

Consider this: (Make sure to run it in headless mode)

package seedu.address.ui;

import static org.junit.Assert.assertTrue;

import org.junit.Test;
import org.testfx.api.FxRobot;
import org.testfx.api.FxToolkit;

import javafx.scene.Scene;
import javafx.scene.layout.Pane;
import javafx.stage.Stage;

public class HelpWindowTest {
    Stage stage1;
    Stage stage2;

    @Test
    public void test() throws Exception {
        FxRobot fxRobot = new FxRobot();
        FxToolkit.registerPrimaryStage();

        fxRobot.interact(() -> {
            stage1 = new Stage();
            stage1.setScene(new Scene(new Pane()));

            stage2 = new Stage();
            stage2.setScene(new Scene(new Pane()));
        });

        fxRobot.interact(stage1::show);
        assertTrue(stage1.isFocused()); // NOTE: stage1 is focused once it is shown!!!
    }
}

Note that upon calling stage1::show, stage1 is immediately focused.

Now, consider the following case where stage1 and stage2 share the same scene:

package seedu.address.ui;

import static org.junit.Assert.assertTrue;

import org.junit.Test;
import org.testfx.api.FxRobot;
import org.testfx.api.FxToolkit;

import javafx.scene.Scene;
import javafx.scene.layout.Pane;
import javafx.stage.Stage;

public class HelpWindowTest {
    Stage stage1;
    Stage stage2;

    @Test
    public void test() throws Exception {
        FxRobot fxRobot = new FxRobot();
        FxToolkit.registerPrimaryStage();

        fxRobot.interact(() -> {
            Scene scene = new Scene(new Pane());

            stage1 = new Stage();
            stage1.setScene(scene);

            stage2 = new Stage();
            stage2.setScene(scene);
        });

        fxRobot.interact(stage1::show);
        assertTrue(stage1.isFocused()); // ASSERTION FAILURE!!!!
    }
}

stage1 is now not automatically focused when it is shown.

So, on normal operation in headless mode stages are supposed to be focused automatically when they are shown. But when stages share a Scene (which the JavaFX docs specifically say not to), then the focus does not occur.

I think this is exactly what is happening in HelpWindowTest. Specifically this:

Stage helpWindowStage = FxToolkit.setupStage((stage) -> stage.setScene(helpWindow.getRoot().getScene()));

I also believe that this is tied to HelpWindowTest randomly stalling our builds, since from my investigation it stalls due to a deadlock between the Application thread and the renderer thread because the Application thread miscounts the number of scenes.

yamidark · 2018-04-18T07:09:07Z

@pyokagan ~~Should I also make the other tests in HelpWindowTest also skip in headless mode?~~

Edit*:
Actually, since only the display test requires setting up of a helpWindowHandle thus needing a new helpWindowStage with the same Scene set up, we can just move the setting up of helpWindowHandle in display test alone.

Now, in focus_helpWindowNotFocused_focused test, the helpWindow will correctly be in focus after it is shown.
However, we still can't remove focus from it in headless mode, so we would still need to skip this test.

CanIHasReview-bot · 2018-04-19T07:41:50Z

v2

@yamidark submitted v2 for review.

[v2 1/1] HelpWindowTest: fix failing test in non-headless mode

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan · 2018-04-19T11:56:39Z

src/test/java/seedu/address/ui/HelpWindowTest.java

@@ -46,8 +49,16 @@ public void isShowing_helpWindowIsHiding_returnsFalse() {
    }

    @Test
-    public void focus_helpWindowNotFocused_focused() {
+    public void focus_helpWindowNotFocused_focused() throws Exception {
+        if (guiRobot.isHeadlessMode()) {


Any way of properly telling JUnit to skip the test? (So that it is reported as a skipped test rather than a test that succeeded)

Will look up on this.

You can use Assumption

Using assumeTrue or assumeFalse will make the test throw a TestAbortedException, causing gradle, travis and appveyor to fail, so it isn't really skipping the test.

Hmm that's weird, cos in the docs:

In direct contrast to failed assertions, failed assumptions do not result in a test failure; rather, a failed assumption results in a test being aborted.

Otherwise, maybe you can try conditional execution and see whether there's any output on the test being skipped.

Hmm that's weird, cos in the docs:
In direct contrast to failed assertions, failed assumptions do not result in a test failure; rather, a failed assumption results in a test being aborted.

Oh, I think I imported and used the wrong one, I used org.junit.jupiter.api.Assumptions instead of org.junit.Assume. Switching over to org.junit.Assume passes/ignores the test, but no message is shown to show that the test is skipped.

I'm referring to the org.junit.jupiter.api.Assumptions though, see the docs I linked above :P

I'm referring to the org.junit.jupiter.api.Assumptions though, see the docs I linked above :P

@_@, ok maybe I confuse with the other links I had xD.
Anyway, I can't seem to get conditional execution to work on my side:

@DisabledIf("guiRobot.isHeadlessMode()")

Should I just stick with org.junit.Assume? It doesn't seem to inform us when we skip the test though.

You can do something like this to print the names of skipped tests:

diff --git a/build.gradle b/build.gradle index 75a518e..5c0329e 100644 --- a/build.gradle +++ b/build.gradle @@ -2,6 +2,8 @@ // For more details take a look at the Java Quickstart chapter in the Gradle // user guide available at http://gradle.org/docs/4.6/userguide/tutorial_java_projects.html +import org.gradle.api.tasks.testing.logging.TestLogEvent + plugins { id 'java' id 'jacoco' @@ -117,12 +119,14 @@ allTests.dependsOn nonGuiTests test { systemProperty 'testfx.setup.timeout', '60000' - // Prints the currently running test's name in the CI's build log, - // so that we can check if tests are being silently skipped or - // stalling the build. - if (System.env.'CI') { - beforeTest { descriptor -> - logger.lifecycle("Running test: ${descriptor}") + testLogging { + events TestLogEvent.FAILED, TestLogEvent.SKIPPED + + // Prints the currently running test's name in the CI's build log, + // so that we can check if tests are being silently skipped or + // stalling the build. + if (System.env.'CI') { + events << TestLogEvent.STARTED } }

Still haven't figured out how to print the reason for test abortion though.

CanIHasReview-bot · 2018-04-20T11:49:54Z

v3

@yamidark submitted v3 for review.

[v3 1/1] HelpWindowTest: fix failing test in non-headless mode

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan

HelpWindowTest: fix failing test in non-headless mode

HelpWindowTest#focus_helpWindowNotFocused_focused() asserts that the
HelpWindow is not in focus when it is first shown, and then asserts
that the HelpWindow comes into focus after calling HelpWindow#focus().

When tests are run in non-headless mode, the HelpWindow will be in
focus when it is first shown, thus rendering our first assumption false
and causing the test to fail.

Let's update the test to focus on another TestFx Stage, which will
remove focus from the HelpWindow when it is first shown, ensuring that
the initial assumption is true.

OK, fixing the test to work in head-full mode is one change.

Also, since this test has an undefined behaviour when run in headless
mode due to different Stages sharing the same Scene, let's also skip
this test when run in headless mode.

This is a completely different change though, and should be put in a different commit.

You also need to explain why it is undefined behavior.

pyokagan · 2018-04-20T13:37:32Z

src/test/java/seedu/address/ui/HelpWindowTest.java

@@ -46,8 +47,12 @@ public void isShowing_helpWindowIsHiding_returnsFalse() {
    }

    @Test
-    public void focus_helpWindowNotFocused_focused() {
+    public void focus_helpWindowNotFocused_focused() throws Exception {
+        assumeFalse(guiRobot.isHeadlessMode());


Should use assumeFalse(String message, boolean b) to provide a reason for why the test is aborted.

CanIHasReview-bot · 2018-04-21T10:47:25Z

v5

@yamidark submitted v5 for review.

[v5 1/3] HelpWindowTest: fix failing test in non-headless mode
[v5 2/3] HelpWindowTest: skip test with buggy behavior in headless mode
[v5 3/3] build.gradle: enable logging of skipped tests

(📚 Archive) (📈 Interdiff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/5/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

pyokagan

[3/3]

build.gradle: enable logging of skipped tests

build.gradle settings does not enable logging of tests which may be

Probably not may be, since we want to only log the tests that were skipped.

skipped for certain reasons, such as buggy behavior in headless mode.

I think buggy behavior in headless mode needs more clarification. (Readers jumping to this commit from git-blame will not know the context)

So, something like:

Our build.gradle configuration does not log to the console tests that were
skipped. These tests may have been skipped for certain reasons, e.g. they do not support headless mode and should be run in non-headless mode instead.

As these tests that are skipped may actually be failing, we should
enable logging of these tests so that users know which tests that
needs to be tested separately.

which tests that needs -> which tests need

Lets update build.gradle to enable logging of tests are that skipped.

of tests are that skipped -> of tests that were skipped

pyokagan

Changes Requested.

CanIHasReview-bot · 2018-04-21T15:11:13Z

v6

@yamidark submitted v6 for review.

[v6 1/3] HelpWindowTest: fix failing test in non-headless mode
[v6 2/3] HelpWindowTest: skip test with buggy behavior in headless mode
[v6 3/3] build.gradle: enable logging of skipped tests

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/6/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

HelpWindowTest#focus_helpWindowNotFocused_focused() asserts that the HelpWindow is not in focus when it is first shown, and then asserts that the HelpWindow comes into focus after calling HelpWindow#focus(). When tests are run in non-headless mode, the HelpWindow will be in focus when it is first shown, thus rendering our first assumption false and causing the test to fail. Let's update the test to focus on another TestFx Stage, which will remove focus from the HelpWindow when it is first shown, ensuring that the initial assumption is true.

HelpWindowTest#focus_helpWindowNotFocused_focused test checks whether calling HelpWindow#focus() will bring the HelpWindow to focus. Normally, when HelpWindow#show() is called, the HelpWindow's Stage should automatically be in focus. However, when tests are run in headless mode, the HelpWindow in HelpWindowTest#focus_helpWindowNotFocused_focused test does not automatically come into focus after it is shown. This is due to HelpWindowStage in setUp() setting the HelpWindow's Scene to itself. According to the JavaFx docs for Window#setScene(Scene)[1], a Scene can only be on one Stage at a time, and setting a Scene on a different Stage will cause the old Stage to lose the reference. As a result, the HelpWindow's Stage will not have a reference to any Scene, resulting in the HelpWindow's Stage not coming into focus automatically, due to a bug with Monocle's headless mode. Let's skip HelpWindowTest#focus_helpWindowNotFocused_focused test when tests are run in headless mode as it is relying on buggy behavior. [1]: JavaFx docs for Window#setScene(Scene): https://docs.oracle.com/javase/8/javafx/api/javafx/stage/Window.html#setScene-javafx.scene.Scene-

Our build.gradle configuration does not log to the console tests that were skipped. These tests may have been skipped for certain reasons, e.g. they do not support headless mode and should be run in non-headless mode instead. As these tests that are skipped may actually be failing, we should enable logging of these tests so that users know which tests needs to be tested separately. Lets update build.gradle to enable logging of tests that were skipped.

pyokagan · 2018-04-22T13:52:04Z

@yamidark Please submit another version because your PR has been modified.

CanIHasReview-bot · 2018-04-22T13:52:55Z

v7

@yamidark submitted v7 for review.

[v7 1/3] HelpWindowTest: fix failing test in non-headless mode
[v7 2/3] HelpWindowTest: skip test with buggy behavior in headless mode
[v7 3/3] build.gradle: enable logging of skipped tests

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/880/7/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

vivekscl

LGTM 👍

CanIHasReview-bot added the s.ToReview label Apr 16, 2018

vivekscl reviewed Apr 17, 2018

View reviewed changes

pyokagan suggested changes Apr 17, 2018

View reviewed changes

yamidark force-pushed the fix-help-window-failing-test branch 3 times, most recently from ccaeedb to 11e7fcb Compare April 17, 2018 17:27

yamidark force-pushed the fix-help-window-failing-test branch 3 times, most recently from ae89610 to d3af275 Compare April 19, 2018 06:40

CanIHasReview-bot requested a review from pyokagan April 19, 2018 07:41

pyokagan suggested changes Apr 19, 2018

View reviewed changes

yamidark force-pushed the fix-help-window-failing-test branch 7 times, most recently from 738b105 to ee81a6f Compare April 20, 2018 11:06

CanIHasReview-bot requested a review from pyokagan April 20, 2018 11:49

pyokagan suggested changes Apr 20, 2018

View reviewed changes

pyokagan added the s.Ongoing label Apr 20, 2018

yamidark force-pushed the fix-help-window-failing-test branch 3 times, most recently from 6bf51c6 to 6e7cd01 Compare April 21, 2018 10:36

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 21, 2018

CanIHasReview-bot requested a review from pyokagan April 21, 2018 10:47

pyokagan reviewed Apr 21, 2018

View reviewed changes

pyokagan suggested changes Apr 21, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Apr 21, 2018

yamidark force-pushed the fix-help-window-failing-test branch from 6e7cd01 to 651d999 Compare April 21, 2018 14:45

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 21, 2018

CanIHasReview-bot requested a review from pyokagan April 21, 2018 15:11

pyokagan approved these changes Apr 21, 2018

View reviewed changes

pyokagan requested a review from damithc April 21, 2018 15:17

yamidark added 3 commits April 22, 2018 16:17

yamidark force-pushed the fix-help-window-failing-test branch from 651d999 to fc69d10 Compare April 22, 2018 08:18

damithc approved these changes Apr 22, 2018

View reviewed changes

pyokagan added s.ToMerge and removed s.ToReview labels Apr 22, 2018

vivekscl approved these changes Apr 22, 2018

View reviewed changes

pyokagan merged commit 8d2322b into se-edu:master Apr 24, 2018

yamidark deleted the fix-help-window-failing-test branch July 5, 2018 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HelpWindowTest: fix failing test in non-headless mode #880

HelpWindowTest: fix failing test in non-headless mode #880

yamidark commented Apr 16, 2018

CanIHasReview-bot commented Apr 16, 2018

yamidark commented Apr 16, 2018

vivekscl left a comment •

edited

Loading

vivekscl Apr 17, 2018

yamidark Apr 17, 2018

vivekscl Apr 17, 2018

yamidark Apr 17, 2018

yamidark commented Apr 17, 2018 •

edited

Loading

pyokagan Apr 17, 2018

pyokagan commented Apr 17, 2018

pyokagan commented Apr 17, 2018

pyokagan commented Apr 17, 2018

yamidark commented Apr 18, 2018 •

edited

Loading

CanIHasReview-bot commented Apr 19, 2018

pyokagan Apr 19, 2018

yamidark Apr 19, 2018

Zhiyuan-Amos Apr 19, 2018

yamidark Apr 20, 2018

Zhiyuan-Amos Apr 20, 2018

Zhiyuan-Amos Apr 20, 2018 •

edited

Loading

yamidark Apr 20, 2018 •

edited

Loading

Zhiyuan-Amos Apr 20, 2018

yamidark Apr 20, 2018

pyokagan Apr 20, 2018

CanIHasReview-bot commented Apr 20, 2018

pyokagan left a comment

pyokagan Apr 20, 2018

CanIHasReview-bot commented Apr 21, 2018

pyokagan left a comment

pyokagan left a comment

CanIHasReview-bot commented Apr 21, 2018

pyokagan commented Apr 22, 2018

CanIHasReview-bot commented Apr 22, 2018

vivekscl left a comment

HelpWindowTest: fix failing test in non-headless mode #880

HelpWindowTest: fix failing test in non-headless mode #880

Conversation

yamidark commented Apr 16, 2018

CanIHasReview-bot commented Apr 16, 2018

v1

yamidark commented Apr 16, 2018

vivekscl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamidark commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

pyokagan commented Apr 17, 2018

pyokagan commented Apr 17, 2018

pyokagan commented Apr 17, 2018

yamidark commented Apr 18, 2018 • edited Loading

CanIHasReview-bot commented Apr 19, 2018

v2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

yamidark Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 20, 2018

v3

pyokagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 21, 2018

v5

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 21, 2018

v6

pyokagan commented Apr 22, 2018

CanIHasReview-bot commented Apr 22, 2018

v7

vivekscl left a comment

Choose a reason for hiding this comment

vivekscl left a comment •

edited

Loading

yamidark commented Apr 17, 2018 •

edited

Loading

yamidark commented Apr 18, 2018 •

edited

Loading

Zhiyuan-Amos Apr 20, 2018 •

edited

Loading

yamidark Apr 20, 2018 •

edited

Loading