1109026 - Refactor testPreferences_masterPassword from multiple files into a single file

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniela Domnici]

Combined three tests from testPreferences_masterPassword into one test.

Comment 1

[Daniela Domnici]

Attachment: patch_V1

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8533674 [details] [diff] [review]
patch_V1

Review of attachment 8533674 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message needs to be updated.
Remove [include:testPreferences_masterPassword/manifest.ini] from firefox/tests/functional/restartTests/manifest.ini.
Please order the test functions from testPreferencesMasterPassword by their running order, and the handlers at the end.

::: firefox/tests/functional/testAddons/testPreferencesMasterPassword.js
@@ +18,5 @@
>  
>  const PREF_BROWSER_IN_CONTENT = "browser.preferences.inContent";
>  const PREF_BROWSER_INSTANT_APPLY = "browser.preferences.instantApply";
>  
>  var setupModule = function(aModule) {

Use the "function <function_name>() {" notation

@@ +62,1 @@
>    // Go to the sample login page and perform a test log-in with inputted fields

Nit: input

@@ +78,5 @@
>    }, {type: "notification"});
>  
>    // After logging in, remember the login information
>    var button = locationBar.getNotificationElement(
> +               "password-save-notification",

This may stay on previous line, with the following lines aligned under it.

@@ +213,5 @@
> +
> +  expect.ok(utils.isDisplayed(aController, passwordCol),
> +            "Password column is visible");
> +
> +

Extra blank line

@@ +299,5 @@
> +  var button = findElement.Lookup(aController.window.document,
> +                           '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');
> +  button.waitThenClick();
> +}
> +

You have an extra blank line at the end of the file

Comment 3

[Daniela Domnici]

Attachment: patch_V2

Changes requested were added.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8543948 [details] [diff] [review]
patch_V2

Review of attachment 8543948 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good to me now, except one nit. Please update before checkin. Thanks!

::: firefox/tests/functional/testAddons/testPreferencesMasterPassword.js
@@ +221,5 @@
> +
> +  expect.ok(utils.isDisplayed(aController, passwordCol),
> +            "Password column is visible");
> +
> +

Please remove this extra blank line

Comment 5

[Daniela Domnici]

Attachment: patch_V3

Changes requested were added.Thanks Mihaela for you reviews:)!

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8544541 [details] [diff] [review]
patch_V3

Review of attachment 8544541 [details] [diff] [review]:
-----------------------------------------------------------------

Please test this on all platforms with the new version, should be ready to land.

::: firefox/tests/functional/testAddons/testPreferencesMasterPassword.js
@@ +80,5 @@
>  
>    // After logging in, remember the login information
> +  var button = locationBar.getNotificationElement("password-save-notification",
> +                                                   {type: "anonid",
> +                                                    value: "button"});

Indentation for last 2 lines should be shifted to the left, to be aligned with " from the first line.

@@ +125,2 @@
>  
> +  var masterPasswordCheck = findElement.ID(aController.window.document, "useMasterPassword");

New line for the id

@@ +159,1 @@
>                             '/id("changemp")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

Wrong indentation. Should start at aController and you can use "+" to concatenate the string on different lines. Same applies to all instances in the file.

@@ +233,5 @@
> + * @param {MozMillController} aController
> + *        MozMillController of the window to operate on
> + */
> +function checkMasterHandler(aController) {
> +  var passwordBox =findElement.ID(aController.window.document, "password1Textbox");

whitespace after =

@@ +235,5 @@
> + */
> +function checkMasterHandler(aController) {
> +  var passwordBox =findElement.ID(aController.window.document, "password1Textbox");
> +
> +  passwordBox.waitForElement();

No need for the blank line above, it's a single block

@@ +239,5 @@
> +  passwordBox.waitForElement();
> +  passwordBox.sendKeys("test1");
> +
> +  var button = findElement.Lookup(aController.window.document,
> +                           '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

Wrong indentation. Should start at aController and you can use "+" to concatenate the string on different lines.

@@ +253,5 @@
> +  var prefDialog = new prefWindow.preferencesDialog(aController);
> +
> +  prefDialog.paneId = "paneSecurity";
> +
> +  var masterPasswordCheck = findElement.ID(aController.window.document, "useMasterPassword");

New line for the id

@@ +283,5 @@
> +  var md = new modalDialog.modalDialog(aController.window);
> +  md.start(confirmHandler);
> +
> +  var button = findElement.Lookup(aController.window.document,
> +                           '/id("removemp")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

Same here

@@ +295,5 @@
> + *        MozMillController of the window to operate on
> + */
> +function confirmHandler(aController) {
> +  var button = findElement.Lookup(aController.window.document,
> +                           '/id("commonDialog")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

And here.

Comment 7

[Daniela Domnici]

Attachment: patch_V4

Fixed all nits.
Tested it on all platforms with the latest version of nightly.Here are the results:
Ubuntu 14-04(local machine): http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe67b9c68 
OS X 10.10 (local machine): http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe6733259 
Windows 8.1 (local machine): http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe67f4f6d

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545224 [details] [diff] [review]
patch_V4

Review of attachment 8545224 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/functional/testAddons/testPreferencesMasterPassword.js
@@ +41,1 @@
>    controller.open("about:blank");

We have an issue here. So this was only in the teardown for test1.js, in setup we closed all tabs, do the test and want about:blank in teardown before test2.

But now, next test will do setupTest which closes all tabs, so previous open of about:blank had no point. Before, we didn't use closeAllTabs in all tests in setup.

What we can do is move this line at the end of the first test. Or even better, to have the closeAllTabs from setupTest at the beginning of the first test only, as it was previously. We don't need to close tabs at each test as after the first one we'll have about:blank only.

Comment 9

[Daniela Domnici]

Attachment: patch_V5

Updated patch.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545920 [details] [diff] [review]
patch_V5

Review of attachment 8545920 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now.

Comment 11

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/677f362183a4 (default)
Thanks!

Comment 12

[Daniela Domnici]

Attachment: patch_V1_Beta

Created patch for beta branch.
I did a testrun. Here are the results: http://mozmill-crowd.blargon7.com/#/functional/report/dd0cbbf05a3aab4a7676502a86ef81fb

Comment 13

[Daniela Domnici]

Comment on attachment 8553055 [details]
patch_V1_Beta

We don`t need this for the beta branch.