612296 - Test failures in restart modules for extensions tests on 1.9.2/1.9.1

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

Currently the following restart modules for extensions related tests are failing on the older branches:

I've listed a brief note on what I'm seeing when running the tests

* testExtensionInstallGetAddons
** Install dialog is not functioning properly. The dialog closes during install timer countdown.

* testExtensionInstallUninstall
* testMultipleExtensionInstallation 
* testThemeInstallUninstall
** Download button click is not triggering the install dialog

I have noticed that going to preview.addons.mozilla.org in these tests now redirects to addons.allizom.org, and we now see a notification bar indicating "Namoroka/Shiretoko (addons.allizom.org) prevented this site from allowing you to install software on your computer".

Comment 1

[Aaron Train [:aaronmt]]

That notification bar is not handled to click 'Allow', and that is more or less the culprit.

Comment 2

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

Is there an easy way to whitelist this domain? I assume there should be an available API call.

Comment 3

[Aaron Train [:aaronmt]]

A culmination of things are happening which all end up at the same result:

1. In order to trigger the install dialog, I flipped the pref 'xpinstall.whitelist.required' to false. This removes the notification bar that appears in the browser on addons.allizom.org that detects that a site want's to install add-ons. The tests that access AMO can now click Download fine.

2. A few tests were using Nightly Tester Tools which Heather recently updated, and it dropped support for 3.x. Changed the tests to use Compatibility Reporter instead.

3. With the changes in one and two, each test is now failing on the same issue: The installation dialog comes and goes, literally, it appears for a brief second and vanishes. No extensions are installed as the dialog isn't being handled properly.

Comment 4

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

(In reply to comment #3)
> 1. In order to trigger the install dialog, I flipped the pref
> 'xpinstall.whitelist.required' to false. This removes the notification bar that
> appears in the browser on addons.allizom.org that detects that a site want's to
> install add-ons. The tests that access AMO can now click Download fine.

We should try to add this site to the whitelist instead of disabling this check completely. 

> 3. With the changes in one and two, each test is now failing on the same issue:
> The installation dialog comes and goes, literally, it appears for a brief
> second and vanishes. No extensions are installed as the dialog isn't being
> handled properly.

Well, we should fail in the assert for the site then. Lets move the domain up as a persisted member which would make it more obvious.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (1.9.2)

Sounds good, from the comments above. If this is alright, I'll post a followup for 1.9.1

Comment 6

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

Comment on attachment 491286 [details] [diff] [review]
Patch v1 - (1.9.2)

>+  // Store the AMO preview site 
>+  persisted.amoPreviewSite = "addons.allizom.org";

Would be nice to get this value from addons.js. In the same step AMO_PREFERENCES also needs an update. It could also use the new global const i.e. AMO_SITE value.
 
>-  // Will the extension be installed from https://addons.mozilla.org/?
>+  // Will the extension be installed from https://addons.allizom.org

For comments ee shouldn't use hard-coded strings: "from the original domain.

>+const PREF_XPINSTALL_WHITELIST_ADD = 'xpinstall.whitelist.add';
[..]
>+  // Whitelist add the AMO preview site
>+  prefs.preferences.setPref(
>+    PREF_XPINSTALL_WHITELIST_ADD, 
>+    persisted.amoPreviewSite
>+  );

This code is used a couple of times in different tests. We have to move it into the addons.js file. Looks like a great addition for the shared module.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (1.9.2)

Suggestions addressed. Added changes to the shared module with the inclusion of:

- whitelistAmoPreviewSite() - Whitelist software installation from the preview site

- Constant AMO_PREVIEW_SITE of which the preferences makes use of, and is exported for the tests to use

Comment 8

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

Comment on attachment 491548 [details] [diff] [review]
Patch v2 - (1.9.2)

>+  persisted.url = "https://addons.allizom.org/en-US/firefox/addon/15003/";

The domain inside the url should be exchanged with the constant too. 

>+  {name: "Add-on Compatibility Reporter",
>+   id: "[e-mail]",
>+   url: "https://addons.allizom.org/en-US/firefox/addon/15003/"},
>   {name: "Mozilla QA Companion",
>    id: "{667e9f3d-0096-4d2b-b171-9a96afbabe20}",
>    url: "https://preview.addons.mozilla.org/firefox/addon/5428"}

Both urls will need an update as mentioned above.

> // Preferences which have to be changed to make sure we do not interact with the
>-// official AMO page but preview.addons.mozilla.org instead
>+// official AMO page but addons.allizom.org instead

I would remove the domain here too.

>+ * Whitelist software installation from the preview site
>+ */
>+function whitelistAmoPreviewSite() { 
>+  var prefSrv = prefs.preferences;
>+  prefSrv.setPref(PREF_XPINSTALL_WHITELIST_ADD, AMO_PREVIEW_SITE);

Well, we will have to allow more than only one single domain to get added. Not sure how this pref works but as it looks like it holds all domains in a comma separated list? If thats the case we need addToWhiteList and removeFromWhiteList functions, which have the domain as parameter.

For the normal tests this pref should also be reset in teardownModule. 

> // Export of functions
> exports.useAmoPreviewUrls = useAmoPreviewUrls;
> exports.resetAmoPreviewUrls = resetAmoPreviewUrls;
>+exports.whitelistAmoPreviewSite = whitelistAmoPreviewSite;
> 
> // Export of classes
> exports.addonsManager = addonsManager;
>+
>+// Export of variables
>+exports.AMO_PREVIEW_SITE = AMO_PREVIEW_SITE;

Variables should be listed before the functions to be consistent across the shared modules.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 10

[Aaron Train [:aaronmt]]

Dave, when exceptions are added to the whitelist in preferences, where are they written to? I was expecting to see them added to the value xpinstall.whitelist.add

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Dave, when exceptions are added to the whitelist in preferences, where are they
> written to? I was expecting to see them added to the value
> xpinstall.whitelist.add

The whitelist is held in the permissions manager, not preferences, see nsIPermissionManager. The permission type is "install"

Comment 12

[Aaron Train [:aaronmt]]

Thanks! Following an example from this test

http://mxr.mozilla.org/mozilla1.9.2/source/xpinstall/tests/browser_softwareupdate.js#14

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.2)

Removes use of the preferences, and uses the permission manager in two new addons API functions: 

* addToWhiteList(aDomain);
* removeFromWhiteList(aHost);

Also added a domain constant that is used in AMO_PREFERENCES and exported. It can be used as the host to in removeFromWhiteList, as the PM doesn't take a URI

Comment 14

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

Comment on attachment 492459 [details] [diff] [review]
Patch v3 - (1.9.2)

>+++ b/firefox/restartTests/testExtensionInstallUninstall/test1.js
>+  persisted.url = addons.AMO_PREVIEW_SITE + "/en-US/firefox/addon/15003/";

Something I haven't seen the last time, we should remove the language part of the URL. That will give us the chance to also test localized pages on AMO. I feel it's somekind important for our crowd testing.

>+  {name: "Add-on Compatibility Reporter",
>+   id: "[e-mail]",
>+   url: addons.AMO_PREVIEW_SITE + "/en-US/firefox/addon/15003/"},
>   {name: "Mozilla QA Companion",
>    id: "{667e9f3d-0096-4d2b-b171-9a96afbabe20}",
>-   url: "https://preview.addons.mozilla.org/firefox/addon/5428"}
>+   url: addons.AMO_PREVIEW_SITE + "/en-US/firefox/addon/5428/"}

Same here.

>+++ b/firefox/restartTests/testThemeInstallUninstall/test1.js
[..]
>+  persisted.themeURL = addons.AMO_PREVIEW_SITE + "/en-US/firefox/addon/122/";

and here.

>+// AMO Preview site
>+const AMO_PREVIEW_SITE = "https://addons.allizom.org";
>+
>+// AMO Preview domain
>+const AMO_PREVIEW_DOMAIN = "addons.allizom.org";

One comment should be enough here. Please stick the DOMAIN constant in-front of the SITE one and don't add a blank line. Then you can construct the SITE constant based on the DOMAIN.

>+function addToWhiteList(aDomain) { 
>+  var pm = Cc["@mozilla.org/permissionmanager;1"].
>+           getService(Ci.nsIPermissionManager);

Move this up to the global scope. It's a service we request here, which can also be used by other functions in this shared module then, without the need to get another reference.

>+  pm.add(utils.createURI(aDomain), "install", pm.ALLOW_ACTION);

Please use Ci.nsIPermissionManager.ALLOW_ACTION here.

Otherwise good work!

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v4 - (1.9.2)

Changes as requested in comment #14

Comment 16

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

Comment on attachment 492648 [details] [diff] [review]
Patch v4 - (1.9.2)

>+  pm.add(
>+       utils.createURI(aDomain),
>+       "install",
>+       Ci.nsIPermissionManager.ALLOW_ACTION

Please check the format we agreed on. No wrap to a new line for the first parameter.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v4.1 - (1.9.2)

Change from comment #16

Comment 18

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

Comment on attachment 492654 [details] [diff] [review]
Patch v4.1 - (1.9.2)

>+function addToWhiteList(aDomain) { 
>+  pm.add(utils.createURI(aDomain),
>+       "install",
>+       Ci.nsIPermissionManager.ALLOW_ACTION
>+     );

It's still the wrong indentation for the additional parameters. Also the closing bracket has to be at the same line as the last parameter.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v4.2 - (1.9.2)

Comment 20

[Aaron Train [:aaronmt]]

1.9.2 landed as http://hg.mozilla.org/qa/mozmill-tests/rev/1c059a928cc2

1.9.1 patch will be made shortly

Comment 21

[Aaron Train [:aaronmt]]

Attachment: Patch v4.2 - (1.9.1)

1.9.1 version based off 1.9.2 - had to make adjustments due to line differences

Comment 22

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

Comment on attachment 493023 [details] [diff] [review]
Patch v4.2 - (1.9.1)

r=me with the premise that it has been tested. I can't see anything which should break 1.9.1.

Comment 23

[Aaron Train [:aaronmt]]

1.9.1 http://hg.mozilla.org/qa/mozmill-tests/rev/46e6290629fb

Comment 24

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

This bug is not fixed. We still want to have those API changes on default.

Comment 25

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (Add-ons API) - (default)

API changes to addons.js on default branch

Comment 26

[Aaron Train [:aaronmt]]

Default - http://hg.mozilla.org/qa/mozmill-tests/rev/3e2733d2a0c8