1109040 - Refactor testAddons_installWithoutEULA from multiple files into a single file

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniela Domnici]

Combined two tests from testAddons_installWithoutEula into one test.

Comment 1

[Daniela Domnici]

Attachment: patch_V1

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

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

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

You should also remove these lines from firefox/tests/remote/restartTests/manifest.ini:
[include:testAddons_InstallAddonWithoutEULA/manifest.ini]
disabled = Bug 1103869 - Test failure 'Notification popup state has been opened'

::: firefox/tests/remote/testAddons/testInstallAddonWithoutEula.js
@@ +5,5 @@
>  "use strict";
>  
>  // Include required modules
> +var addons = require("../../../../lib/addons");
> +var browser = require("../../../lib/ui/browser");

Being an UI lib, browser should be at the end, with a blank line before it

@@ +67,1 @@
>  /**

Add a blank line before this comment

@@ +92,5 @@
>    }, {type: "notification"});
>  
>    md.waitForDialog(TIMEOUT_DOWNLOAD);
>  }
> +/**

Please add a blank line before this

Comment 3

[Daniela Domnici]

Attachment: patch_V2

Changes requested were added.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

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

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

Looks good now, thanks!

Comment 5

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

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

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

Mihaela, you are not a peer, so we still need the final review from at least Andreea.

Comment 6

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/tests/remote/testAddons/testInstallAddonWithoutEula.js
@@ +64,3 @@
>  
> +  addons.resetDiscoveryPaneURL();
> +  aModule.controller.stopApplication(true);

closeAllTabs is missing, which was in the previous test.

@@ +100,4 @@
>  }
> +
> +/**
> + * Test install addon without EULA and check if the addon is correctly instaled

nit: installed

@@ +101,5 @@
> +
> +/**
> + * Test install addon without EULA and check if the addon is correctly instaled
> + */
> +function testInstallAddonWithoutEULA() {

Hm. Actually, this method is not installing the addon but it checks if it remained installed after the restart. Lets rename this and the comment above.

Comment 7

[Daniela Domnici]

Attachment: patch_V3

Updated patch.Changes requested were added. Thanks, Andreea for your reviews :).

Comment 8

[Andreea Matei [:AndreeaMatei]]

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

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

http://hg.mozilla.org/qa/mozmill-tests/rev/a14207da4ba5 (defauly)
Thanks Daniela!

Comment 9

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

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

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

::: firefox/tests/remote/testAddons/testInstallAddonWithoutEula.js
@@ +15,4 @@
>  
>  const PREF_INSTALL_DIALOG = "security.dialog_enable_delay";
>  const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";

nit: wrong order of declarations.

@@ +44,5 @@
>  
> +function teardownTest(aModule) {
> +  if (addonsManager.isOpen) {
> +    addonsManager.close();
> +  }

This changes the behavior of the test! Formerly we were not closing the addon manager, so the open tab was re-used.

Comment 10

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> >  
> > +function teardownTest(aModule) {
> > +  if (addonsManager.isOpen) {
> > +    addonsManager.close();
> > +  }
> 
> This changes the behavior of the test! Formerly we were not closing the
> addon manager, so the open tab was re-used.

Previously, we had teardownModule in test1.js which was closing all tabs and in test2.js we started with addonManager.open(), that remained unchanged here. So it's the same behavior as before.

Comment 11

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

Strange. Looks like I looked that at the wrong thing. Thanks Andreaa.