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

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: aaronmt, Assigned: aaronmt)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Mistake in comment above, testCloseWindow.js is the module and test is testCloseWindow
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?
(Assignee)

Comment 4

8 years ago
Created attachment 485838 [details] [diff] [review]
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)
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
Attachment #485838 - Flags: review?(hskupin)
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?
Attachment #485838 - Flags: review?(hskupin) → review+
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][needs landing after CommonJS)
We can't land it now but have to wait until the CommonJS patches have been checked-in.
Whiteboard: [mozmill-test-failure][needs landing after CommonJS) → [mozmill-test-failure][needs landing after CommonJS]
(Assignee)

Comment 7

8 years ago
Created attachment 486058 [details] [diff] [review]
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
Attachment #486058 - Flags: review?
Attachment #486058 - Flags: review? → review+
Summary: Test failure in testCloseWindow.js (default) → Test failures for private browsing (Timeout exceeded for waitForElement) (default)
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.
Whiteboard: [mozmill-test-failure][needs landing after CommonJS] → [mozmill-test-failure]
(Assignee)

Comment 9

8 years ago
Created attachment 487107 [details] [diff] [review]
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
Attachment #487107 - Flags: review?(hskupin)
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).
Attachment #487107 - Flags: review?(hskupin)
(Assignee)

Comment 11

8 years ago
Created attachment 487251 [details] [diff] [review]
Patch v1.1 - (1.9.2/1.9.1)

Adds the tabBrowser instances
Attachment #487107 - Attachment is obsolete: true
Attachment #487251 - Flags: review?(hskupin)
(Assignee)

Comment 12

8 years ago
Created attachment 487254 [details] [diff] [review]
Patch v1.2 - (1.9.2/1.9.1)

Removes the controller parameter from closeAllTabs() calls when using tabBrowser instances
Attachment #487251 - Attachment is obsolete: true
Attachment #487254 - Flags: review?(hskupin)
Attachment #487251 - Flags: review?(hskupin)
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.
Attachment #487254 - Flags: review?(hskupin) → review-
(Assignee)

Comment 14

8 years ago
Created attachment 487336 [details] [diff] [review]
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.
Attachment #487254 - Attachment is obsolete: true
Attachment #487336 - Flags: review?(hskupin)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 606971
Duplicate of this bug: 609064
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.
Attachment #487336 - Flags: review?(hskupin)
(Assignee)

Comment 18

8 years ago
With 1.5.1 released, it looks like waitForElement is not needed now.  I'll put up updated patches.
(Assignee)

Comment 19

8 years ago
Created attachment 489606 [details] [diff] [review]
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
Attachment #487336 - Attachment is obsolete: true
Attachment #489606 - Flags: review?(hskupin)
(Assignee)

Comment 20

8 years ago
Created attachment 489607 [details] [diff] [review]
Patch - (default) - waitForElement removal

Removes the waitForElement and places an assert to be consistent with the other tests and branches
Attachment #489607 - Flags: review?(hskupin)
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.
Attachment #489606 - Flags: review?(hskupin) → review+
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.
Attachment #489607 - Flags: review?(hskupin) → review+
(Assignee)

Comment 23

8 years ago
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
(Assignee)

Comment 24

8 years ago
Created attachment 489853 [details] [diff] [review]
Patch - (default) - stale element fix + waitForElementRemoval

Simply testCloseWindow.js had the declaration before waitForPageLoad
Attachment #489607 - Attachment is obsolete: true
Attachment #489853 - Flags: review?(hskupin)
Attachment #489853 - Flags: review?(hskupin) → review+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

8 years ago
Created attachment 490158 [details] [diff] [review]
Patch - (1.9.2/1.9.1) - stale element fix

Backport patch attached, land?
(Assignee)

Comment 28

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

># HG changeset patch
># User Aaron Train <atrain@mozilla.com>
># 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);
>   }
> }
Attachment #490158 - Attachment description: Patch - (1.9.2/1.9.1) - stale element fix + waitForElementRemoval → Patch - (1.9.2/1.9.1) - stale element fix
(Assignee)

Comment 29

8 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.