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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: aaronmt, Assigned: aaronmt)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

8 years ago
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".
(Assignee)

Comment 1

8 years ago
That notification bar is not handled to click 'Allow', and that is more or less the culprit.
Is there an easy way to whitelist this domain? I assume there should be an available API call.
(Assignee)

Comment 3

8 years ago
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.
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 491286 [details] [diff] [review]
Patch v1 - (1.9.2)

Sounds good, from the comments above. If this is alright, I'll post a followup for 1.9.1
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
Attachment #491286 - Flags: review?(hskupin)
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.
Attachment #491286 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

8 years ago
Created attachment 491548 [details] [diff] [review]
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
Attachment #491286 - Attachment is obsolete: true
Attachment #491548 - Flags: review?(hskupin)
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: "compatibility@addons.mozilla.org",
>+   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.
Attachment #491548 - Flags: review?(hskupin) → review-
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
Version: 1.9.2 Branch → unspecified
(Assignee)

Comment 10

8 years ago
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
(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"
(Assignee)

Comment 13

8 years ago
Created attachment 492459 [details] [diff] [review]
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
Attachment #491548 - Attachment is obsolete: true
Attachment #492459 - Flags: review?(hskupin)
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: "compatibility@addons.mozilla.org",
>+   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!
Attachment #492459 - Flags: review?(hskupin) → review-
(Assignee)

Comment 15

8 years ago
Created attachment 492648 [details] [diff] [review]
Patch v4 - (1.9.2)

Changes as requested in comment #14
Attachment #492459 - Attachment is obsolete: true
Attachment #492648 - Flags: review?(hskupin)
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.
Attachment #492648 - Flags: review?(hskupin) → review-
(Assignee)

Comment 17

8 years ago
Created attachment 492654 [details] [diff] [review]
Patch v4.1 - (1.9.2)

Change from comment #16
Attachment #492648 - Attachment is obsolete: true
Attachment #492654 - Flags: review?(hskupin)
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.
Attachment #492654 - Flags: review?(hskupin) → review-
(Assignee)

Comment 19

8 years ago
Created attachment 493003 [details] [diff] [review]
Patch v4.2 - (1.9.2)
Attachment #492654 - Attachment is obsolete: true
Attachment #493003 - Flags: review?(hskupin)
Attachment #493003 - Flags: review?(hskupin) → review+
(Assignee)

Comment 20

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

1.9.1 patch will be made shortly
(Assignee)

Comment 21

8 years ago
Created attachment 493023 [details] [diff] [review]
Patch v4.2 - (1.9.1)

1.9.1 version based off 1.9.2 - had to make adjustments due to line differences
Attachment #493023 - Flags: review?(hskupin)
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.
Attachment #493023 - Flags: review?(hskupin) → review+
(Assignee)

Comment 23

8 years ago
1.9.1 http://hg.mozilla.org/qa/mozmill-tests/rev/46e6290629fb
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
This bug is not fixed. We still want to have those API changes on default.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

8 years ago
Created attachment 493038 [details] [diff] [review]
Patch v1 - (Add-ons API) - (default)

API changes to addons.js on default branch
Attachment #493038 - Flags: review?(hskupin)
Attachment #493038 - Flags: review?(hskupin) → review+
(Assignee)

Comment 26

8 years ago
Default - http://hg.mozilla.org/qa/mozmill-tests/rev/3e2733d2a0c8
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.