512363 - Mozmill test for verifying Set, Invoke, and Delete a Master Password

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aakash Desai [:aakashd]]

Attachment: testPasswordManager/testMasterPassword.js

The testscript uses a sample login page from mozilla, submits login information
via the sample form and clicks the "remember" button on the resulting
notification bar. It then creates a master password in the preferences dialog, invokes the master password by accessing the password manager and toggles the show password button in the password manager. The saved password is deleted in the teardown module.

****** Warning: new notification bar functionality is not implemented yet. Once it is, the testscript will be sent to review******


Litmus Testcases Represented:
 * Testcase ID #6279 -  Set Master Password
 * Testcase ID #6066 -  Invoke Master Password

Comment 1

[Aakash Desai [:aakashd]]

Attachment: patch_file with new notification bar implementation

I apologize for the many patches today, Henrik. I've found the notification bar
implementation and made the changes needed. Here's the patch

Comment 2

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

Comment on attachment 396352 [details] [diff] [review]
patch_file with new notification bar implementation

I mostly only run the test. So the feedback I will give for now are the most visible things which should have to be cleared up.

>+ * Testcase ID #6279 -  Set Master Password

You mean 6276? Please update the spreadsheet and add the test ids for 3.6 there too.

>+  module.panel = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})');

That's not needed. Please remove it and the commented out version in testMasterPassword too.


>+var teardownModule = function(module) {
>+  // Remove all logins set by the testscript
>+  var pm = Components.classes["@mozilla.org/login-manager;1"]
>+                         .getService(Components.interfaces.nsILoginManager);
>+
>+  pm.removeAllLogins();
>+}

What about the master password? You have to reset it too. Looks like we have to use the UI to do this. Is there another Litmus test which takes care about this particular steps?

>+var testMasterPassword = function() {

Please always add a comment before the test function so we know what it does. Simply using the Litmus test description would be fine. Also please name it testSetMasterPassword.

>+  // Leave only one tab open in the browser
>+  UtilsAPI.closeAllTabs(controller);

You don't rely on only one open tab. Just kill it. :)

>+  controller.waitThenClick(new elementslib.ID(controller.tabs.activeTab, "LogIn"), gTimeout);
>+  controller.sleep(500);

Please don't use sleep here. Replace it by waitForPageLoad. Can you also please split the first in into the element creation and usage? Just for shortening the length.

>+var prefDialogSetMasterPasswordCallback = function(controller) {
>+  controller.waitThenClick(new elementslib.Lookup(controller.window.document, '/id("BrowserPreferences")/anon({"orient":"vertical"})/anon({"anonid":"selector"})/{"pane":"paneSecurity"}'), gTimeout);

Please separate those things into two lines when it is possible. We should try to not have lines longer then 80 chars when possible.

>+  controller.sleep(500);

Not necessary. You are using waitThenClick afterward.

>+  controller.waitThenClick(new elementslib.ID(controller.window.document, "useMasterPassword"), gTimeout);

Please two lines too (also applies to all following code).

>+/**
>+ * masterPasswordHandler - Sets the master password via the master password dialog
>+ */

Can you please also add an @param line?

>+  // Fill in the master password into both input fields and click ok
>+  controller.waitForElement(pw1, gTimeout);
>+  controller.type(pw1, "test1");
>+  controller.sleep(500);
>+  controller.type(pw2, "test1");	

Can we use a password with some special chars? Even outside of the normal ASCII charset. Would be great to fetch regressions. Sadly we cannot declare it as a global variable because the callbacks cannot access it. We would need a way to pass custom values to callback handlers.

>+  controller.waitThenClick(new elementslib.Lookup(controller.window.document, '/id("changemp")/anon({"anonid":"buttons"})/{"label":"OK"}'), gTimeout);

Again, please don't use a label.

>+var confirmHandler = function(controller) {
>+  controller.waitThenClick(new elementslib.Lookup(controller.window.document, '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept","xbl:inherits":"disabled=buttondisabledaccept","label":"OK"}'), gTimeout);

Please two lines and remove the label and the xbl:inherits attributes.

>+var prefDialogInvokeMasterPasswordCallback = function(controller) {
>+  controller.waitThenClick(new elementslib.Lookup(controller.window.document, '/id("BrowserPreferences")/anon({"orient":"vertical"})/anon({"anonid":"selector"})/{"pane":"paneSecurity"}'), gTimeout);

Two lines.

>+  // Call showPasswords dialog and view the passwords on your profile
>+  var md = new ModalDialogAPI.modalDialog(checkMasterHandler);
>+  md.start();  	
>+
>+  controller.waitThenClick(new elementslib.ID(controller.window.document, "showPasswords"), gTimeout);

Why you need this modal dialog instance? There is no modal dialog when clicking on the showPasswords button.

>+  UtilsAPI.assertElementVisible(pwdController, passwordCol, false);	
>+  
>+  pwdController.click(togglePasswords);
>+  UtilsAPI.assertElementVisible(pwdController, passwordCol, true);

Do we have to use the visible status here? Means it is always in the tree?

>+var checkMasterHandler = function(controller) {	
>+  controller.type(new elementslib.ID(controller.window.document, "password1Textbox"), "test1");

Two lines and wait for the element first.

>+  controller.click(new elementslib.Lookup(controller.window.document, '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept","xbl:inherits":"disabled=buttondisabledaccept","label":"OK"}'));	

No label please.

Further the show password window and the preferences dialog are not closed.

Comment 3

[Aakash Desai [:aakashd]]

Attachment: patch with changes from comment #2

The testscript has panel removed from setupModule, deleteMasterPassword has been added, closeAllTabs wsa removed, double lines were added, sleep was replaced with waitForPageLoad, we have to deal with the modal dialog the way that it is used for now until we find another way to deal with (make a note that this test case needs to be run with a new profile), also took out labels.



Henrik, we need to stop adding code conventions on the fly like this. Let's push out testscripts with the way we have been doing it and do a mass change at the end of this quarter. This is a huge hassle doing it mid-way like this because we need to take care of backwards compatibility, while still trying to accomplish a goal. Please a schedule a meeting and don't allow this to block this test script.

Comment 4

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

Comment on attachment 396500 [details] [diff] [review]
patch with changes from comment #2

The main reason why I have to r- this test is that it doesn't work on Windows. Looks like there are different ids for some ui elements. Also please check on Linux.

>+/**
>+ * Testcase ID #6276 -  Set Master Password
>+ */
>+var testSetMasterPassword = function() {

The intention of my last comment to add a description to the function wasn't to move the Litmus id listing to each function body. You should only add another comment. See a test like http://hg.mozilla.org/qa/mozmill-tests/rev/edee6f1e92eb.

>+  // Close the password manager and preferences dialog
>+  window.close();

Please use pwdController.keypress instead.


Because this test has an high impact for other tests I would feel safer at the moment to put it under restartTests so we can run it independently from the other tests until it has been stabilized. Further we could do an additional master password check at a later time after a restart. So please create a new subfolder under restartTests and put the file in there. The file name should be test1.js.

Comment 5

[Aakash Desai [:aakashd]]

Attachment: patch with testscript in restartTests

param code conventions added, testscript moved to restartTests, window.close() changed to keypresses and comments changed.

Comment 6

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

Comment on attachment 396865 [details] [diff] [review]
patch with testscript in restartTests 

Does it work on Linux and Windows now?

Comment 7

[Aakash Desai [:aakashd]]

Attachment: have i told you how much i hate this testscript?

Ran it against windows and found an issue...changes were made in prefDialogInvokeMasterPasswordCallback to make sure it works on windows.

Comment 8

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

Comment on attachment 396892 [details] [diff] [review]
have i told you how much i hate this testscript?

Looks good so far. There are still problems on Windows and Linux with modal dialogs. But it is a result of timing and needs the fix from bug 513215.

Just some hints for upcoming scripts:

>+var setupModule = function(module) {

Please move the '{' bracket to its own line. That's something we haven't obeyed yet.

>+var teardownModule = function(module) {
>+  // Remove all logins set by the testscript
>+  var pm = Components.classes["@mozilla.org/login-manager;1"]
>+                         .getService(Components.interfaces.nsILoginManager);

As said please use Cc and Ci and do a left indentation.

>+/**
>+ * Test removing the master password
>+ */
>+var testRemoveMasterPassword = function() {

In a later stage we could move this function to test.js and do some more masterpassword checks after a restart of Firefox.

Comment 9

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

Comment on attachment 396892 [details] [diff] [review]
have i told you how much i hate this testscript?

This test has never been run on the new location. Aakash I strongly advice doing that in the future. The referenced path for shared modules is wrong. I will update everything and push it later.

Comment 10

[Aakash Desai [:aakashd]]

Attachment: patch that is semi-work in progress

So, this testscript has the updated path and some sleep(500) so that it passes on WinXP as well OSX. There's still an issue with this testscript only working on a profile when a master password has never been set.

If a master password has been set before or the password manager was opened again, then a checkMasterHandler dialog will not pop-up since it's already registered with the profile.

I talked to jnightingale and sstamm about this and hte only possible way around this is to *maybe* clear the master password pref, wait awhile so the password is logged out and then access the master password dialog.


...I'm not sure if this is really worthwhile unless an easier method is determined. What do you think Henrik?

Comment 11

[Aakash Desai [:aakashd]]

Attachment: patch with changes made to preference dialog handler

This passes on OSX, but not on a Win XP run via VM. Try it out for yourself, I'm seeing errors on time-sensitive handling of modal dialog instancing.

Comment 12

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

Comment on attachment 398515 [details] [diff] [review]
patch with changes made to preference dialog handler

The test always fails on Windows with the following errors:

ERROR - Test Failure: {'function': {'message': 'timeout exceeded for waitForElem
ent ID: pw1', 'lineNumber': 306, 'stack': 'Error("timeout exceeded for waitForEl
ement ID: pw1")

ERROR - Test Failure: {'function': {'message': 'timeout exceeded for waitForElem
ent ID: useMasterPassword', 'lineNumber': 306, 'stack': 'Error("timeout exceeded
 for waitForElement ID: useMasterPassword")

ERROR - Test Failure: {'function': {'message': 'timeout exceeded for waitForElem
ent ID: password1Textbox', 'lineNumber': 306, 'stack': 'Error("timeout exceeded
for waitForElement ID: password1Textbox")

ERROR - Test Failure: {'function': {'message': 'window is null', 'lineNumber': 1
83

ERROR - Test Failure: {'function': {'message': 'timeout exceeded for waitForElem
ent ID: password', 'lineNumber': 306, 'stack': 'Error("timeout exceeded for wait
ForElement ID: password")

Looks like we still have timing problems. Aakash, would you need help to figure those out?

Comment 13

[Aakash Desai [:aakashd]]

Attachment: patch_MasterPassword

I added some sleeps check this against win XP using mozmill 1.2. It works there and on mozmill-trunk for osx.

Comment 14

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

Comment on attachment 400789 [details] [diff] [review]
patch_MasterPassword

Sorry for denying the patch again but you should run it on Linux too. It fails on this platform. I assume that at least one of the element in the password dialog has a different Lookup string as on OS X and Windows.

Comment 15

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

Comment on attachment 400789 [details] [diff] [review]
patch_MasterPassword

Mmh the comments haven't been sent I made for this patch. So once again. :/

>+  var pm = Components.classes["@mozilla.org/login-manager;1"]
>+                         .getService(Components.interfaces.nsILoginManager);

Please shorten to Cc and Ci.

>+var prefDialogSetMasterPasswordCallback = function(controller) {
>[...]
>+  // Call setMasterPassword dialog and set a master password to your profile
>+  var md = new ModalDialogAPI.modalDialog(masterPasswordHandler);
>+  md.start();

Use a value of at least 200 as parameter for the md.start call. That will delay the observer and could probably fix the random timeout problem. Do the same for the other instances.

>+  // Fill in the master password into both input fields and click ok
>+  controller.waitForElement(pw1, gTimeout);
>+  controller.type(pw1, "test1");
>+  controller.sleep(500);
>+  controller.type(pw2, "test1");	

I don't think that this sleep is really needed.

Comment 16

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

Ok, the above problems are definitely because of the login button is outside of the viewable area in the content area. We need bug 516729 to get it working for all screen resolutions.

Comment 17

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

Comment on attachment 400789 [details] [diff] [review]
patch_MasterPassword

I'll make the mentioned changes and check it in. Thanks Aakash for this tendentious play.

Comment 18

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/fa70d484d9e2
http://hg.mozilla.org/qa/mozmill-tests/rev/8597e745af6b

Comment 19

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

No failures anymore with bug 516729 fixed.

Comment 20

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

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