1108999 - Refactor testAddons_installFromFTP from multiple files into a single file

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniela Domnici]

Combined two tests from testAddons_installFromFTP into one test.

Comment 1

[Daniela Domnici]

Attachment: patch_V1

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

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

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

You should also remove [include:testAddons_installFromFTP/manifest.ini] from firefox/tests/remote/restartTests/manifest.ini.

::: firefox/tests/remote/testAddons/testInstallAddonsFromFTP.js
@@ +52,5 @@
> +  delete persisted.nextTest;
> +
> +  addons.resetDiscoveryPaneURL();
> +  aModule.controller.stopApplication(true);
> + }

Nit: there is an extra blank space before }

@@ +72,5 @@
> +/**
> + * Verifies the extension is installed
> + */
> +function testAddonInstalled() {
> +  // Verify the extension is installed

This message is redundant here

@@ +83,5 @@
> +                                       value: ADDON[0].id})[0];
> +
> +  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> +            "Extension '" + ADDON[0].id +
> +            "' has been correctly installed");

The entire message can stay on the same line

Comment 3

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #2)

And you should also update the commit message.

Comment 4

[Daniela Domnici]

Attachment: patch_V2

Updated patch

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

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

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

You missed to update the commit message.

::: firefox/tests/remote/testAddons/testInstallAddonsFromFTP.js
@@ +18,5 @@
>  
>  const ADDON = [
>    {id: "[e-mail]",
>     url: "ftp://ftp.mozqa.com/data/firefox/addons/extensions/empty.xpi"}
>  ];

You don't need to have an array here since it's only one addon. Also update everywhere else where you use ADDON[0]

@@ +78,5 @@
> +    category: addonsManager.getCategoryById({id: "extension"})
> +  });
> +
> +  var addon = addonsManager.getAddons({attribute: "value",
> +                                       value: ADDON[0].id})[0];

Single line here, please

Comment 6

[Daniela Domnici]

Attachment: patch_V3

Changes requested were added.

We still have the problem with test run when the first test is skipped in the manifest(I`ve tried to do a remote test run with this patch applied.The first test in manifest/restartTests is skipped and it skips all the other tests.)

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

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

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

The tests looks good to me now.

Comment 8

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

This is not blocked by Mozmill 2.1. Feel free to add a dummy restart test instead as first test to run under restartTests.

Comment 9

[Daniela Domnici]

Attachment: patch_V4

Updated patch with the testDummy.js setted as the first test in manifest. Now we can do testruns. Here are the remote testrun results using Ubuntu 14.04 local machine : http://mozmill-crowd.blargon7.com/#/remote/report/5bb4668cfa6af53a0ba505835d364e9b

Comment 10

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/tests/remote/testAddons/manifest.ini
@@ +1,5 @@
>  [parent:../manifest.ini]
>  
>  [testInstallAddonWithoutEula.js]
>  disabled = Bug 1103869 - Test failure 'Notification popup state has been opened'
> +[testInstallAddonsFromFTP.js]

We should have the name say addon, not plural, we only install one.

::: firefox/tests/remote/testAddons/testInstallAddonsFromFTP.js
@@ +37,5 @@
> +
> +function teardownTest(aModule) {
> +  if (addonsManager.isOpen) {
> +    addonsManager.close();
> +  }

I think this has to be in teadownModule, I don't see that we open addons manager in first test and previously this check was in teardown in test2.js: 
aModule.addonsManager.close();

Comment 11

[Daniela Domnici]

Attachment: patch_V5

Changes requested were added.Thanks, Andreea!

Comment 12

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/tests/remote/testAddons/manifest.ini
@@ +1,5 @@
>  [parent:../manifest.ini]
>  
>  [testInstallAddonWithoutEula.js]
>  disabled = Bug 1103869 - Test failure 'Notification popup state has been opened'
> +[testInstallAddonFromFTP.js]

This will be the first one now.

Comment 13

[Daniela Domnici]

Attachment: patch_V6

Updated patch.

Comment 14

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/daaa14ceaf07 (default)

With today's merge we'll have this in aurora too. Thanks Daniela!