657497 - Mozmill test for installing an add-on from the "Up & Coming" module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Tracking bug to develop a Mozmill test for the following Litmus test:

Install an add-on from the 'Up &Coming' module
https://litmus.mozilla.org/show_test.cgi?id=15373

Comment 1

[u415141]

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

Comment 2

[Alex Lakatos[:AlexLakatos]]

I want to take a stab at this. Assigning to myself.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: WIP

WorkInProgress attached for feedback

Comment 4

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

Comment on attachment 535035 [details] [diff] [review]
WIP

>+      var firstUpAndComing = new elementslib.XPath(controller.window.document, "//section[@id='up-and-coming']/ul/li/a/span");
>+      var addToFirefox = new elementslib.XPath(controller.window.document, "//ul[@id='install']/li/div/div/p/a/span");

The documents you are referring here should not come from the 'controller' but from the browser element inside the addons manager. I have sent you the link earlier today on IRC.

>+      var installButton = new elementslib.Lookup(controller.window.document,
>+                                                 '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

I get a failure here when I hit Install manually:

"message": "Expression \"id(\"xpinstallConfirm\")\" returned null. Anonymous == false"

Looks like the dialog has been updated. We should better use a CSSSelector or XPath here instead of a Lookup.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Using:
> var installButton = new elementslib.Selector(controller.window.document, '.dialog-button[dlgtype="accept"]'); 
Gives:
> ERROR | Test Failure: {"exception": {"message": ": could not find element Selector: .dialog-button[dlgtype=\"accept\"]"

Using:
> var installButton = new elementslib.XPath(controller.window.document, "//button[@dlgtype='accept']");
Gives:
> ERROR | Test Failure: {"exception": {"message": ": could not find element XPath: //button[@dlgtype='accept']"

Comment 6

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

Attachment: Updated WIP

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: WIP v0.2

Needs a bit more polishing

Comment 8

[u279076]

Comment on attachment 535634 [details] [diff] [review]
WIP v0.2

>+ * Contributor(s):
>+ *   Alex Lakatos <[e-mail]>

Please add (original author) after your email address

>+var tabs = require("../../../lib/tabs");
>+
>+
>+function setupModule() {

Only one line between logical code blocks.

>+  // Select the Get Add-ons pane
>+  //am.setCategory({category: am.getCategoryById({id: "discover"})});

Remove any commented code for check-in patches.

>+  //I'm using sleep because there is an intermintent error here due to slow internet connections
>+  controller.sleep(10000);

There's probably some property or event you could be waiting for, in which case use a waitFor(). Only use a sleep() as a last resort.

>+  controller.assert(function() {
>+    return am.isAddonInstalled({addon: addon});
>+  }, "Addon is marked as being installed");

Can you change this message to something like:
"Add-on has been installed - got '" + am.isAddonInstalled({addon: addon}) + "'", expected 'true'" 

>+function handleTriggerDialog(controller) {
>+  // Wait for the install button is enabled before clicking on it
>+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

If this element is going to be commonly used in tests it might be worth adding a getter to the shared module. Henrik, what do you think?

Comment 9

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

This is for the beta branch. Here the Extensions pane has id=extensions, on aurora it has id=extension

Comment 10

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.1

added a few nits, so i'm resubmitting

Comment 11

[u279076]

Comment on attachment 536619 [details] [diff] [review]
patch v1.1

>+  am = new addons.AddonsManager(controller);
>+  tabs.closeAllTabs(controller);

Please separate with a newline.

>+  // Select the Get Add-ons pane
>+  am.setCategory({category: am.getCategoryById({id: "discover"})});
>+  var discovery = am.discoveryPane;
>+  discovery.waitForPageLoad();

Separate these into two blocks...probably "Select the Get Add-ons pane" and "Wait for the Get Add-ons pane to load"


>+  var upComing = discovery.getSection("up-and-coming");
>+  
>+  //Click on a random addon

Move the var under the comment, and make sure you put a space between // and the first letter in your comment.

>+  var addonList = discovery.getElements({type: "upAndComing_addons", parent: upComing});
>+  var randomNumber = Math.floor(Math.random()*addonList.length);
>+  var randomAddon = addonList[randomNumber];

I'm not familiar with Math.floor(). With this logic, is it ever possible for randomNumber to NOT equal an index of addonList[]? If so, we should probably wrap this is a conditional or some sort of loop.

>+  // XXX: If there is a large addon this will fail with the normal TIMEOUT. Using EXT_TIMEOUT untill we can use waitForDownloaded()

Break this comment into two lines and fix the typos.

>+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

You should move this XPath into the shared module as a constant: ADDON_INSTALL_BUTTON

>+  controller.waitFor(function(){
>+    return !installButton.getNode().disabled; 
>+  }, "Install button is enabled: got '" + !installButton.getNode().disabled + "', expected 'true'");
>+  controller.click(installButton);

Separate the waitFor() and click() by a new line.

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

> I'm not familiar with Math.floor(). With this logic, is it ever possible for
> randomNumber to NOT equal an index of addonList[]? If so, we should probably
> wrap this is a conditional or some sort of loop.

Math.floor() rounds the number. It will always give a result between 0 and addonList.length - 1

Comment 13

[u279076]

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

>+/**
>+ * Verifies that an addon from the Up&Coming section can be installed
>+ */

nit: simplify to "Tests installation of an Up & Coming add-on"

>+function testInstallUpAndComing() {

nit: please rename to "testInstallUpAndComingAddon() -- same with the file name

>+  // XXX: If there is a large addon this will fail with the normal TIMEOUT. 
>+  // Using EXT_TIMEOUT untill we can use waitForDownloaded()
>+  md.waitForDialog(EXT_TIMEOUT);

Please reference a bug number if one is on file.

>+  // Wait for the install button is enabled before clicking on it
>+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

XPath should be added to the shared module.

>+  controller.waitFor(function(){
>+    return !installButton.getNode().disabled; 
>+  }, "Install button is enabled: got '" + !installButton.getNode().disabled + "', expected 'true'");
>+
>+  controller.click(installButton);
>+}

You can probably use a waitThenClick() here.

Comment 14

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.1

(In reply to comment #13)
> >+  // Wait for the install button is enabled before clicking on it
> >+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');
> 
> XPath should be added to the shared module.
Please take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=657496#c9 . I'm going to need a definitive answer here: am I moving the xpath to addons.js or not?
> >+  controller.waitFor(function(){
> >+    return !installButton.getNode().disabled; 
> >+  }, "Install button is enabled: got '" + !installButton.getNode().disabled + "', expected 'true'");
> >+
> >+  controller.click(installButton);
> >+}
> 
> You can probably use a waitThenClick() here.
waitThenClick() waits for the element and then clicks it. The above mentioned code waits for a certain property to change and then clicks the element. Is there a function that does this in controller.js?(i haven't found one)

If there are no changes required please reflag for review?
Thanks.

Comment 15

[u279076]

Comment on attachment 537802 [details] [diff] [review]
patch v2.1

RE XPaths - keep them in the test for now. We should move them into the shared module in a follow-up bug, perhaps as part of the API refactor. Let's not worry about it for now.

RE waitThenClick: use the waitFor() as you have it. Your explanation is correct.

Here is my re-review:

>+  var addon = am.getAddons({attribute: "value", value: addonId})[0];
>+  controller.assert(function() {

Separate these two lines with a newline.

>+  }, "Add-on has been installed - got '" + am.isAddonInstalled({addon: addon}) + "', expected 'true'");

Wrap into two lines.

>+function handleTriggerDialog(controller) {

nit: please rename to handleInstallAddonDialog

>+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

Wrap into two lines.

>+  }, "Install button is enabled: got '" + !installButton.getNode().disabled + "', expected 'true'");

Wrap into two lines.

Comment 16

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.2

Comment 17

[u279076]

This test should also include an assert to cover bug 660789:

"Download URL is appended with src=discovery-upandcoming."

Comment 18

[u279076]

Comment on attachment 538011 [details] [diff] [review]
patch v2.2

>+  var installButton = new elementslib.Lookup(controller.window.document, '/id("xpinstallConfirm")/' +
>+                                             'anon({"anonid":"buttons"})/{"dlgtype":"accept"}');

Please format the XPath wrapping using the style I indicated in your other review.

>+  }, "Install button is enabled: got '" + 
>+     !installButton.getNode().disabled + "', expected 'true'");

For readability, I think it's better to have:

}, "Install button is enabled: got '" + !installButton.getNode().disabled +
    "', expected 'true'");

Thanks

Comment 19

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

Comment 20

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.1

Comment 21

[u279076]

Comment on attachment 538473 [details] [diff] [review]
patch v3.1

>+  controller.assert(function () {
>+    return addonUrl.indexOf("src=discovery-upandcoming") !== -1;
>+  }, "Add-on URL has an SRC value - got '" + addonUrl.indexOf("src=discovery-featured") + 
>+      "', expected different than'-1'.");

Sorry, but I'm starting to think a modification on the way you are doing it before is the right way to go. After looking at doing it this way doesn't seem quite right. Here is what I am thinking now:

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

Other than that, this patch looks ok. Please update so I can test it.

Comment 22

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.2

Comment 23

[u279076]

Comment on attachment 539164 [details] [diff] [review]
patch v3.2

Codewise this looks fine. Holding off final review until I can get these remote tests running locally.

Comment 24

[Alex Lakatos[:AlexLakatos]]

Here is the screencast
http://dl.dropbox.com/u/22633148/Mozmill/screencast657497.ogv

Comment 25

[u279076]

>  var discovery = am.discoveryPane;
>  discovery.waitForPageLoad();

There is a failure point in here due to an insufficient timeout. Please refactor to use a waitFor() instead, as per email instructions.

Comment 26

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

Comment 27

[u279076]

Comment on attachment 540723 [details] [diff] [review]
patch v4.0

This test fails with "Disconnect Error: Application unexpectedly closed" using Firefox 5.0 Release on mozilla-release.

Comment 28

[u279076]

Looks like this failure is a regression from bug 666245. While waiting for that to be resolved can you make one minor change...

Please move this test to mozmill-tests/tests/remote/restartTests/testDiscoveryPane/test3.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 29

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.0

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

Comment 30

[u279076]

Comment on attachment 542842 [details] [diff] [review]
patch v5.0

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

Comment 31

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

Comment on attachment 542842 [details] [diff] [review]
patch v5.0

Functionality looks fine. I have some style comments, which you can consider optional but perhaps something to do in the future.

>+  var randomNumber = Math.floor(Math.random()*addonList.length);
>+  var randomAddon = addonList[randomNumber];

Please use spaces around binary operators in expressions (the *), makes them easier to read:

Also, as it's used, it's more than a number; it's a random index. It's best to favor names that reflect the intended usage, so I'd have gone with randomIndex. 

That also would've clarified the floor/random expression without needing a comment. Right now, it's a little unclear if you don't know that Math.random() returns 0 inclusive..1 exclusive.

+  controller.assert(function() {
+    return am.isAddonInstalled({addon: addon});
+  }, "Add-on has been installed - got '" + am.isAddonInstalled({addon: addon}) + 
+      "', expected 'true'");
+}

I generally think that we should keep expressions (anything more than a single variable) out of the assertions:

Always assign to a variable above, preferably such that the final value is true if it passes, and name the variable after the "true" condition (addonIsInstalled, in this case).

I wouldn't necessarily make that suggestion if it weren't for the rather baroque assertion messages we're insisting on people using each and every time. Right now it's guaranteed to duplicate between the assertion and the message, and any long expression tends to make things horribly unclear.

On a side note, we really should've wrapped assert with something that appended the got/expected to the message, because this way actually makes the test less clear to read and is kind of ridiculous to have to keep duplicating everywhere. But we didn't, so I guess we're stuck. :/

Anyway, r+, as far as I'm concerned. If someone could add the spaces around the * before checkin though, that'd be nice.

Comment 32

[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 33

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.1

Comment 34

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

Comment on attachment 543095 [details] [diff] [review]
patch v5.1

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

Why 'testDiscoveryPane/test3.js'? What are test1 and test2? Sadly we do not support more than 2 levels of subfolders yet, so you will have to make the testDiscoveryPane folder including the details what you are testing.

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

Please use .getAttribute() instead which should be used to retrieve DOM attributes.

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

Can you move down the declaration right before the md.start call. We should group code as best as possible.

>+  var addonUrl = addToFirefox.getNode().href;
>+  var addonSRC = addonUrl.indexOf("src=discovery-upandcoming");

addonUrl isn't used later, so you can simply combine those two lines. 

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

As Geo has mentioned on his other reviews checkSRC isn't a name which tells someone what the value is we are comparing against. So something like hasSRCAttribute is more descriptive.

Since this check is used across different tests we should probably move that into the addons.js DiscoverPane class as a method like checkAddonSRC().

>+  // XXX: Bug 659487 - Update addons.js module to incorporate remaining changes in Firefox 4
>+  // If there is a large addon this will fail with the normal TIMEOUT. 
>+  // Using EXT_TIMEOUT untill we can use waitForDownloaded()
>+  md.waitForDialog(EXT_TIMEOUT);

We need support for the notification panel. Can you please mention that on bug 659487? I just don't want that we forget about it. Also there is no need for a comment why a longer timeout is necessary. Please rename the constant to TIMEOUT_DOWNLOAD so we are consistent with other tests and modules.

Comment 35

[u279076]

(In reply to comment #34)
> Comment on attachment 543095 [details] [diff] [review] [review]
> patch v5.1
> 
> >+++ b/tests/remote/restartTests/testDiscoveryPane/test3.js
> 
> Why 'testDiscoveryPane/test3.js'? What are test1 and test2? Sadly we do not
> support more than 2 levels of subfolders yet, so you will have to make the
> testDiscoveryPane folder including the details what you are testing.

This was my suggestion. I explicitly asked for a specific test# for each of the Discovery Pane tests. Did you want a second subfolder for each test and then every test is test1.js?

Example, this test would be tests/remote/restartTests/testDiscoveryPane/testUpAndComingModule/test1.js

Comment 36

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

This has been answered via email already.

Comment 37

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

Comment on attachment 543095 [details] [diff] [review]
patch v5.1

With Henrik's feedback, must r-. I don't see additional problems.

Comment 38

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v6.0

Comment 39

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

Comment on attachment 543385 [details] [diff] [review]
patch v6.0

>+  // Install the addon
>+  var addToFirefox = discovery.getElement({type: "addon_installButton"});
>+  var hasSRCAttribute = (discovery.getInstallSource(addToFirefox) == "discovery-upandcoming");
>+
>+  controller.assert(function () {
>+    return hasSRCAttribute;
>+  }, "Add-on URL has an SRC value - got '" + hasSRCAttribute +
>+     "', expected 'true'");

As mentioned already on vlad's patch lets do the check inside of assert and update the message to mention the expected and the real value. See my review comment on the pick of the month test.

Does this test work on Nightly and Aurora builds?

Comment 40

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v6.1

Comment 41

[Alex Lakatos[:AlexLakatos]]

(In reply to comment #39)
> 
> Does this test work on Nightly and Aurora builds?
It does not work on Aurora and Nightly because there category id is "extension" and in Firefox 5 it is "extensions".
Should I make a separate patch to be checked in on Aurora and Nightly?

Comment 42

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

Comment on attachment 543386 [details] [diff] [review]
patch v6.1

>+  }, "Add-on URL has a SRC value - got '" + currentInstallSource +

Don't reference 'Add-on URL' here because that's confusing. See my comment on vlads patch.

Otherwise looks good. Please also give feedback about the results on Nightly and Aurora.

Comment 43

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v6.2.1 (beta, release) [checked-in]

to be checked in on mozilla-beta and mozilla-release

Comment 44

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v6.2.2 (nightly, aurora) [checked-in]

to be checked-in on mozilla-aurora and default

Comment 45

[u279076]

Comment on attachment 543392 [details] [diff] [review]
patch v6.2.1 (beta, release) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/2d99b1476d19 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/d671e67e1ad8 (mozilla-beta)

Comment 46

[u279076]

Comment on attachment 543393 [details] [diff] [review]
patch v6.2.2 (nightly, aurora) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/b9847f5acf1b (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/6c08b0d8b4eb (default)

Comment 47

[u279076]

Litmus testcase and tracking documents have been updated.