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

Patch includes localization and some minor formatting changes.

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

> >- 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)?

Sounds like a plan.

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

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

(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 on attachment 478133 [details] [diff] [review] Patch v3.1 (default) Wrong patch, sorry...

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.

(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]

Backport to 1.9.1

(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

(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]

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.