658369 - Mozmill test for installing an add-on with a EULA

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Tracking bug for developing a Mozmill test for the following Litmus test:

Install an add-on with EULA from the discovery pane
https://litmus.mozilla.org/show_test.cgi?id=17324

Comment 1

[u415141]

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

Comment 2

[u279076]

CCing WebQA on this bug. Apparently the Litmus test is broken. The test says that you can get EULA addons from the Recommended section. However, this does not appear to be the case.

Can WebQA confirm?

Comment 3

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

Krupa or Marlena, can we please get feedback from you?

Comment 4

[u279076]

One minor note, please make this test at mozmill-tests/tests/remote/restartTests/testDiscoveryPane/test7.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 5

[Maniac Vlad Florin (:vladmaniac)]

Still waiting for feedback from webQA regarding the litmus test, to determine if it is unclear/broken and what exactly steps we should have in our mozmill test. Thanks!

Comment 6

[krupa raj[:krupa]]

Attachment: screenshot of EULA

From the disco pane, click an add-on which has EULA (Echofon for twitter)
In the details page, click on 'Continue to Download' button

observed behavior:
EULA opens up in the disco pane

Comment 7

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

How can we make sure that this add-on is always listed in the disco pane?

Comment 8

[krupa raj[:krupa]]

We can probably add it to the promo module in staging. But this will be deleted anytime we do a DB refresh.

Comment 9

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

Krupa, what's the URL of the frame inside the discovery pane for this add-on? I don't have it listed anywhere myself. I wonder if we could directly go to the details page.

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Our interest is to get this add-on out of the discovery pane, otherwise it will not be a disc pane test anymore. we have to force a situation to have this type of addon always there. So i second Henrik's idea

Comment 11

[krupa raj[:krupa]]

(In reply to comment #9)
> Krupa, what's the URL of the frame inside the discovery pane for this
> add-on? I don't have it listed anywhere myself. I wonder if we could
> directly go to the details page.


prod: https://services.addons.mozilla.org/en-US/firefox/discovery/addon/echofon-for-twitter/?src=discovery-featured

staging: https://addons.allizom.org/en-US/firefox/discovery/addon/echofon-for-twitter/?src=discovery-featured

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Henrik, this test is a little left behind, and is a require for Discovery Pane. What would be the final approach here?

Comment 13

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

Because we can't predict if any of the sections show an add-on with an EULA I would go the route by just opening the URL from above for Echofon on production, and run the installation of the add-on from there.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

This workaround sounds great for me, thanks!

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0 for release branch

Based on Henrik's decision, uploading first patch. 

Used selectors to retrieve elements which are not in API. I don't recommend adding them to a shared module since we work on the AMO production site only this once. 

Thanks!

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0 for release branch

Ups! something unwanted slipped in 

Fixed patch

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 549050 [details] [diff] [review]
patch v1.0 for release branch

>+const ADDON_PAGE = "https://services.addons.mozilla.org/en-US/firefox/" + 
>+                   "discovery/addon/echofon-for-twitter/?src=discovery-featured";
>+const ADDON_NAME = "Echofon";

Please make this an array with a key value pair.

>+/*
>+ * Tests installation of EULA add-on
>+ * XXX: Bug 658369

Please add a blank comment line between these two lines.

>+function testInstallEulaAddon() {

Please use ALL_CAPS for "Eula"

>+  // Click on continue to download link
>+  var continueToDownload = new elementslib.Selector(controller.window.document, 
>+					            ".install-action");

Please fix the indentation, rename variable to continueToDownloadLink

>+  var acceptAndInstall = new elementslib.Selector(controller.window.document, 
>+				                  ".install-button");

Please fix indentation, rename to acceptAndInstallButton

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1 for release branch

Added patch with requested changes. All nits fixed. 

Comments: 
* This patch is working only with firefox release and its intended to be in that specific branch.

Comment 19

[u279076]

Comment on attachment 551406 [details] [diff] [review]
patch v1.1 for release branch

>+++ b/tests/remote/restartTests/testDiscoveryPane_installEulaAddon/test1.js

nit: upper case EULA

>+/*
>+ * Tests installation of EULA add-on
>+ *
>+ * XXX: Bug 658369
>+ *      Retrieving the add-on by direct access of its detailed page because 
>+ *      at the moment we can't predict that any of the sections will provide 
>+ *      an add-on with EULA
>+ */

Fixing this issue should be filed as an API bug. The API bug number should then be the XXX reference.

>+function testInstallEULAAddon() {

The 'AA' makes this name hard to read. Please rename to testInstallAddonWithEULA (do the same to the file name).

>+  // Click on continue to download link
>+  var continueToDownloadLink = new elementslib.Selector(controller.window.document, 
>+					                 ".install-action");

Can you email me a list of all needed Selectors for the Discovery Pane so I can work on getting those in the API?

>+  assert.ok(addonsManager.isAddonInstalled({addon: addon}), 
>+            "The add-on has been correctly installed");
>+}

Please include the add-on name in the message.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

>+/*
>+ * Tests installation of EULA add-on
>+ *
>+ * XXX: Bug 658369
>+ *      Retrieving the add-on by direct access of its detailed page because 
>+ *      at the moment we can't predict that any of the sections will provide 
>+ *      an add-on with EULA
>+ */

This is not an API issue. It's just that we can't know for sure if the discovery pane will provide us with an EULA add-on. Those are random they are not categorized by any criteria. That's why this bug is in the right place.

Comment 21

[u279076]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #20)
> >+/*
> >+ * Tests installation of EULA add-on
> >+ *
> >+ * XXX: Bug 658369
> >+ *      Retrieving the add-on by direct access of its detailed page because 
> >+ *      at the moment we can't predict that any of the sections will provide 
> >+ *      an add-on with EULA
> >+ */
> 
> This is not an API issue. It's just that we can't know for sure if the
> discovery pane will provide us with an EULA add-on. Those are random they
> are not categorized by any criteria. That's why this bug is in the right
> place.

In this case, file a bug against Discovery Pane to provide that information. Even if they mark it WONTFIX at least we can refer to something. I don't like XXX comments which are self-referential.

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

Filed bug 678478 for the disc pane component on addons.mozilla.org product. 

Anyways, the patch is ready for r? except the adding selectors to the API. If this is really necessary and urgent, the patch can wait until that. 

If not, we can submit the patch, r on it, then check in. We can change with the API revisions later. 

Ashughes, your call! (in the meantime, i'll get you the list of selector and elements we will want to have in the API)

Comment 23

[u279076]

Now that you have filed the bug, please update the XXX block with the blocker bug number.

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Fixed! 

This patch will work for all branches, release, default, aurora and beta, except Firefox 4 and Firefox 5 releases (in which case will error with "Categories could not be found" because the extensions category in the discovery pane is named id = "extensions" instead of "extension") 

With this, i believe this will be the final patch for this test!

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Cancelling the r? - I'll make the shared module changes first so we don't need to modify again

Comment 26

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Since we can't get those elements into the API, we'll just have to have them in the test. 

The patch has one small addons.js update, so we don't need a separate bug for that. 

The patch is intended to work with all branches, minus Firefox 4 and Firefox 5 version, which will have a separate patch if needed.

Comment 27

[u279076]

Comment on attachment 554086 [details] [diff] [review]
patch v1.2

>+  // XXX: Bug 680045
>+  //      Add elements to UI map for add-ons with EULA  
>+  var continueToDownloadLink = new elementslib.Selector(controller.window.document, 
>+					                 ".install-action");

Let's make this a CONSTANT so it's easy to replace...
SELECTOR_INSTALL-ACTION = ".install-action"

Comment 28

[krupa raj[:krupa]]

when the test fires, can it not have a setup stage where it says here are the 6 add-ons I want displayed in the featured section? That way, we have complete control and will be able to test all kinds of edge cases.

Comment 29

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

How? The selection of the addons happens on the server side depending on the add-ons installed locally. We do not know about the logic so it will be nearly impossible for us. But lets ask a dev from AMO.

Comment 30

[u279076]

Krupa or Marlena, can you please get an AMO dev on this bug to provide us some expert feedback RE: comment 29?

Thanks

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

Guys, how will comment 28, comment 29 and comment 30 affect my progress on this bug? 
Do we have those changes in plan for the immediate future? If not, i can easily fix nit from comment 27 and have this patch checked in. Thanks!

Comment 32

[krupa raj[:krupa]]

Cvan, can you take a look? Thanks!

Comment 33

[u279076]

Comment on attachment 554086 [details] [diff] [review]
patch v1.2

Canceling review until WebDev has a chance to look at this bug. Vlad, please re-flag once this has been addressed.

Comment 34

[Christopher Van Wiemeersch [:cvan]]

Comment on attachment 554086 [details] [diff] [review]
patch v1.2

>+					                 ".install-action");
There are a bunch of tabs here.

>+				                         ".install-button");
Here too.

>+    
Trailing white space.


I'm not familiar with reading mozmill tests, but I don't see any obvious issues and the logic seems okay.

Comment 35

[u279076]

@Chris, that's not really the feedback we were looking for. Please see comment 28 and 29.

Comment 36

[Christopher Van Wiemeersch [:cvan]]

(In reply to krupa raj 82[:krupa] from comment #28)
> when the test fires, can it not have a setup stage where it says here are
> the 6 add-ons I want displayed in the featured section? That way, we have
> complete control and will be able to test all kinds of edge cases.

No, we can't.

But directly testing that add-on seems fine to me. This test is okay IMO.

Comment 37

[Maniac Vlad Florin (:vladmaniac)]

Adding checkin-needed as Chris r+ -ed the patch. Anthony, what do you think?

Comment 38

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

Chris is not a reviewer for Mozmill tests. Please ask Anthony for review.

Comment 39

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #38)
> Chris is not a reviewer for Mozmill tests. Please ask Anthony for review.

Well if he did review it, I figured he is :) Thanks for correcting this one

Comment 40

[u279076]

Comment on attachment 554086 [details] [diff] [review]
patch v1.2

>+  // Verify the add-on is installed
>+  addonsManager.open();
>+  addonsManager.setCategory({
>+    category: addonsManager.getCategoryById({id: "extension"})
>+  });
>+
>+  var addon = addonsManager.getAddons({attribute: "name", value: ADDON.name})[0];
>+
>+  assert.ok(addonsManager.isAddonInstalled({addon: addon}), 
>+            "The add-on has been correctly installed");
>+}

Just a small nit. I'd like to see the commenting moved around.

>+  // Open the Add-ons Manager
>+  addonsManager.open();

>+  // Verify the add-on is installed
>+  var addon = addonsManager.getAddons(...

Comment 41

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Fixed

Comment 42

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

Comment on attachment 559121 [details] [diff] [review]
patch v1.3

> function handleInstallAddonDialog(aController) {
>   // Get the install button
>   // XXX: Bug 671322 
>   //      Using .documentElement until we can support the window object as parameter
>   //      for the nodeCollector constructor
>-  var nodeCollector = new domUtils.nodeCollector(aController.window.document.documentElement);
>+  var nodeCollector = new domUtils.nodeCollector(aController.window.document);
>   nodeCollector.queryNodes("#xpinstallConfirm");
>   nodeCollector.root = nodeCollector.nodes[0];
>   nodeCollector.queryAnonymousNodes("dlgtype", "accept");

Remove the comment because the bug has been fixed. Also pass in aController.window into the constructor.

Comment 43

[u279076]

Comment on attachment 559121 [details] [diff] [review]
patch v1.3

Canceling my review due to Henrik's comment. Please re-request review once his changes are implemented.

Comment 44

[Maniac Vlad Florin (:vladmaniac)]

Can we also add the Echofon add-on to the mozmill-tests/data folder? right now Henrik advised to install it from production but why do that when we can have it locally? 

Your opinions are highly appreciated.

Comment 45

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

Those are remote tests and do not belong to the function test-run. Only the latter has the requirement that we want to have local data as much as possible. In this case we really test the EULA page of AMO. So we can't make it local in any case.

Comment 46

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4 release branch only

Fixed nits. 

The addons.js modifications are no longer needed in the patch => were deleted 
This patch is intended for the release branch only because the Echofon add-on is not compatible with other branches at the moment. 

Please test against release branch

Comment 47

[Maniac Vlad Florin (:vladmaniac)]

Attachment: test results file

Runned the test 100 times - there are 2/100 disconnect failures caused by local hardware issues

Comment 48

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

Comment on attachment 560161 [details] [diff] [review]
patch v1.4 release branch only

>+++ b/tests/remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js

What about test2.js to really verify that the add-on has been installed? I miss it in this patch.

>+const ADDON = {
>+  name: "Echofon",
>+  page: "https://services.addons.mozilla.org/en-US/firefox/" + 
>+        "discovery/addon/echofon-for-twitter/?src=discovery-featured"
>+};

We need a comment that this test will really only work on release builds. Otherwise we will get into trouble on other branches. That means we would need an updated patch for the other branches with the correct skipped messages.

Comment 49

[u279076]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #47)
> Created attachment 560163 [details]
> test results file
> 
> Runned the test 100 times - there are 2/100 disconnect failures caused by
> local hardware issues

Could you be more specific? I'd rather not check this in if its an issue which might appear in the wild.

Comment 50

[u279076]

Comment on attachment 560161 [details] [diff] [review]
patch v1.4 release branch only

Nothing to add to Henrik's feedback.

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #48)
> Comment on attachment 560161 [details] [diff] [review]
> patch v1.4 release branch only
> 
> >+++ b/tests/remote/restartTests/testDiscoveryPane_installAddonWithEULA/test1.js
> 
> What about test2.js to really verify that the add-on has been installed? I
> miss it in this patch.
> 
> >+const ADDON = {
> >+  name: "Echofon",
> >+  page: "https://services.addons.mozilla.org/en-US/firefox/" + 
> >+        "discovery/addon/echofon-for-twitter/?src=discovery-featured"
> >+};
> 
> We need a comment that this test will really only work on release builds.
> Otherwise we will get into trouble on other branches. That means we would
> need an updated patch for the other branches with the correct skipped
> messages.

We have decided in the past that we don't need a test2.js in the remote tests. All the other remote tests we have already checked in do not have test2.js 

The comment I agree with. Once we can check this on release branch, see if it fails or not on your machines, we'll check in the others with skipped if necessary.

Comment 52

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #49)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #47)
> > Created attachment 560163 [details]
> > test results file
> > 
> > Runned the test 100 times - there are 2/100 disconnect failures caused by
> > local hardware issues
> 
> Could you be more specific? I'd rather not check this in if its an issue
> which might appear in the wild.

The disconnect failure appears because the test was interrupted by a system message. I really need to get another machine from somewhere which purpose will be only running tests.

Comment 53

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #51)
> We have decided in the past that we don't need a test2.js in the remote
> tests. All the other remote tests we have already checked in do not have
> test2.js 

Can you please point me to the location when we have decided that? I can't remember it right now. Thanks.

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

Sure Henrik 

https://bugzilla.mozilla.org/show_bug.cgi?id=657496#c6

Hope this helps.

Comment 55

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

Ok, lets go this way. If it turns out the decision was wrong we can change it later. For now we will be consistent with all the other already checked-in tests.

Comment 56

[Maniac Vlad Florin (:vladmaniac)]

Thanks Henrik

Comment 57

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5 release branch only

Followed up with the next patch: 

Fix: Commented that the test will work only against release branch
Hope it's commented in the right place, as Henrik's feedback points out

Comment 58

[u279076]

Comment on attachment 560539 [details] [diff] [review]
patch v1.5 release branch only

>+// XXX: This test is intended to work on release branch only due to add-on 
>+//      incompatibility issues on the other branches

Can you move this to the comment block for the test function?
 
Fix this nit and we can land it. Thanks.

Comment 59

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

I wouldn't even use this comment on a patch for the release branch. This should really only be a comment for the skipped messages on the other branches.

Comment 60

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #59)
> I wouldn't even use this comment on a patch for the release branch. This
> should really only be a comment for the skipped messages on the other
> branches.

Ok, so we would like to delete the comment and use one when skipping patches for other branches? 

What is the final decision here?

Comment 61

[u279076]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #60)
> What is the final decision here?

Remove the comment altogether -- only comment the skipping block.

Comment 62

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.6 release branch only [checked-in]

Fixed. 

Comment is removed for release branch patch. Once we are confident that this won't fail, we'll have skip patches for the other branches. Does this plan sound decent?

Comment 63

[u279076]

Comment on attachment 562015 [details] [diff] [review]
patch v1.6 release branch only [checked-in]

Patch looks fine to me. Can you please follow this up with patches for skipping the test on other branches?

Comment 64

[u279076]

Comment on attachment 562015 [details] [diff] [review]
patch v1.6 release branch only [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c32846cb4d40 (mozilla-release)

Comment 65

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [skip] patch v1.6 default branch

Skipping the test for default branch due to add-on incompatibility reason

Comment 66

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [skip] patch v1.6 aurora branch

Skipping the test for aurora branch due to add-on incompatibility reason

Comment 67

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [skip] patch v1.6 beta branch

Skipping the test on beta branch due to add-on incompatibility reason

Comment 68

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

Please do not create different patches for each branch. Instead list all to skip branches in the comment. That will help a lot during the merge process!

Comment 69

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #68)
> Please do not create different patches for each branch. Instead list all to
> skip branches in the comment. That will help a lot during the merge process!

Thanks Henrik.

Vlad, as Henrik says, please merge all three branch patches into a unified patch with a unified skip message. Thanks.

Comment 70

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [skip] patch v1.6 default, aurora, beta

Unified skip patch 

Please apply the patch for default, aurora and beta branch. we only have go for release on this test at the moment. thanks

Comment 71

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

Comment on attachment 564760 [details] [diff] [review]
[skip] patch v1.6 default, aurora, beta

As I have mentioned in my last comment, you should name those versions of Firefox the test cannot be run for. Also we should really stop to use the XXX comment here because it's really only a duplication of the information which is already present in the __force_skip__ message.

Comment 72

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [skip] patch v1.7 default, aurora, beta [checked-in]

fixed

Comment 73

[u279076]

Comment on attachment 564810 [details] [diff] [review]
[skip] patch v1.7 default, aurora, beta [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/b1066b4ed2ea (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/2a76812fb7c2 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/505eb554e3b6 (mozilla-beta)

Comment 74

[u279076]

I believe this can be marked FIXED now. Please reopen if that is a mistake, mark verified if not.

Comment 75

[Maniac Vlad Florin (:vladmaniac)]

This is verified