Timeout failure (waitForPageLoad) in testSafeBrowsingNotificationBar.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: aaronmt, Assigned: aaronmt)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
MODULE: testSafeBrowsingNotificationBar.js
TEST: testNotificationBar
ERROR: controller.waitForPageLoad(): Timeout waiting for page loaded
PLATFORM: All
BRANCH: All

Fixing this unmasked two problems in the test: a) goBack is untrustworthy, and b) another waitForPageLoad() issue resolved by acting on the activeTab rather than getTab.
(Assignee)

Comment 1

7 years ago
Created attachment 498472 [details] [diff] [review]
Patch v1 - (default)

Removed goBack() for open(), and used activeTab instead of getTab
Attachment #498472 - Flags: review?(hskupin)
Comment on attachment 498472 [details] [diff] [review]
Patch v1 - (default)

>+++ b/firefox/testSecurity/testSafeBrowsingNotificationBar.js
>     // Go back to the notification bar
>-    controller.goBack();
>+    controller.open(badSites[i]);
>     controller.waitForPageLoad();

goBack() uses the bfcache which doesn't work with waitForPageLoad at the moment. See bug 605784. So removing it is the right approach here.

>-    controller.waitForPageLoad(controller.tabs.getTab(1));
>+    controller.waitForPageLoad(controller.tabs.activeTab);

Why does getTab(1) fail? Do we have more than 2 tabs? Can you please give a more detailed explanation?
Attachment #498472 - Flags: review?(hskupin) → feedback+
(Assignee)

Comment 3

7 years ago
Created attachment 498738 [details] [diff] [review]
Patch v1.1 - (default)

When I ran this it seemed to hang on getTab instead of activeTab, although, with some comparison testing now, I'm unable to see the behaviour again at home. I just did a couple runs cross platform and it's not needed. For refactoring, since the tab opened becomes the activeTab, I think it would make sense to change it in the future.

Patch for review just has the switch from from goBack() to open()
Attachment #498738 - Flags: review?(hskupin)
Attachment #498738 - Flags: review?(hskupin) → review+
(In reply to comment #3)
> home. I just did a couple runs cross platform and it's not needed. For
> refactoring, since the tab opened becomes the activeTab, I think it would make
> sense to change it in the future.

Agreed on. In the same step we should check if we can apply that change also to other existent tests. Just for consistency.
(Assignee)

Comment 5

7 years ago
default - http://hg.mozilla.org/qa/mozmill-tests/rev/ef576d0eb075
1.9.2 - http://hg.mozilla.org/qa/mozmill-tests/rev/93eb9734927f
1.9.1 - http://hg.mozilla.org/qa/mozmill-tests/rev/d18792253ec5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.