620051 - Timeout failure (waitForPageLoad) in testSafeBrowsingNotificationBar.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

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.

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Removed goBack() for open(), and used activeTab instead of getTab

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 3

[Aaron Train [:aaronmt]]

Attachment: 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()

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 5

[Aaron Train [:aaronmt]]

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