664018 - Mozmill test for installing an add-on from "First Time with Add-ons?"

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Tracking bug to develop a Mozmill test for the "First Time with Add-ons?" section of the Promo module of the Discovery Pane of the Add-ons Manager.

Comment 1

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/14522901

Comment 2

[u279076]

Ccing WebQA on this bug.

This test depends on there being no add-ons installed, but Mozmill automatically installs a couple of per-requesite add-ons on startup. Krupa, Dave, Marlena, is there any way to trick AMO into thinking no add-ons are installed? How is it determined whether to display the "First Time" section?

Depending on WebQA's answer, I think we have 3 potential outcomes for this test:
1) Find a hack to trick AMO into displaying the First Time section (possible)
2) Find a way for Mozmill to start without pre-installed add-ons (unlikely)
3) Shelve this test as not-doable given current technology (most likely)

Comment 3

[Marlena Compton [:marlenac]]

I don't know of any tricks offhand.

Comment 4

[krupa raj[:krupa]]

I don't know of any ways to trick AMO into thinking that no add-ons are installed.

Comment 5

[u279076]

Alex, let's just shelve this test for now. We can revisit it once Henrik comes back. It's probably not doable given current technology.

Comment 6

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

Simply set the 'extensions.getAddons.cache.enabled' preference to false inside setupModule and we will no longer send add-on information to the Discovery Pane.

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Comment 8

[u279076]

Comment on attachment 540727 [details] [diff] [review]
patch v1.0

>+const EXT_TIMEOUT = 25000;
>+const prefName = "extensions.getAddons.cache.enabled";

Call this ADDONS_CACHE_ENABLED

Comment 9

[u279076]

Also, please move this test to mozmill-tests/tests/remote/restartTests/testDiscoveryPane/test5.js and add a check to the end of your test which verifies the add-on is listed in the list of installed add-ons.

Comment 10

[Alex Lakatos[:AlexLakatos]]

(In reply to comment #9)
> Also, please move this test to
> mozmill-tests/tests/remote/restartTests/testDiscoveryPane/test5.js and add a
These tests(Discovery Pane Tests) are not Restart tests. Should I still move them under the specified path?

> check to the end of your test which verifies the add-on is listed in the
> list of installed add-ons.
This snippet:
>+  // Verify the addon is installed
>+  am.setCategory({category: am.getCategoryById({id: "extensions"})});
>+  var addon = am.getAddons({attribute: "value", value: addonId})[0];
>+
>+  controller.assert(function() {
>+    return am.isAddonInstalled({addon: addon});
>+  }, "Add-on has been installed - got '" + am.isAddonInstalled({addon: >addon}) + 
>+      "', expected 'true'");
>+}
goes to the "Extensions" Panel, gets the addon from the list (of addons) and checks that it's "status"=="installed" and that it's not "remote"(isAddonInstalled).
Isn't it redundant to add a check that verifies the add-on is listed in the list of installed add-ons?

Comment 11

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

Whenever we are trying to install extensions we want to run those tests as restart tests, yes. The problem is the test cleanup. To be safe we would have to call a backend method to get rid of all additionally installed add-ons, but that's complicated because we can already inject other add-ons via the command line. So the safest thing here is really to use the restart capabilities.

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Please read the second part of https://bugzilla.mozilla.org/show_bug.cgi?id=664018#c10 and if necessary r- this.

Comment 13

[u279076]

Comment on attachment 542845 [details] [diff] [review]
patch v2.0

Patch looks and tests okay -- over to Geo for pre-checkin review.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 542845 [details] [diff] [review]
patch v2.0

Fundamentally the same test code reviewed in:

https://bugzilla.mozilla.org/show_bug.cgi?id=657497#c31

...and same style comments apply, re: spacing around binary operators, randomNumber, and the asserts.

r+, but again would prefer someone add spaces around the * in the random expression prior to checkin.

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Just noticed all the "function()" in the assert/waitFor code. Needs to be "function ()". So r=me, those changes.

Comment 16

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

Comment 17

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

Comment on attachment 543094 [details] [diff] [review]
patch v3.0

>+++ b/tests/remote/restartTests/testDiscoveryPane/test5.js

Same as for the other review. Each test should be located in its own subfolder.

>+const EXT_TIMEOUT = 25000;

This should be named TIMEOUT_DOWNLOAD.

>+function teardownModule() {
>+  am.close();
>+  prefs.preferences.clearUserPref(ADDONS_CACHE_ENABLED);

Reverse the order or use closeAllTabs. If am.close() fails we wouldn't reset the preference.

>+  var addonId = randomAddon.getNode()["data-guid"];

As mentioned please use .getAttribute()

>+  var md = new modalDialog.modalDialog(am.controller.window);

Move this down to md.start

>+  var addonUrl = addToFirefox.getNode().href;
>+  var addonSRC = addonUrl.indexOf("src=discovery-promo");
>+  var checkSRC = (addonSRC !== -1);
>+
>+  controller.assert(function () {
>+    return checkSRC;
>+  }, "Add-on URL has an SRC value - got '" + checkSRC +
>+     "', expected 'true'");

If you do not want to create the helper method on your own in addons.js, please file a bug and I will take care of immediately.

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 543094 [details] [diff] [review]
patch v3.0

With Henrik's feedback, I have to r-. But I don't see any additional problems past the ones he's pointed out.

Comment 19

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.1 (beta, release)

Comment 20

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.2 (aurora, nightly)

Comment 21

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

Comment on attachment 543399 [details] [diff] [review]
patch v4.1 (beta, release)

>+const ADDONS_CACHE_ENABLED = "extensions.getAddons.cache.enabled";

Can you please add the PREF_ prefix? I missed that before.

Otherwise it looks good and works fine.

Comment 22

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

Comment on attachment 543400 [details] [diff] [review]
patch v4.2 (aurora, nightly)

Same update as for the other patch please.

Comment 23

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.1.1 (beta, release) [checked-in]

Comment 24

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.2.1 (aurora, nightly)

Comment 25

[u279076]

Comment on attachment 543405 [details] [diff] [review]
patch v4.1.1 (beta, release) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/4a87ec2ba003 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/710009acc5c8 (mozilla-beta)

Comment 26

[u279076]

Comment on attachment 543406 [details] [diff] [review]
patch v4.2.1 (aurora, nightly)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/14745e6e3f4a (mozilla-aurora)

Test fails on Nightly (default) giving the following message:
"message": "randomAddon is undefined", "fileName": "resource://mozmill/modules/frame.js -> file:///home/ashughes/Development/Mozmill/DiscoveryPane/testFirstTimeAddon/nightly/mozmill-tests/tests/remote/restartTests/testDiscoveryPane_FirstTimeModule/test1.js", "name": "TypeError", "lineNumber": 79

Comment 27

[Alex Lakatos[:AlexLakatos]]

This fails on Nightly because the "First time with addons" panel has no addons in it. We should not check-in this on nightly.

Comment 28

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

Comment on attachment 543406 [details] [diff] [review]
patch v4.2.1 (aurora, nightly)

Ok, so please mark this test as skipped and add a good comment. For help see my comment on bug 657492.

Comment 29

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.2 (nightly)

Comment 30

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

Comment on attachment 544458 [details] [diff] [review]
patch v4.2 (nightly)

Please sync with Vlad in how he disables the test on bug 657492 on default.

Comment 31

[Alex Lakatos[:AlexLakatos]]

Attachment: parch v4.2 (nightly)

Skipped the test as instructed in the previous comment

Comment 32

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

Comment on attachment 545949 [details] [diff] [review]
parch v4.2 (nightly)

>+// XXX: Bug 664018
>+//      Skip because the "First Time with Addons" add-ons are not compatible with 
>+//      Nightly builds 
>+setupModule.__force_skip__ = "'First Time with Addons' add-ons " + 
>+                             "are not compatible with Firefox Nightly builds";
>+teardownModule.__force_skip__ = "'First Time with Addons' add-ons " + 
>+                                "are not compatible with Firefox Nightly builds";

Please use the bug number as prefix for the skip message. Also remove the 'Nightly' part so the message is the same across branches. Otherwise looks good.

Comment 33

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.2 (nightly)

Removed 'Nightly' and prefixed the message

Comment 34

[u279076]

Henrik, can this patch be landed? I miss the checkin-needed keyword.

Comment 35

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

I think so. Even that the version numbering is totally off here.

Comment 36

[u279076]

Comment on attachment 546761 [details] [diff] [review]
patch v4.2 (nightly)

>+// XXX: Bug 664018
>+//      Skip because the "First Time with Addons" add-ons are not compatible with 
>+//      Nightly builds 
>+setupModule.__force_skip__ = "Bug 664018: 'First Time with Addons' add-ons " + 
>+                             "are not compatible with Firefox builds";
>+teardownModule.__force_skip__ = "Bug 664018: 'First Time with Addons' add-ons " + 
>+                                "are not compatible with Firefox builds";

I still don't like this wording. A better unified message would be:
"Bug 664018 - 'First Time' add-ons are incompatible with this version of Firefox"

You should also use the same wording in both the comment and the skip message.

Resetting r flag so you can make these changes. Sorry it took this long.

Comment 37

[Alex Lakatos[:AlexLakatos]]

Attachment: skip patch (nightly) [checked-in]

Reworded to the suggested message

Comment 38

[u279076]

Comment on attachment 562032 [details] [diff] [review]
skip patch (nightly) [checked-in]

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

Comment 39

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #38)
> Comment on attachment 562032 [details] [diff] [review] [diff] [details] [review]
> skip patch (nightly) [checked-in]
> 
> Landed:
> http://hg.mozilla.org/qa/mozmill-tests/rev/c89c31559274 (default)

Does this need landing on Aurora as well?

Comment 40

[Alex Lakatos[:AlexLakatos]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #39)

> Does this need landing on Aurora as well?

Comment 26 says it's landed in aurora. I did a quick check in hg.mozilla.org and the test is there, enabled. brass-tacks say's it's passing on Firefox 8.a2 so this does not need landing in Aurora.

Comment 41

[u279076]

Test appears to be passing -- verified fixed.

Comment 42

[u415141]

Anthony Hughes changed story state to delivered in Pivotal Tracker

Comment 43

[u415141]

Anthony Hughes changed story state to accepted in Pivotal Tracker

Comment 44

[u415141]

Anthony Hughes changed story state to accepted in Pivotal Tracker