585720 - Make Mozmill-test testMasterPassword local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: restartTests/testMasterPassword/test1.js
Test-page: firefox/password_manager/login_form.html

Comment 1

[u279076]

Attachment: Patch v1 (default)

Patch includes localization and some minor formatting changes.

Comment 2

[Aaron Train [:aaronmt]]

Comment on attachment 464618 [details] [diff] [review]
Patch v1 (default)

> // Include necessary modules
> var RELATIVE_ROOT = '../../../shared-modules';
>-var MODULE_REQUIRES = ['ModalDialogAPI','PrefsAPI', 'TabbedBrowsingAPI',
>-                       'UtilsAPI'];
>+var MODULE_REQUIRES = ['ModalDialogAPI', 'PrefsAPI', 'TabbedBrowsingAPI', 'UtilsAPI'];

const
 
>-  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), gTimeout);
>-  controller.sleep(gDelay);
>+  var loginButton = new elementslib.ID(controller.tabs.activeTab, "LogIn");
>+  controller.waitThenClick(loginButton, TIMEOUT);
>+  controller.sleep(DELAY);

I've been told to s/gDelay/100. It's what I've been using. 

r-, otherwise patch looks good with the fixes

Comment 3

[u279076]

Attachment: Patch v2 (default)

Comment 4

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

Comment on attachment 464894 [details] [diff] [review]
Patch v2 (default)

>-  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), gTimeout);
>-  controller.sleep(gDelay);
>+  var loginButton = new elementslib.ID(controller.tabs.activeTab, "LogIn");
>+  controller.waitThenClick(loginButton, TIMEOUT);
>+  controller.sleep(100);

Why do we need a sleep call here? Shouldn't it be a waitForPageLoad instead?

>-  controller.waitThenClick(button, gTimeout);
>-  controller.sleep(gDelay);
>+  controller.waitThenClick(button, TIMEOUT);
>+  controller.sleep(100);
>   controller.assertNodeNotExist(button);

This will certainly break. gDelay was 500ms before and the notification bar will not close within 100ms. We should get rid of the sleep call and check for the opening state of the notification bar instead.

>-  md.start(gDelay);
>+  md.start(100);

Please leave it as 500ms for all instances. Eventually we should keep it as constant but rename it like DELAY_MODAL_DIALOG.

Comment 5

[u279076]

> >-  controller.waitThenClick(button, gTimeout);
> >-  controller.sleep(gDelay);
> >+  controller.waitThenClick(button, TIMEOUT);
> >+  controller.sleep(100);
> >   controller.assertNodeNotExist(button);
> 
> This will certainly break. gDelay was 500ms before and the notification bar
> will not close within 100ms. We should get rid of the sleep call and check for
> the opening state of the notification bar instead.
> 

This is not checking for the opening of the notification bar.  This is clicking on the Save Password button then waiting 100ms then checking that the button no longer exists (ie. the notification bar has disappeared).  I could replace the sleep and assertNodeNotExist(button) with a waitForEval(button_not_visible)?

Comment 6

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

Sounds like a plan.

Comment 7

[u279076]

Attachment: Patch v3 (default)

Approval of this method for checking the state of the notification bar may be able to be duplicated to other notification bars (ie. Safe Browsing).  It may be worth investigating the creation of a couple of notification bar helpers to the tabbedBrowsingAPI as well.

Comment 8

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

Comment on attachment 465388 [details] [diff] [review]
Patch v3 (default)

>+const MODULE_REQUIRES = ['ModalDialogAPI','PrefsAPI', 'TabbedBrowsingAPI',
>                        'UtilsAPI'];

nit: please fix the indentation.

>-  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), gTimeout);
>-  controller.sleep(gDelay);
>+  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), TIMEOUT);
>+  controller.sleep(500);
> 
>   // After logging in, remember the login information
>   var label = UtilsAPI.getProperty("chrome://passwordmgr/locale/passwordmgr.properties",
>                                    "notifyBarRememberButtonText");
>   var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>                                              '/{"value":"password-save"}/{"label":"' + label + '"}');
> 
>   UtilsAPI.assertElementVisible(controller, button, true);

We should replace the sleep call here too with a waitForEval call right before assertElementVisible.

>+  controller.waitForEval("subject.isNotificationHidden == true", TIMEOUT, 100,
>+                         {isNotificationHidden: tabBrowser.getTabPanelElement(tabBrowser.selectedIndex).
>+                                                getNode().allNotifications.length == 0});

The comparison has to be part of the expression otherwise you will timeout. Also you can remove the parameter for getTabPanelElement because it automatically uses the current tab.

Comment 9

[Aaron Train [:aaronmt]]

The online version of this test presently fails due to bug 595226

Comment 10

[u279076]

(In reply to comment #8)
> Comment on attachment 465388 [details] [diff] [review]
> Patch v3 (default) 
> >-  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), gTimeout);
> >-  controller.sleep(gDelay);
> >+  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), TIMEOUT);
> >+  controller.sleep(500);
> > 
> >   // After logging in, remember the login information
> >   var label = UtilsAPI.getProperty("chrome://passwordmgr/locale/passwordmgr.properties",
> >                                    "notifyBarRememberButtonText");
> >   var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
> >                                              '/{"value":"password-save"}/{"label":"' + label + '"}');
> > 
> >   UtilsAPI.assertElementVisible(controller, button, true);
> 
> We should replace the sleep call here too with a waitForEval call right before
> assertElementVisible.
> 
> >+  controller.waitForEval("subject.isNotificationHidden == true", TIMEOUT, 100,
> >+                         {isNotificationHidden: tabBrowser.getTabPanelElement(tabBrowser.selectedIndex).
> >+                                                getNode().allNotifications.length == 0});
> 
> The comparison has to be part of the expression otherwise you will timeout.
> Also you can remove the parameter for getTabPanelElement because it
> automatically uses the current tab.

I'm reverting these changes as they are out of scope for local-data.  We can address them with the refactoring work.

  // Check that the Remember Password button is visible
  UtilsAPI.assertElementVisible(controller, button, true);
  
  // Click the Remember Password button
  controller.waitThenClick(button, TIMEOUT);
  controller.sleep(500);
  controller.assertNodeNotExist(button);

All our Password Manager tests use the above method, and have for some time.  I agree we should get away from sleep() but this should fall under the scope of refactoring work, not local-data.

Patch forthcoming addressing the nits and reverts.

Comment 11

[u279076]

Attachment: Patch v3.1 (default)

Comment 12

[u279076]

Comment on attachment 478133 [details] [diff] [review]
Patch v3.1 (default)

Wrong patch, sorry...

Comment 13

[u279076]

Attachment: Patch v3.1 (default)

Comment 14

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

Comment on attachment 478134 [details] [diff] [review]
Patch v3.1 (default)

The changes look ok but please file a bug to get the test fixed. The first time we invoke the master password we hang. Probably a timing issue.

Comment 15

[u279076]

(In reply to comment #13)
> Created attachment 478134 [details] [diff] [review]
> Patch v3.1 (default)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/de767486b2ae [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/a1c25fe43ce4 [mozilla1.9.2]

Comment 16

[u279076]

Attachment: Patch v3.1 (1.9.1)

Backport to 1.9.1

Comment 17

[u279076]

(In reply to comment #14)
> Comment on attachment 478134 [details] [diff] [review]
> Patch v3.1 (default)
> 
> The changes look ok but please file a bug to get the test fixed. The first time
> we invoke the master password we hang. Probably a timing issue.

Filed -> bug 599502

Comment 18

[u279076]

(In reply to comment #16)
> Created attachment 478405 [details] [diff] [review]
> Patch v3.1 (1.9.1)
> 
> Backport to 1.9.1

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e4c10f33dbaf [mozilla1.9.1]

Comment 19

[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.