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

NEW
Unassigned

Status

Mozilla QA
Mozmill Tests
7 years ago
5 years ago

People

(Reporter: ashughes, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-restart][aom-discovery] , URL)

Attachments

(1 attachment, 13 obsolete attachments)

5.88 KB, patch
geo
: review-
whimboo
: feedback-
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13610017
Assignee: nobody → vlad.maniac
Status: NEW → ASSIGNED
Created attachment 537131 [details] [diff] [review]
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?
Attachment #537131 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 3

7 years ago
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.
Attachment #537131 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 4

7 years ago
CCing Krupa and Dave Burns on this bug. Is this test sufficiently covered by Selenium or is there value in having a Mozmill test?
This test requires installation so I think i can ask for review back, based on webQA response in the e-mail
Created attachment 537752 [details] [diff] [review]
patch v1.1

Uploaded 1.1 with changes to fit style guide
Attachment #537131 - Attachment is obsolete: true
Attachment #537752 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 7

7 years ago
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
Attachment #537752 - Flags: review?(anthony.s.hughes) → review-
Created attachment 537985 [details] [diff] [review]
patch v1.2

Uploaded new version with all requested changes.
Attachment #537752 - Attachment is obsolete: true
Attachment #537985 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 9

7 years ago
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
Attachment #537985 - Flags: review?(anthony.s.hughes) → review-
Created attachment 538212 [details] [diff] [review]
patch v1.3
Attachment #537985 - Attachment is obsolete: true
Attachment #538212 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 11

7 years ago
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"}');
Attachment #538212 - Flags: review?(anthony.s.hughes) → review-
Created attachment 538452 [details] [diff] [review]
patch v1.4

Done
Attachment #538212 - Attachment is obsolete: true
Attachment #538452 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 13

7 years ago
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.
Attachment #538452 - Flags: review?(anthony.s.hughes) → review-
Will do, in the editor it looks fine..it's just how the patch was created..makes no sense
Created attachment 538854 [details] [diff] [review]
patch v1.5

Patch including test for checking the src attribute of the addon link
Attachment #538452 - Attachment is obsolete: true
Attachment #538854 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 16

7 years ago
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.
Attachment #538854 - Flags: review?(anthony.s.hughes) → review-
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?
(Reporter)

Comment 18

7 years ago
(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.
Here is the screencast 
http://dl.dropbox.com/u/22633148/Mozmill/screencast658365.ogv
(Reporter)

Comment 20

7 years ago
Seems like my Mozmill version was out of date -- please make sure you are using Mozmill 1.5.4b3.
(Reporter)

Comment 21

7 years ago
>  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.
Created attachment 540740 [details] [diff] [review]
patch v1.6
Attachment #538854 - Attachment is obsolete: true
Attachment #540740 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 23

7 years ago
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.
Attachment #540740 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Updated

7 years ago
Depends on: 666245
(Reporter)

Comment 24

7 years ago
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.
Created attachment 542749 [details] [diff] [review]
patch v1.7

This patch also tests if the addon is installed and present in the addons list
Attachment #540740 - Attachment is obsolete: true
Attachment #542749 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 26

7 years ago
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.
Attachment #542749 - Flags: review?(anthony.s.hughes) → review-
You are right. It fails with this error 1 out of 10 times when running the test
Depends on: 668489
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.
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.
Created attachment 543747 [details] [diff] [review]
patch v2.0
Attachment #542749 - Attachment is obsolete: true
Attachment #543747 - Flags: review?(hskupin)
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 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.
Attachment #543747 - Flags: review?(hskupin) → review+
Created attachment 544443 [details] [diff] [review]
patch v2.1
Attachment #544443 - Flags: review?(anthony.s.hughes)
Attachment #544443 - Flags: feedback?(hskupin)
(Reporter)

Comment 34

7 years ago
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.
Attachment #544443 - Flags: review?(anthony.s.hughes) → review-
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 we are to keep the old assert, then this patch is eligible for r.
Attachment #544443 - Flags: feedback?(hskupin) → feedback+
(Reporter)

Comment 37

7 years ago
(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.
(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()
Created attachment 545357 [details] [diff] [review]
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!
Attachment #543747 - Attachment is obsolete: true
Attachment #544443 - Attachment is obsolete: true
Attachment #545357 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 40

7 years ago
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...
Attachment #545357 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 41

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

Updated

7 years ago
Depends on: 671059
Created attachment 545894 [details] [diff] [review]
patch v2.3 for release branch

Added patch with requested changes
Attachment #545357 - Attachment is obsolete: true
Attachment #545894 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 43

7 years ago
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.
Attachment #545894 - Flags: review?(gmealer)
Attachment #545894 - Flags: review?(anthony.s.hughes)
Attachment #545894 - Flags: review+
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.
Attachment #545894 - Flags: review?(gmealer) → review+
Created attachment 546478 [details] [diff] [review]
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
Attachment #545894 - Attachment is obsolete: true
Attachment #546478 - Flags: review?(gmealer)
(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.
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 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.
Attachment #546478 - Flags: review?(gmealer) → review-
Created attachment 546726 [details] [diff] [review]
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!
Attachment #546478 - Attachment is obsolete: true
Attachment #546726 - Flags: review?(gmealer)
make that *four add-ons. (i hate win keyboard)
(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 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.
Attachment #546726 - Flags: review?(gmealer)
Attachment #546726 - Flags: review-
Attachment #546726 - Flags: feedback?(hskupin)
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?
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.
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.
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.
(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.
(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.
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.
(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.
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 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.
Attachment #546726 - Flags: feedback?(hskupin) → feedback-
We need to update our litmus data repo with more sample add-ons. I'll work on a fixing patch then for this test.
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)
mozqa.com has now three extensions for testing. One more needed to get this test fixed :)
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?
(Reporter)

Comment 67

6 years ago
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.
Vlad, why does it need a multi-packaged extension? Just to reduce the timing for installing all of those?
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.
To speed things up we can reduce the install dialog count down. Probably not such a bad idea for most of our tests.
(Reporter)

Comment 71

6 years ago
(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?
Yes, but I don't know it off-hand. Querying MXR should reveal it quickly.
(Reporter)

Comment 73

6 years ago
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
(Reporter)

Comment 74

6 years ago
Any update on this bug, Vlad?
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?
(Reporter)

Comment 76

6 years ago
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)?
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.
(Reporter)

Comment 78

6 years ago
Thanks Henrik.

Vlad, please make the proposed changes in comment 77.
Yes I will put it in my queue, thanks for clarifying guys
Good news: i have the patch ready for this one 
Bad news: i still need the restartless extension from Alex (bug )
Depends on: 685515
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.
Why don't you use the icons.xpi and longname.xpi? There is no need to install empty.xpi again.
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
So please check our current set of Litmus tests and if we would need one more example extension. That would solve your question.
Well we have enough add-ons under mozqa.com but not under mozmill-tests/data folder
(Reporter)

Comment 86

6 years ago
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.
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?
(Reporter)

Comment 88

6 years ago
Is restartless porting not being taken care of by bug 684867?
(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.
(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.
(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 ?
(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.
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.
Or if restartless extensions will also work lets use the one we have in our repository or the one from bug 695281.
(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
(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
(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.
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.
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.
(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!
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?
(Reporter)

Updated

6 years ago
Whiteboard: [aom-discovery] → [mozmill-remote][aom-discovery]
(Reporter)

Updated

6 years ago
Whiteboard: [mozmill-remote][aom-discovery] → [mozmill-restart][aom-discovery]
Assignee: vlad.mozbugs → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.