595226 - Test failure in testSetMasterPassword (test1.js) [default]

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

MODULE: firefox/restartTests/testMasterPassword/test1.js
TEST: testSetMasterPassword
FAIL:  Expression "{"value":"password-save"}" returned null. Anonymous == false
BRANCH: default

http://hg.mozilla.org/qa/mozmill-tests/file/2140b48d7c42/firefox/restartTests/testMasterPassword/test1.js#l83

This is a result of the removal of the traditional browser notification in replace of the doorh-hanger style from bug 567814

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Our other tests aren't affected and getTabPanelElement still works? So we simply have to check for another element in the new notification. Geo, can you take it?

Comment 2

[Aaron Train [:aaronmt]]

Looks like this also affects testSavePasswordAndDeleted.js, which has a getProperty change, when fixed also has the issue here. Does getTabPanelElement work with door hanger notifications? If not, looks like we have to get the button from a lookup

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I'm not sure. But I would tend to say, no. Are the door hanger notifications bound to the current tab in the DOM? Means are those elements childs of the tab itself or is there a reference to the real node from the tab element?

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) [door hanger API fix]

Recent test failure result shows it failing http://mozmill-release.brasstacks.mozilla.com/#/general/report/4a2217a8a22f02aec478e6684a158853

Fixed in this patch with the door hanger API

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 486352 [details] [diff] [review]
Patch v1 - (default) [door hanger API fix]

>+  var notification = locationBar.getNotificationElement(
>+                       "password-save-notification"
>+                     );
[..]
>+  // After clicking the 'Remember Password' button, check notification state
>+  controller.waitFor(function() { 
>+    return notification.getNode().parentNode.state == 'closed';
>+  }, "Password notification should be closed", TIMEOUT);

In certain situations this can fail. Not in this particular test but if the notification gets replaced you will operate on a non-existent node. Instead of using the password notification and walking up the DOM, please add a new function to the ToolbarsAPI called getNotification which returns the notification popup element (#notification-popup).

Comment 6

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (default) [door hanger API fix + testToolbarAPI addition]

+ Added a simple API function

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (default) [door hanger API fix + testToolbarAPI addition]

Comment adjustment to not reflect door hanger specific

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 486651 [details] [diff] [review]
Patch v1.1 - (default) [door hanger API fix + testToolbarAPI addition]

>+var toolbars = require("../../../shared-modules/testToolbarAPI");

A bit bit-rotted. This should use the new toolbars module now.

>+  controller.waitFor(function() { 
>+    return notification.getNode().state == 'closed';
>+  }, "Password notification should be closed", TIMEOUT);

And here we don't need the timeout at the end. We default to 5s now.

I will make those changes before the check-in. Thanks Aaron!

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8c3ca4e826ae

Btw. there is no need to explicitly call out the branch name in the patch commit message. You can simply drop that.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.