658365 - Mozmill test for installing an add-on from "Recommended for you"

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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

Install an add-on from the 'Recommended for you' section
https://litmus.mozilla.org/show_test.cgi?id=17323

Comment 1

[u415141]

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

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

This test needs to be run with --addons command since it must run in a profile with at least 4 add-ons installed. Tested if the Recommended panel is visible due to this condition, but maybe it would be thorough to also test if we have the 4 add-ons successfully installed. What do you say, Anthony?

Comment 3

[u279076]

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

Canceling review on this patch for now. I don't know if this test is necessary -- it might be sufficiently covered by a Selenium test. Let's ask WebQA.

Comment 4

[u279076]

CCing Krupa and Dave Burns on this bug. Is this test sufficiently covered by Selenium or is there value in having a Mozmill test?

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

This test requires installation so I think i can ask for review back, based on webQA response in the e-mail

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Uploaded 1.1 with changes to fit style guide

Comment 7

[u279076]

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

>+++ b/tests/remote/testDiscoveryPane/testInstallRecommended.js

nit: Please rename to testInstallRecommendedAddon.js

>+function testInstallRecommended() {

nit: Please rename to testInstallRecommendedAddon()

>+  am.setCategory({category: am.getCategoryById({id: "discover"})});
>+  var discovery = am.discoveryPane;

Can you put a newline in here?

>+  // Verify that the Recommended Add-ons section is visible
>+  var displayVisible = (section.getNode().style["display"] !== 'none');
>+
>+  controller.assert(function () {
>+    return displayVisible;
>+  }, "The Recommended Add-ons section should be visible - got '" +
>+     displayVisible + "', expected 'true'.");

I would refactor this a little bit. Instead of doing the comparison outside of the waitFor(), lets do it inside. Simply assign the style to a variable outside the waitFor() and compare in your return statement.

>+  //Click on the first addon
>+  var firstAddon = elems[0];
>+  addonId = firstAddon.getNode()["data-guid"];

Can you simplify this into a single line? Also, make sure there is a space between // and C in your comment.

>+function handleTriggerDialog(controller) {

nit: please rename to handleInstallAddonDialog

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

Can you wrap the XPath parameter to a second line aligned with the 'c' in controller?

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

Can you break this into a wrapped line as well?

Thanks

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Uploaded new version with all requested changes.

Comment 9

[u279076]

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

>+  // Verify that the Recommended Add-ons section is visible
>+  var displayVisible = section.getNode().style["display"];

nit: Please call this variable "sectionDisplayStyle"

>+  controller.assert(function () {
>+    return (displayVisible !== 'none');

nit: You don't need the brackets around this return statement

>+  }, "The Recommended Add-ons section should be visible - got '" +
>+     (displayVisible !== 'none') + "', expected 'true'.");

For "got", just use the variable name, not the comparison
For "expected", use whatever style value is expected

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Comment 11

[u279076]

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

>+  var firstAddon = discovery.getElements({type: "recommendedAddons_addons", 
>+                                         parent: section})[0]; 

Align the 'p' in parent with the 't' in type.

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

This seems terribly fragile. Is there no better way to get the Add To Firefox button (maybe by ID or Name)?

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

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

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4

Done

Comment 13

[u279076]

Comment on attachment 538452 [details] [diff] [review]
patch v1.4

>+  // 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"}');

The indentation is still wrong. Please move the last two lines back 2 spaces.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Will do, in the editor it looks fine..it's just how the patch was created..makes no sense

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5

Patch including test for checking the src attribute of the addon link

Comment 16

[u279076]

Comment on attachment 538854 [details] [diff] [review]
patch v1.5

Codewise this is fine but I cannot get it to run without failing. I get an error about "specified tab hasn't been found" immediately after the discovery pane is loaded.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Please run this test with --addons command, and have at least 4 addons pre-installed at runtime. "Specified tab hasn't been found" error is probably because the recommended for you pane is not visible. Is this the case?

Comment 18

[u279076]

(In reply to comment #17)
> Please run this test with --addons command, and have at least 4 addons
> pre-installed at runtime. "Specified tab hasn't been found" error is
> probably because the recommended for you pane is not visible. Is this the
> case?

I've tried this and it's still failing:

mozmill -b ~/Desktop/firefox/firefox --addons=/home/ashughes/Development/add-ons/ -t ./tests/remote/testDiscoveryPane/testInstallRecommendedAddon.js --show-errors

I have 4 add-ons in the add-ons folder all compatible with Firefox 5.0b5. They all get installed when I start with Mozmill. The discovery pane appears to load but fails immediately after with the following error:

Test Failure: {"exception": {"stack": "([object Proxy],(void 0),(void 0))@resource://mozmill/modules/controller.js:1275\n@:0\n", "message": "controller.waitForPageLoad(): Specified tab hasn't been found.", "fileName": "resource://mozmill/modules/controller.js", "name": "Error", "lineNumber": 1275}}

I think it would be helpful to see a screencast of you running this test successfully, including the cloning of a new repository and applying the patch.

Comment 19

[Alex Lakatos[:AlexLakatos]]

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

Comment 20

[u279076]

Seems like my Mozmill version was out of date -- please make sure you are using Mozmill 1.5.4b3.

Comment 21

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

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.6

Comment 23

[u279076]

Comment on attachment 540740 [details] [diff] [review]
patch v1.6

Test fails with "Disconnect Error: Application unexpectedly closed" using Firefox 5.0 Release on mozilla-release. It times out handling the modal install dialog. Probably something needs to be handled in your handler function.

Comment 24

[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/test4.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 25

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.7

This patch also tests if the addon is installed and present in the addons list

Comment 26

[u279076]

Comment on attachment 542749 [details] [diff] [review]
patch v1.7

Test fails on line 81 with a "TypeError" exception and resulting in a failure of "firstAddon is undefined".

>  // Click on the first addon
>  addonId = firstAddon.getNode()["data-guid"];
>  controller.click(firstAddon);

Please fix this failure and resubmit for review.

Comment 27

[Maniac Vlad Florin (:vladmaniac)]

You are right. It fails with this error 1 out of 10 times when running the test

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

I've tried to refactor this using getNode().getAttribute("data-guid") and installing a random addon rather than just retrieving the first one on the list. 

It still fails sometimes with this error and I'm out of ideas at the moment.

Comment 29

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

Attach the code to the bug, otherwise we cannot help. Also add some dump statements to output values to the console. That will help you to nail down the issue.

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

works for me - tested 20 runs with command 

mozmill-restart -t tests/remote/restartTests/testDiscoveryPane_installRecommendedAddon/ -b /home/vladmaniac/fx5release/firefox/firefox --show-all --addons /home/vladmaniac/addons

i tend to think bug 668835 also fixed the problem which failed this test. 
Please correct me otherwise.

Comment 32

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

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

>+  var addonList = discovery.getElements({type: "recommendedAddons_addons", 
>+                                       parent: section});

nit: please fix the indentation. The 'p' should start right under type.

>+  // Verify that the Recommended Add-ons section is visible
>+  var sectionDisplayStyle = section.getNode().style["display"];
>+
>+  controller.assert(function () {
>+    return sectionDisplayStyle !== 'none';
>+  }, "The Recommended Add-ons section should be visible - got '" +
>+     sectionDisplayStyle + "', expected 'block'.");

We have this test only to ensure that the right sub section is visible? If that's the case please expand the comment to say that we are waiting for the id of the active sub section added to the parent element. Also reference the bug # you have filed.

>+  // Install the addon
>+  var addToFirefox = discovery.getElement({type: "addon_installButton"});
>+
>+  // Retrieve addon src parameter from installation link
>+  var currentInstallSource = discovery.getInstallSource(addToFirefox);

Can you please align those lines accordingly to the tests which have already been landed?

Please make those changes and also request review from Anthony. And yes, I believe the failure we were seeing here was caused by the multiple platform install button.

Comment 33

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.1

Comment 34

[u279076]

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

Since this hasn't been checked in yet on any branches, can you please refactor your assert()s to use Expect() from the new Assertions module?

Keep your waitFor() though.

Comment 35

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

Anthony, as I have proposed on another bug we should probably keep the old assert calls for those type of tests to stay consistent. What do you think?

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

If we are to keep the old assert, then this patch is eligible for r.

Comment 37

[u279076]

(In reply to comment #35)
> Anthony, as I have proposed on another bug we should probably keep the old
> assert calls for those type of tests to stay consistent. What do you think?

If a test has not been checked in on any branches yet, we should use the new assert method. However, if it has been checked in on at least one branch already, we should not refactor.

This test has not yet been checked in on any branches so I would like it to use the new assertion module.

Comment 38

[Maniac Vlad Florin (:vladmaniac)]

(In reply to comment #26)
> Comment on attachment 542749 [details] [diff] [review] [review]
> patch v1.7
> 
> Test fails on line 81 with a "TypeError" exception and resulting in a
> failure of "firstAddon is undefined".
> 
> >  // Click on the first addon
> >  addonId = firstAddon.getNode()["data-guid"];
> >  controller.click(firstAddon);
> 
> Please fix this failure and resubmit for review.

This is going to be fixed in patch v2.2 for release branch. 

The problem was that the Recommended section was loaded lazily and sometimes 
the retrieved addon list array was not populated with add-ons. 
Fixed that with a waitFor()

Comment 39

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.2 for release branch

Fixed with requested changes. Also fixed the unreproducible error we were facing sometimes. 

Please test with mozilla-release branch and mozmill 1.5.4b6 version. 

Thanks!

Comment 40

[u279076]

Comment on attachment 545357 [details] [diff] [review]
patch v2.2 for release branch

>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  am = new addons.AddonsManager(controller);

Please use addonsManager instead of am

>+    var section = discovery.getSection("recs");
>+    // Get the addons list array of elements

Separate with a newline.

>+    var addonList = discovery.getElements({type: "recommendedAddons_addons", 
>+                                         parent: section});

Fix alignment

>+    persisted.currentSection = section;
>+    // Store addons list array in the persisted object

Separate with a newline

>+  expect.notEqual(sectionDisplayStyle, "none", "The Recommended " + 
>+                  "Add-ons section is visible");

Put the whole message on the 2nd line

>+  expect.equal(currentInstallSource, INSTALL_SOURCE, "Installation link " + 
>+               "has expected source set");

Put the whole message on the 2nd line

>+  var md = new modalDialog.modalDialog(am.controller.window);
>+  md.start(handleInstallAddonDialog);
>+  controller.click(addToFirefox);
>+
>+  md.waitForDialog(TIMEOUT_DOWNLOAD);

Move the newline to between md.start and controller.

>+  // Fail and brake the test if the add-on is not installed 

Reword to "Fail and quit"

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

Put the whole message on the 2nd line

>+/**
>+ * Handle the modal dialog to install an addon
>+ */
>+function handleInstallAddonDialog(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"}');
>+  controller.waitFor(function(){
>+    return !installButton.getNode().disabled; 
>+  }, "Install button is enabled: got '" + !installButton.getNode().disabled 
>+  + "', expected 'true'");
>+
>+  controller.click(installButton);
>+}

This seems to be reused a lot in every test -- Henrik, do you think it's appropriate to have this in the API? If so, I can file a bug for that...

Comment 41

[u279076]

Talked to Henrik offline and we've decided to go ahead with moving the handleInstallAddonDialog() function into addons.js. I'll file a bug for this.

Comment 42

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.3 for release branch

Added patch with requested changes

Comment 43

[u279076]

Comment on attachment 545894 [details] [diff] [review]
patch v2.3 for release branch

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

I'd like to see a different style of indentation here...

addonsManager.setCategory({ 
  category:addonsManager.getCategoryById({id: "discover"})
});

Other than that, this test looks fine. Over to Geo for pre-check-in review.

Comment 44

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

Agreed on the indentation above, except that there should be a space between category: and addonsManager in the second line. 

r=me on the code with the indentation change (needed in two spots).

Re: the setting categories, though,

That construction is really clumsy, and it's a bummer we have to use it especially since it's a common operation. 

We probably need a addonsManager.setCategoryById() method so it could have just been:

addonsManager.setCategoryById("discover");

It would look like:

setCategoryById: function addonsManager_setCategoryById(categoryId) {
  this.setCategory({category: this.getCategoryById({id: categoryId})});
}

(and on another note, this is one reason why I don't like using spec objects for input. Nested functions get crapped up very quickly!)

Adding that to the shared mods and using it would clean up the test code a bit, especially if you have multiple tests doing this. I don't know that you should block on that, though.

Perhaps whimboo could comment here.

Comment 45

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.4 for release branch

Patch with requested changes. 

Geo, i agree with moving setCategory code to the API. Still, some tests are already checked in with this and we should probably be consistent. 

Furthermore, i believe we can follow up with a bug for addons.js shared module and fix this for the future. 

What do you think?
 
Thanks

Comment 46

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

(In reply to comment #45)
> Geo, i agree with moving setCategory code to the API. Still, some tests are
> already checked in with this and we should probably be consistent. 
> 
> Furthermore, i believe we can follow up with a bug for addons.js shared
> module and fix this for the future. 

See bug 671514. It has already been filed.

Comment 47

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

I'm not sure I think bug 671514 covers this problem. 

That's more pointing out that you need to move the magic strings that are the category names into addons.js as constants or similar so they can be adjusted in one place per branch (and this is true--we're pretty bad about using magic strings in our code, and it bites us in situations like this).

I'm saying that even past that, there should be a convenience function that just takes one of those category name constants and sets the category, as I suggested above in comment 44. The current way of retrieving the entity by name, then setting by entity is clumsy and getting repeated often.

Vlad,

Yeah, that's fine. We can do it later. Worse to worse, I'll make it a priority in the new API and introduce it then, though I'd prefer we still fix easy things in the current API when we find them.

Comment 48

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

Comment on attachment 546478 [details] [diff] [review]
patch v2.4 for release branch

>+function teardownModule() {
>+  delete persisted.currendAddonList;
>+  delete persisted.currentSection;
>+
>+  addonsManager.close();
>+}

Should be persisted.currentAddonList.

Aside from that, code looks fine but I'm stuck by the same process problem described in bug 669286; I couldn't r+ you today even without that typo.

Do me a favor though and touch that up for resubmit. I should be able to get things in tomorrow if all runs well.

Comment 49

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch 2.5 for release branch

Fixed nasty typo 

Geo, please run --addons <local path to your xpis> you need to have at least for addons pre-installed so that the recommended add-ons pane should be visible. 

Thanks!

Comment 50

[Maniac Vlad Florin (:vladmaniac)]

make that *four add-ons. (i hate win keyboard)

Comment 51

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

(In reply to comment #47)
> I'm not sure I think bug 671514 covers this problem. 

I already wanted to cover what you have mentioned here. I can make a comment over on bug 671514 so it is clear. Moving the internal category names to a map would require a separate method which can get/set the category. So that's what you are talking about here.

Comment 52

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

Comment on attachment 546726 [details] [diff] [review]
patch 2.5 for release branch

Vlad, same problem on this as mentioned in Bug 669286; you can't assume there are four addons installed, tests are required to do their own setup and teardown for environmental concerns.

Also, this test doesn't restart (or need to). That makes me wonder if it should be in restartTests or in a standard test folder.

The fact that it doesn't restart also makes me think the use of persisted may not be appropriate. And even if it is, I think you have to clean it up in teardown.

I have no idea why all this is coming up so late in the process when I can see you've gone back and forth with Anthony (even telling him to install four addons). 

Because I want a little bit of assurance I'm doing the right thing (and want some experienced eyes on the restart/persisted issue) I'm calling in Henrik for feedback. He can reverse my r- and correct me if I'm wrong.

Comment 53

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

So, does it mean that for this test to be executed at least one extension has to be installed locally? Or what are the exact requirements here? I follow Geo here and wonder why that came in that late. Can you please elaborate?

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

The addons are required for the recommended for you panel to be visible. we can surely use --addons here. Otherwise we will need to install four addons. It doesn't test installation of those 4 addons, those are needed to get this panel active.

Comment 55

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

Would that also work with any kind of extension? Or are the recommendations bound to extensions from AMO? If that's not the case we can install some of our local extensions we are currently working on.

Comment 56

[Maniac Vlad Florin (:vladmaniac)]

It wouldn't make any sense to bound the installed extensions to AMO. I don't think so. I think we can use our local extensions since --addons installs add-ons from the local machine.

Comment 57

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

(In reply to comment #56)
> It wouldn't make any sense to bound the installed extensions to AMO. I don't
> think so. I think we can use our local extensions since --addons installs
> add-ons from the local machine.

As we have pointed out multiple times we do not want to install any addons via the --addons option. If it was unclear with local I mean our example extensions we want to access and install via localhost.

Comment 58

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

(In reply to comment #54)
> The addons are required for the recommended for you panel to be visible. we
> can surely use --addons here. Otherwise we will need to install four addons.
> It doesn't test installation of those 4 addons, those are needed to get this
> panel active.

Vlad,

Having to install four extensions in setup is fine, if that's the minimum requirement of the test. For portability's sake, though, it's pretty important the test does it instead of assuming a human has done it.

I think the only real question is where to install from, which is what Henrik's getting at. We could, perhaps, put some in the data directory in the repo and install from there.

Comment 59

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

BTW, if you do have to install the four addons, you can likely ignore my remark re: not being a restart test as you'll have to restart for the addon install. I suspect it'll look something like:

test1.js:
  setup: install 4 addons, add data to persisted, initiate restart
  test: empty test
  teardown: empty teardown

test2.js:
  setup: empty setup
  test: recommended addons test as now
  teardown: uninstall 4 addons, delete data from persisted, initiate restart

Henrik can check me on that though.

Comment 60

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

(In reply to comment #59)
> test1.js:
>   setup: install 4 addons, add data to persisted, initiate restart
>   test: empty test
>   teardown: empty teardown

I would put the add-on installation in the test function itself and not in setupModule, because those steps are related to manual steps you would have to do in the Litmus test as well. Tracking failures in setupModule is a pain.

> test2.js:
>   setup: empty setup
>   test: recommended addons test as now
>   teardown: uninstall 4 addons, delete data from persisted, initiate restart

No uninstallation is necessary, because we are using restart tests here. The profile will be wiped-out right after.

Comment 61

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

Ah, good point re: uninstall.

Even from Litmus, some stuff gets you to the test and some stuff is the test. Part of analyzing the manual test for automation is figuring out which is which.

My preference is to keep the test tight to the state change (load discovery pane) and the desired checks (look for recommended addons).

It's not quite as important for our stuff, because what it really serves is the classic pattern of having a particular fixture state (in this case, four addons installed) and multiple test functions designed to run against that fixture. But I still like the separation.

Anyway, let's talk offline. You raise a good practical reason why we might want to compromise that, but in that case I might want to talk to you about using // SETUP and // TEST comments within the test function to differentiate or something.

Comment 62

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

Comment on attachment 546726 [details] [diff] [review]
patch 2.5 for release branch

>+  // Wait for the Recommended section to be populated with add-ons
>+  // XXX: Making usage of persisted object to store the addons list and 
>+  //      the recommended section objects. Avoiding usage of global variables
>+  controller.waitFor(function() {
>+    // Get the Recommended Add-ons section
>+    var section = discovery.getSection("recs");
>+
>+    // Get the addons list array of elements
>+    var addonList = discovery.getElements({type: "recommendedAddons_addons", 
>+                                           parent: section});
>+
>+    // Store section element in the persisted object 
>+    persisted.currentSection = section;
>+
>+    // Store addons list array in the persisted object
>+    persisted.currentAddonList = addonList; 
>+
>+    return (addonList.length !== 0);
>+  }, "The Recommended section is populated with add-ons");

No, you do not want to have all that code in the callback function. Check that the addonList has entries.

>+  var sectionDisplayStyle = persisted.currentSection.getNode().style["display"];

Why do we have to store the current section in persisted? This object is only used when we have to transfer content between test modules or across different threads, e.g. modal dialogs.

Comment 63

[Maniac Vlad Florin (:vladmaniac)]

We need to update our litmus data repo with more sample add-ons. I'll work on a fixing patch then for this test.

Comment 64

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

I have some more test addons on my own website. I will move those over to the Litmus data repository hopefully by tomorrow (depending on the network connection)

Comment 65

[Maniac Vlad Florin (:vladmaniac)]

mozqa.com has now three extensions for testing. One more needed to get this test fixed :)

Comment 66

[Maniac Vlad Florin (:vladmaniac)]

To my mind, this test needs a multipackage containing 4 add-ons to get installed. If we install 4 add-ons in iterations, the test will take long to finish and it's not going to be as stable - i can smell lots of timeout error coming. What do you guys think?

Comment 67

[u279076]

The question is, does the Discovery Pane simply parse already installed add-ons, or does it work based on individual installs? If it requires 4 separate add-on installations, this will not work.

Comment 68

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

Vlad, why does it need a multi-packaged extension? Just to reduce the timing for installing all of those?

Comment 69

[Maniac Vlad Florin (:vladmaniac)]

This is one thing, yes, but keep in mynd that we do not actually use the installed add-ons in this test. We only need them to have the "Recommended" section visible which is triggered only if the user has at least 4 add-ons installed.

Comment 70

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

To speed things up we can reduce the install dialog count down. Probably not such a bad idea for most of our tests.

Comment 71

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #70)
> To speed things up we can reduce the install dialog count down. Probably not
> such a bad idea for most of our tests.

Is there a pref we can set so the countdown is effectively 0?

Comment 72

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

Yes, but I don't know it off-hand. Querying MXR should reveal it quickly.

Comment 73

[u279076]

I spoke with Mossop on IRC. He suggests trying security.dialog_enable_delay.

Here is the only documentation I can find on this pref:
http://kb.mozillazine.org/Disable_extension_install_delay_-_Firefox

Comment 74

[u279076]

Any update on this bug, Vlad?

Comment 75

[Maniac Vlad Florin (:vladmaniac)]

Well, from the previous comments it seems that we haven't got to any conclusion for having the right patch for this one. 

Let me summarize this in one comment here

The problem: 
We need to have 4 extensions installed to get the Recommended section visible. 

Solutions: 
1. Install the 4 add-ons from mozqa.com or mozmill-tests/data folder? We don't have yet 4 extensions as stated in comment 65 

2. Suggested a multipackage installation of 4 add-ons so that we make some time saving here. Installing 4 add-ons one at a time will take a long time. - You guys tried to come up with another workaround - finished by comment 73 

With this problems, I don't know if we reached to any decision on how to proceed yet. Or have we?

Comment 76

[u279076]

I believe the proposed solution was both:
 * Install a multipackage add-on which contained 4 add-ons
 * Alter the security.dialog_enable_delay pref so the countdown was reduced

Henrik, can you give feedback on this proposal? If we simply want to speed up add-ons manager testruns is there a way we can get that pref changed more globally (ie. not on a test-by-test basis)?

Comment 77

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

We could use our new multipackaged extension and two other ones. For now please change the timer only in the test, because we shouldn't reduce the timer in all of our tests. We could take care of in the new API.

Comment 78

[u279076]

Thanks Henrik.

Vlad, please make the proposed changes in comment 77.

Comment 79

[Maniac Vlad Florin (:vladmaniac)]

Yes I will put it in my queue, thanks for clarifying guys

Comment 80

[Maniac Vlad Florin (:vladmaniac)]

Good news: i have the patch ready for this one 
Bad news: i still need the restartless extension from Alex (bug )

Comment 81

[Maniac Vlad Florin (:vladmaniac)]

multipackage.xpi contains one extension + one theme 
    * empty.xpi
    * plain.jar (theme) 

The empty.xpi add-on is also present standalone in the extensions folder, so we practically install it twice.

Comment 82

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

Why don't you use the icons.xpi and longname.xpi? There is no need to install empty.xpi again.

Comment 83

[Maniac Vlad Florin (:vladmaniac)]

icons.xpi + longname.xpi + empty.xpi = 3 add-ons 
I need 4 

the multipackage contains a theme (which does not count) and empty.xpi again

Comment 84

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

So please check our current set of Litmus tests and if we would need one more example extension. That would solve your question.

Comment 85

[Maniac Vlad Florin (:vladmaniac)]

Well we have enough add-ons under mozqa.com but not under mozmill-tests/data folder

Comment 86

[u279076]

Are the addons on mozqa.com minimalist add-ons like icons, longname, and empty? If so, just add a patch to this bug to port them to mozmill-tests. If not, please file a dependency bug to create another add-on.

Comment 87

[Maniac Vlad Florin (:vladmaniac)]

There are two more which are useful here http://mozqa.com/data/firefox/addons/extensions/

* large.xpi 489K
* restartless.xpi 157K

Is that minimalist enough to have them in a patch here?

Comment 88

[u279076]

Is restartless porting not being taken care of by bug 684867?

Comment 89

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #87)
> * large.xpi 489K
> * restartless.xpi 157K

We don't want to use both here for this test. First you have to install non-restartless extensions, and second the large.xpi is too big and we don't need it at all for our tests. As I have proposed please check if we need another extension for other existent litmus tests.

Comment 90

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #89)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #87)
> > * large.xpi 489K
> > * restartless.xpi 157K
> 
> We don't want to use both here for this test. First you have to install
> non-restartless extensions, and second the large.xpi is too big and we don't
> need it at all for our tests. As I have proposed please check if we need
> another extension for other existent litmus tests.

I've looked into the other litmus tests and it seems that we do not need another extension for other ones.

Comment 91

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #90)
> I've looked into the other litmus tests and it seems that we do not need
> another extension for other ones.

Create one for https://litmus.mozilla.org/show_test.cgi?id=15624 ?

Comment 92

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #91)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #90)
> > I've looked into the other litmus tests and it seems that we do not need
> > another extension for other ones.
> 
> Create one for https://litmus.mozilla.org/show_test.cgi?id=15624 ?

Why would we need one here? the litmus tests points out to a prerequisite which consists of installing one add-on, so I would say we don't need another test extension.

Comment 93

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

Why do you think that? First, we need a sample extension for the about window for the referenced litmus test. And secondly 4 extensions have to be installed before we can proceed with this patch here. So I do not understand the logic right now.

Comment 94

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

Or if restartless extensions will also work lets use the one we have in our repository or the one from bug 695281.

Comment 95

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #93)
> Why do you think that? First, we need a sample extension for the about
> window for the referenced litmus test. And secondly 4 extensions have to be
> installed before we can proceed with this patch here. So I do not understand
> the logic right now.

Well we can use any extension we have to address https://litmus.mozilla.org/show_test.cgi?id=15624 as the test says, "This test assumes you've installed at least one extension." - this is only one - we have three right now. 

We need four extensions to address this test only. It is the only situation

Comment 96

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #94)
> Or if restartless extensions will also work lets use the one we have in our
> repository or the one from bug 695281.

tested already - it doesn't work. i'll have a WIP patch for you immediately

Comment 97

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #96)
> (In reply to Henrik Skupin (:whimboo) from comment #94)
> > Or if restartless extensions will also work lets use the one we have in our
> > repository or the one from bug 695281.
> 
> tested already - it doesn't work. i'll have a WIP patch for you immediately

Hmm the multipackage xpi is the problem not the restartless extension - i think i can somehow make it work.

Comment 98

[Maniac Vlad Florin (:vladmaniac)]

The message i get in the recommended section is "Sorry, we couldn't find any recommendations for you. Please visit the add-ons site to find an add-on that's right for you." 

This is why i think it needs to have registered add-ons from AMO.

Comment 99

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

If that's the case we will then have to use add-ons from AMO. Should be fine since the test is in the remote test-run.

Comment 100

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #99)
> If that's the case we will then have to use add-ons from AMO. Should be fine
> since the test is in the remote test-run.

Are we 100 % positive we want to install add-ons from amo? amo is changing often and it will make the test not robust. 

If you give the go, it will be done!

Comment 101

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

Basically we have those remote tests to cover remaining areas Selenium cannot test. Also when using selectors to get elements should now be more robust as any other way we have used in the past. We should select add-ons which are mostly in our hands, e.g Nightly tester tools, mozmill crowd, QA companion, and probably ACR?

Comment 102

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.