Closed Bug 843553 Opened 8 years ago Closed 8 years ago

Test failure "Checkbox ID: acceptCookies could not be checked/unchecked" in /testPreferences/testDisableCookies.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox22 fixed, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed
firefox-esr17 --- fixed

People

(Reporter: daniela.p98911, Assigned: daniela.p98911)

References

()

Details

(Whiteboard: [mozmill-test-failure] s=130225 u=failure c=cookies p=1)

Attachments

(2 files)

Happened today on MAC 10.7.5 and FF Nightly:
http://mozmill-ci.blargon7.com/#/functional/report/f36358d058daf73ddbf781501671301d

Happens because of this line: http://hg.mozilla.org/qa/mozmill-tests/file/0dd2dc62cc0e/tests/functional/testPreferences/testDisableCookies.js#l67

It might be that the 0.5 seconds sleep is not enough for waiting before clicking on the checkbox
OS: Linux → Mac OS X
Hardware: x86 → x86_64
Whiteboard: [mozmill-test-failure]
Ouch, so there is still a silly sleep() call. Yes, we should get this eliminated.
Hardware: x86_64 → All
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=130225 u=failure c=cookies p=1
We might want to check if other cookie tests have the same sleep call included.
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Attached patch patch v1.0Splinter Review
I have removed the sleep calls from the testPreferences/ cookies tests and added an assert.waitFor. 

The issue reproduces constantly under heavy load on the local MAC machine I have (e.g. A failing run: http://mozmill-crowd.blargon7.com/#/functional/report/66aad0ebfa7a01580628b3af4c23eec5). The problem is not that we cannot focus on the elements after changing history mode as the comments in the tests suggest, but that we are not waiting for the history mode to be fully changed before continuing the test. 

The same issue applies for bug 799032, bug 833805 and bug 813988 - we are not waiting when changing history mode, and adding the wait for history mode fixes those issues, too.

Also, there were two other sleep calls in the following files:
http://hg.mozilla.org/qa/mozmill-tests/file/f1f141b9ff87/tests/functional/testPreferences/testDisableCookies.js#l85
http://hg.mozilla.org/qa/mozmill-tests/file/f1f141b9ff87/tests/functional/testPreferences/testEnableCookies.js#l99
http://hg.mozilla.org/qa/mozmill-tests/file/f1f141b9ff87/tests/functional/testPreferences/testRemoveCookie.js#l79

These sleep calls are not needed in case we are properly waiting for the history mode to be changed.

Passing run for this patch: http://mozmill-crowd.blargon7.com/#/functional/report/66aad0ebfa7a01580628b3af4c2b5f06
Attachment #719044 - Flags: review?(andreea.matei)
Since we replace the sleep calls here for all the cookies tests and this fixes bugs: 799032, 833805 and 813988, I am marking this bug as blocking the others
Blocks: 799032, 833805, 813988
Comment on attachment 719044 [details] [diff] [review]
patch v1.0

Review of attachment 719044 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testPreferences/testDisableCookies.js
@@ +59,5 @@
>    controller.waitForElement(historyMode, TIMEOUT);
>    controller.select(historyMode, null, null, "custom");
> +  assert.waitFor(function () {
> +    return historyMode.getNode().value === "custom";
> +  }, "History mode is set to custom");

That sounds like a bug in Mozmill. Don't we wait until the apropriate entry has been selected? We should file a Mozmill bug if that also happens in Mozmill 2.
Comment on attachment 719044 [details] [diff] [review]
patch v1.0

Review of attachment 719044 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, landed for default:
http://hg.mozilla.org/qa/mozmill-tests/rev/0cfefc928c33 (default)

This test was affected only on default, but giving the change on the others as well, I say we backport to all branches. 
Lets wait for some reports first.
Attachment #719044 - Flags: review?(andreea.matei) → review+
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Comment on attachment 719044 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 719044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/functional/testPreferences/testDisableCookies.js
> @@ +59,5 @@
> >    controller.waitForElement(historyMode, TIMEOUT);
> >    controller.select(historyMode, null, null, "custom");
> > +  assert.waitFor(function () {
> > +    return historyMode.getNode().value === "custom";
> > +  }, "History mode is set to custom");
> 
> That sounds like a bug in Mozmill. Don't we wait until the apropriate entry
> has been selected? We should file a Mozmill bug if that also happens in
> Mozmill 2.

Mozmill 1.5 - The controller.select does not have a waitFor call anywhere. It is just open, click and close. So we are not waiting for the appropiate entry to be selected.

And it looks the same in Mozmill 2.0. I have logged bug 846277 for this issue.

Do we want to backout the patch from Default branch or leave it until a fix is done in Mozmill?
We want to land this across all branches. We will not spin another Mozmill 1.5 release for this issue.
Attachment #719935 - Flags: review?(andreea.matei) → review+
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.