Closed Bug 607000 Opened 12 years ago Closed 12 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronmt, Assigned: aaronmt)

References

Details

(Keywords: regression, Whiteboard: [mozmill-test-failure])

Attachments

(5 files, 5 obsolete files)

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.
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?
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]
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]
Attached patch Patch v1 - (1.9.2/1.9.1) (obsolete) — Splinter Review
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)
Attached patch Patch v1.1 - (1.9.2/1.9.1) (obsolete) — Splinter Review
Adds the tabBrowser instances
Attachment #487107 - Attachment is obsolete: true
Attachment #487251 - Flags: review?(hskupin)
Attached patch Patch v1.2 - (1.9.2/1.9.1) (obsolete) — Splinter Review
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-
Attached patch Patch v1.3 - (1.9.2/1.9.1) (obsolete) — Splinter Review
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)
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)
With 1.5.1 released, it looks like waitForElement is not needed now.  I'll put up updated patches.
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)
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+
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
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+
Status: ASSIGNED → RESOLVED
Closed: 12 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 → ---
Backport patch attached, land?
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
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
Closed: 12 years ago12 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.
Product: Testing → Mozilla QA
Version: Trunk → unspecified
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.