Assert failure in testDisabledElements.js | assertJSProperty(ID: menu_import) : true == false

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: aaronmt, Assigned: aaronmt)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
MODULE: testDisabledElements.js
TEST: checkImportMenu
BRANCH: default
PLATFORM: Mac
ERROR: assertJSProperty(ID: menu_import) : true == false

http://hg.mozilla.org/qa/mozmill-tests/file/6a13621ad288/firefox/testPrivateBrowsing/testDisabledElements.js#l88

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=4.0&platform=All&from=2011-01-29&to=2011-02-03&test=firefox%2FtestPrivateBrowsing%2FtestDisabledElements.js&func=testCheckAboutPrivateBrowsing

What seems to be happening is that the assert is being called in a race with the change to set the disabled property on the element with the change taken place on the new window (library). For some reason it's slower to set the property on Mac for the Library window. The following patch waits for the change to happen.

See the change on entry to private browsing here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#8037
(Assignee)

Comment 1

7 years ago
Created attachment 510351 [details] [diff] [review]
Patch v1 - (default)
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
Attachment #510351 - Flags: review?(hskupin)
Comment on attachment 510351 [details] [diff] [review]
Patch v1 - (default)

>+  controller.waitFor(function () {
>+    return importItem.getNode().disabled === true;
>+  }, "Import item is disabled");

Just return the state of disabled. It's already a boolean. Also please enhance the message, so we know which import item is checked here.
Attachment #510351 - Flags: review?(hskupin) → review-
(Assignee)

Comment 3

7 years ago
Created attachment 510373 [details] [diff] [review]
Patch v2 - (default)
Attachment #510373 - Flags: review?(hskupin)
Attachment #510351 - Attachment is obsolete: true
Comment on attachment 510373 [details] [diff] [review]
Patch v2 - (default)

> function checkImportMenu(controller) {
>   // Check File -> Import entry again
>   var importItem = new elementslib.ID(controller.window.document, "menu_import");
>-  controller.assertJSProperty(importItem, "disabled", true);
>+  controller.waitFor(function () {
>+    return importItem.getNode().disabled
>+  }, "File -> Import menu entry item is disabled");

You see, even comments can be misleading. This is not the browser window but the library window. So please mention that in the assert message, i.e. "Import menu entry of the Library window". Also add the semicolon in the return row.
Attachment #510373 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 510601 [details] [diff] [review]
Patch v3 - (default)
Attachment #510373 - Attachment is obsolete: true
Attachment #510601 - Flags: review?(hskupin)
Comment on attachment 510601 [details] [diff] [review]
Patch v3 - (default)

>+    return importItem.getNode().disabled

There is still the missing semicolon.

>+  }, "Import menu entry item of the Library window is disabled");

Remove the 'entry' which is not necessary.

r=me with both changes. Please upload the patch before checking in.
Attachment #510601 - Flags: review?(hskupin) → review+
(Assignee)

Comment 7

7 years ago
Created attachment 510611 [details] [diff] [review]
Patch v3.1 w/corrections - (default) [checked-in]
Attachment #510611 - Flags: review+
(Assignee)

Comment 8

7 years ago
Comment on attachment 510611 [details] [diff] [review]
Patch v3.1 w/corrections - (default) [checked-in]

Landed (default) as:

http://hg.mozilla.org/qa/mozmill-tests/rev/945880f4a2a4
Attachment #510611 - Attachment description: Patch v3.1 w/corrections - (default) → Patch v3.1 w/corrections - (default) [checked-in]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Why hasn't this been landed on older branches?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

7 years ago
The change in the browser only affected trunk
(Assignee)

Comment 11

7 years ago
Oh I was thinking of the stack element bug. Yeah, we should land this on the other branches too incase of failure.
(Assignee)

Comment 12

7 years ago
Created attachment 511059 [details] [diff] [review]
Patch v3.1 - (backport 1.9.2/1.9.1)

Backport for 1.9.2 and 1.9.1
Attachment #511059 - Flags: review?(hskupin)
Attachment #511059 - Flags: review?(hskupin) → review+
(Assignee)

Comment 13

7 years ago
Backport landed as 
http://hg.mozilla.org/qa/mozmill-tests/rev/bed2e447341e - 1.9.2
http://hg.mozilla.org/qa/mozmill-tests/rev/a1771f0e0403 - 1.9.1
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.