Don't compare unnecessary labels during browser-chrome tests.

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
We have a number of browser chrome tests that are looking at strings to make sure they are the correct ones. These seem fragile and can fail easily if we change branding, or even if run into race conditions where an element might not update its text quickly enough. We should try to minimize these test cases, and look for better ways to inspect these strings.
(Assignee)

Comment 1

7 years ago
Created attachment 490674 [details] [diff] [review]
Fixes v1

Fixes for tests. This mostly occurs in addon tests, in particular checking the text shown in the restart notification. I'd really like to test this in order to make sure the message is correct. And in fact, if you stop the tests and look, the message is correct. So there is some sort of race condition.

So here, instead of checking the exact message we just check for the word "update" in the message. I also refactored some of the test so that it was listening for AlertActive events. There are still failures though because the call to notificationbox.removeAllNotifications is not doing its job (probably because we are still inside the AlertActive event listener?). So I added a 1sec timeout wrapping the:

notificationbox.removeAllNotifications(true);
aCallbacK();

and finally things seem to pass. That's ugly though, so here I'm just commenting out the test. I left the timeout in though, as I would like to remove the notifications if at all possible, and it seems to help.
Attachment #490674 - Flags: review?(mark.finkle)
Comment on attachment 490674 [details] [diff] [review]
Fixes v1

>diff --git a/chrome/tests/browser_addons.js b/chrome/tests/browser_addons.js

>-function isRestartShown(aShown, isUpdate) {
>+function isRestartShown(aShown, isUpdate, aCallback) {
>   let msg = document.getElementById("addons-messages");
>   ok(!!msg, "Have message box");
>+
>+  let done = function(aNotification) {
>+    is(!!aNotification, aShown, "Restart exists = " + aShown);
>+    if (aShown && aNotification) {
>+      let showsUpdate = aNotification.label.match(/update/i) != null;
>+      // this test regularly fails due to race conditions here
>+      //is(showsUpdate, isUpdate, "Restart shows correct message");
>+    }
>+    setTimeout(function() {
>+      msg.removeAllNotifications(true);
>+      aCallback();
>+    }, 1000);
>+  }

Is the setTimeout(..., 1000) really needed? or can a setTimeout(..., 0) be used instead? Any real timeouts are frowned upon in tests.

>diff --git a/chrome/tests/browser_bookmarks.js b/chrome/tests/browser_bookmarks.js

>   run: function() {
>-    this._currentTab = Browser.addTab(testURL_01, true);
>-
>     // Need to wait until the page is loaded
>     messageManager.addMessageListener("pageshow",
>     function(aMessage) {
>       if (gCurrentTest._currentTab.browser.currentURI.spec != "about:blank") {
>         messageManager.removeMessageListener(aMessage.name, arguments.callee);
>         gCurrentTest.onPageReady();
>       }
>     });
>+    this._currentTab = Browser.addTab(testURL_01, true);

This just seems wrong. We want to catch the "pageshow" message for the newly opened tab. But you are hooking the event to the wrong tab.

Perhaps we need to use | gCurrentTest._currentTab | instead of | this._currentTab | although that shouldn't make much difference.

>-    this._currentTab = Browser.addTab(testURL_02, true);
>-
>     // Need to wait until the page is loaded
>     messageManager.addMessageListener("pageshow", function(aMessage) {
>       if (gCurrentTest._currentTab.browser.currentURI.spec != "about:blank") {
>         messageManager.removeMessageListener(aMessage.name, arguments.callee);
>         gCurrentTest.onPageReady();
>       }
>     });
>+    this._currentTab = Browser.addTab(testURL_02, true);

Same

>diff --git a/chrome/tests/browser_preferences_text.js b/chrome/tests/browser_preferences_text.js

Can you remove the data from the top of the file where the expected labels are setup too?
Attachment #490674 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 3

7 years ago
Created attachment 490738 [details] [diff] [review]
Fixes v2

Hmm... it seems unintuitive to me to assign the event handler after you start loading the page. Either way seems to work fine though, and that's not what this bug was supposed to be about.

As to the timeout thing, I tried with both earlier, and a timeout of zero did not get rid of the notification. However, I found a small bug in this where the addon being used in the upgrade tests thought it was bootstrapped. Fixing that, these pass without the timeout for me.
Attachment #490674 - Attachment is obsolete: true
Attachment #490738 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Assignee: nobody → wjohnston
Attachment #490738 - Flags: review?(mark.finkle) → review+
nice cleanup

pushed:
http://hg.mozilla.org/mobile-browser/rev/b6d3914bb0c4
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 5

7 years ago
Can you please provide some steps to reproduce?
You need to log in before you can comment on or make changes to this bug.