607000 - Test failures for private browsing (Timeout exceeded for waitForElement) (default)

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

MODULES: firefox/testPrivateBrowsing/testStartStopPBMode.js
TEST: testStopPrivateBrowsingMode
ERROR: {"exception": {"message": "Timeout exceeded for waitForElement Name: community"}}
BRANCH: default

http://hg.mozilla.org/qa/mozmill-tests/file/16ea43ce9241/firefox/testPrivateBrowsing/testCloseWindow.js#l90

I suspect this is happening because there is no need to wait for the element after the page has waited for the full page load.

Comment 1

[Aaron Train [:aaronmt]]

Mistake in comment above, testCloseWindow.js is the module and test is testCloseWindow

Comment 2

[Aaron Train [:aaronmt]]

Recent change to code from 

http://hg.mozilla.org/qa/mozmill-tests/diff/ce25e8c02110/firefox/testPrivateBrowsing/testCloseWindow.js

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Well, it doesn't matter if you are using waitForElement or a simple assertNode. The element itself cannot be fetched. Will you have a look at it Aaron?

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Well, I have no issue getting a reference to the element and asserting its existence by just re-writing the code to work on the activeTab. Do we even really need to check these elements?

Patch v1 - (default)

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 485838 [details] [diff] [review]
Patch v1 - (default)

Makes absolutely sense. The fade-in animation is causing those failures on slower machines. Can you also please make the same patch for testStartStopPBMode.js?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

We can't land it now but have to wait until the CommonJS patches have been checked-in.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) [more pb tests]

Two more tests with similar changes, and one with an assert check on the existence of an element to keep consistent with the others.

* /testPrivateBrowsing/testStartStopPBMode.js
* /testPrivateBrowsing/testTabsDismissedOnStop.js
* /testPrivateBrowsing/testTabRestoration.js

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Landed a combined patch on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/c3edd72de916

Please come up with a backport for the older branches. It doesn't apply on 1.9.2.

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (1.9.2/1.9.1)

Combined patch, applies to both older branches and includes the code missing from default in my other bug 608442

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 487107 [details] [diff] [review]
Patch v1 - (1.9.2/1.9.1)

Please make the changes I have proposed on the other bug (which wasn't really necessary to file as a new one).

Comment 11

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (1.9.2/1.9.1)

Adds the tabBrowser instances

Comment 12

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - (1.9.2/1.9.1)

Removes the controller parameter from closeAllTabs() calls when using tabBrowser instances

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 487254 [details] [diff] [review]
Patch v1.2 - (1.9.2/1.9.1)

>+++ b/firefox/testPrivateBrowsing/testTabRestoration.js
> var setupModule = function(module) {
>   controller = mozmill.getBrowserController();
>   pb = new privateBrowsing.privateBrowsing(controller);
>-
>+  
>   tabBrowser = new tabs.tabBrowser(controller);

nit: please correct all of those whitespace changes across this complete file.

Also when running the complete test folder with Namoroka, at least one single test failed. I have seen it for tab restoration and start/stop pb mode. Please check that.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: Patch v1.3 - (1.9.2/1.9.1)

Removed the new lines, and fixed the failures happening in testTabRestoration.js and testStartStopPBMode.js happening on slower machines (from the two failures I've seen). 

Noticed the failures happening on these lines:

http://hg.mozilla.org/qa/mozmill-tests/file/cda3cf03c015/firefox/testPrivateBrowsing/testStartStopPBMode.js#l132

and

http://hg.mozilla.org/qa/mozmill-tests/file/cda3cf03c015/firefox/testPrivateBrowsing/testTabRestoration.js#l94

Fixed for slower machines with a waitForElement. Ran with patch applied a couple dozen times and could not encounter the issue anymore.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 487336 [details] [diff] [review]
Patch v1.3 - (1.9.2/1.9.1)

>-    controller.waitForPageLoad(controller.tabs.getTab(i));
>+    var tab = controller.tabs.getTab(i);
>+    controller.waitForPageLoad(tab);
> 
>-    var elem = new elementslib.ID(controller.tabs.getTab(i), LOCAL_TEST_PAGES[i].id);
>+    var elem = new elementslib.ID(tab, LOCAL_TEST_PAGES[i].id);
>+    controller.waitForElement(elem);
>     controller.assertNode(elem);

We have a failure in the waitForPageLoad function which didn't wait for a page finished loading in the background. Can you please check if my patch on bug 610134 fixes this failure? We shouldn't have to use waitForElement here.

Comment 18

[Aaron Train [:aaronmt]]

With 1.5.1 released, it looks like waitForElement is not needed now.  I'll put up updated patches.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v1.4 - (1.9.2/1.9.1)

1.9.1/1.9.2
Same as the previous patch but as well removes the unnecessary waitForElement

One small patch for default branch coming up

Comment 20

[Aaron Train [:aaronmt]]

Attachment: Patch - (default) - waitForElement removal

Removes the waitForElement and places an assert to be consistent with the other tests and branches

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 489606 [details] [diff] [review]
Patch v1.4 - (1.9.2/1.9.1)

Looks fine. Please make sure you fix the unnecessary white-space changes introduced by this patch before the check-in.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 489607 [details] [diff] [review]
Patch - (default) - waitForElement removal

>+++ b/firefox/testPrivateBrowsing/testCloseWindow.js
>     var elem = new elementslib.Name(tab, LOCAL_TEST_PAGES[i].name);
> 
>     controller.waitForPageLoad(tab);
>-    controller.waitForElement(elem, TIMEOUT);
>+    controller.assertNode(elem);

As it looks like we still have code here which retrieves the element before the page has been loaded. Please come up with a follow-up patch for all those files we cover on this bug, so we really fix those stale elements.

Comment 23

[Aaron Train [:aaronmt]]

Landed on 1.9.1 - http://hg.mozilla.org/qa/mozmill-tests/rev/58021d7d2831
Landed on 1.9.2 - http://hg.mozilla.org/qa/mozmill-tests/rev/e0909d350009

Patch for stale elements on default coming up

Comment 24

[Aaron Train [:aaronmt]]

Attachment: Patch - (default) - stale element fix + waitForElementRemoval

Simply testCloseWindow.js had the declaration before waitForPageLoad

Comment 25

[Aaron Train [:aaronmt]]

Last default patch landed as http://hg.mozilla.org/qa/mozmill-tests/rev/dfe4fde2a3ff

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #25)
> Last default patch landed as
> http://hg.mozilla.org/qa/mozmill-tests/rev/dfe4fde2a3ff

And which still needs a backport.

Comment 27

[Aaron Train [:aaronmt]]

Attachment: Patch - (1.9.2/1.9.1) - stale element fix

Backport patch attached, land?

Comment 28

[Aaron Train [:aaronmt]]

Comment on attachment 490158 [details] [diff] [review]
Patch - (1.9.2/1.9.1) - stale element fix

># HG changeset patch
># User Aaron Train <[e-mail]>
># Date 1289588121 28800
># Branch mozilla1.9.2
># Node ID 9da389179c62a1abb019f855fccd995a03d021cd
># Parent  a423b626492dd8f6cc8b9be3bd021e525bb0c0ef
>Bug 607000 - Test failures for private browsing (Timeout exceeded for waitForElement). r=hskupin
>
>diff --git a/firefox/testPrivateBrowsing/testCloseWindow.js b/firefox/testPrivateBrowsing/testCloseWindow.js
>--- a/firefox/testPrivateBrowsing/testCloseWindow.js
>+++ b/firefox/testPrivateBrowsing/testCloseWindow.js
>@@ -117,9 +117,9 @@
>   // Check if all pages were re-loaded and show their content
>   for (var i = 0; i < LOCAL_TEST_PAGES.length; i++) {
>     var tab = controller.tabs.getTab(i);
>+    controller.waitForPageLoad(tab);
>+
>     var elem = new elementslib.Name(tab, LOCAL_TEST_PAGES[i].id);
>-
>-    controller.waitForPageLoad(tab);
>     controller.assertNode(elem);
>   }
> }

Comment 29

[Aaron Train [:aaronmt]]

With patch above landed on:

1.9.2 - http://hg.mozilla.org/qa/mozmill-tests/rev/9da389179c62
1.9.1 - http://hg.mozilla.org/qa/mozmill-tests/rev/00c5b0dd6c5a

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.