657492 - Mozmill test for installing an add-on from "Mozilla's pick of the month!"

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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

Install the add-on featured in 'Mozilla's pick of the month!' section
https://litmus.mozilla.org/show_test.cgi?id=15369

Comment 1

[u415141]

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

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Comment 3

[u279076]

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

>+var addonName; 

Try to find a way to accomplish this without using a global. If you have to use global, call it "gAddonName".

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

Put a newline between these two lines.

>+  var panel = new elementslib.Selector(controller.window.document, "#main-feature .slider");
>+  var panelStyle = controller.window.getComputedStyle(panel.getNode(), "");
>+  var panelStyleLeft = panelStyle.getPropertyValue('left');
>+  var panelStyleLeftNumber = parseInt(panelStyleLeft.replace("-","").replace("px",""));
>+  var expectedStyle = "-" + panelStyleLeftNumber + "px";
>+
>+  while (parseInt(panelStyle.getPropertyValue('left').replace("-","").replace("px","")) < 1917) {
>+     discovery.controller.click(nextLink);
>+     controller.waitFor(function(){return (parseInt(panelStyle.getPropertyValue('left').replace("-","")
>+                                           .replace("px","")) !== panelStyleLeftNumber)},"Wait for panel - got '" 
>+                                           +(parseInt(panelStyle.getPropertyValue('left').replace("-","")
>+                                           .replace("px","")) !== panelStyleLeftNumber) + "', expected 'true'");
>+  }

I'm a bit confused with what you are trying to accomplish here; it seems overly complicated. If it can't be simplified in the test, perhaps it can be simplified by a helper in the shared module. Can you explain to me what you are trying to do here?
 
>+  // 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"}');

XPaths should be in 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 use a waitThenClick() here instead.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Anthony, in the complicated part, I need to properly get to the "Mozilla's pick of the month pane". In some builds, you need to click twice on featureNext and on other you need to click three times to get there. In order for the test to work on all branches (take beta and aurora) i need to work with styles rather than just simply do a for{} loop and discovery.controller.click();

Comment 5

[u279076]

(In reply to comment #4)
> Anthony, in the complicated part, I need to properly get to the "Mozilla's
> pick of the month pane". In some builds, you need to click twice on
> featureNext and on other you need to click three times to get there. In
> order for the test to work on all branches (take beta and aurora) i need to
> work with styles rather than just simply do a for{} loop and
> discovery.controller.click();

Thanks for explaining. This sounds like a larger issue which should be incorporated into the shared module. Would you mind filing a bug on that, providing the code example and explanation, and mark it as blocking this bug?

Thanks.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Sure, asap

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Logged https://bugzilla.mozilla.org/show_bug.cgi?id=662284

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Uploaded v1.1 with the following comments: 
 - waitThenClick() sometimes failes the test so I left it waitFor() and then controller.click(); 
 - the complex code is out, so this test works for Beta branch only 
 - global variable is out, use local declaration instead
 - Henrik advised not to make any changes in the shared module for now, so I guess the XPATH stays.

Comment 9

[u279076]

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

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

Nit: simplify the name of the file to testInstallPickOfTheMonthAddon.js

>+  for (var i = 0; i < 3; i++){
>+     controller.click(nextLink);
>+  }

For the versioning across branches, it might make sense to store '3' in a CONSTANT. That way we simply change the CONSTANT value across versions and the rest of the test remains the same.

>+  // Verify the addon is installed
>+  am.setCategory({category: am.getCategoryById({id: "extensions"})});
>+  var addonName; 
>+  var addon = am.getAddons({attribute: "name", value: addonName})[0];
>+  controller.assert(function() {
>+    return am.isAddonInstalled({addon: addon});
>+  }, "Add-on has been installed - got '" + am.isAddonInstalled({addon: addon}) + "', expected 'true'"); 

Can you break this up a bit? A newline between am.setCategory and var addonName. Another newline between var addon and controller.assert

>+function handleTriggerDialog(controller, addonName) {

Nit: can you rename this to handleInstallAddonDialog?

RE Xpaths: I agree to check it in with these XPaths for the time being since these are pretty trivial XPaths. We should revisit moving these into the shared module in a followup bug (probably as part of the API refactor).

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Uploaded v1.2 with requested changes

Comment 11

[u279076]

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

>+function teardownModule() {
>+  am.close();
>+}
>+/**
>+ * Verifies that an addon from the Mozilla Pick of the month section can be installed
>+ */

You should put a newline between the } and /**

>+function testInstallMonth() {

Please rename to testInstallPickOfTheMonthAddon (same with the file name).

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

Please wrap this into two lines.

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

Can you wrap this line too? Split the XPath with a leading '/'

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

Please wrap this onto two lines as well.

Thanks.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Comment 13

[u279076]

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

Code-wise this patch is fine. However, I'm not able to test the patch. I get an error when trying to run. Vlad, how are you testing this patch works?

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Worked fine for me. Please try this one, maybe it's just good old hg playing games again

Comment 15

[u279076]

(In reply to comment #14)
> Created attachment 538443 [details] [diff] [review] [review]
> patch v1.3
> 
> Worked fine for me. Please try this one, maybe it's just good old hg playing
> games again

How are you testing the patch? Can you please tell me the commands you are running?

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Still not working? 

The commands for applying the patch are the ones from MDN https://developer.mozilla.org/en/Mozmill_Tests 

The command for running the test in mozmill env is 
mozmill --show-all -t tests/remote/testDiscoveryPane/..

Patch applies fine to my local hg repo, no errors. and test passes. 
What branch are you on to? This needs to be tested with Beta.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4

Final patch, adding test for addon url src

Comment 18

[u279076]

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

Code-wise this is fine, but I still cannot get this to run without failing.  Patch is applied cleanly but when I run...

mozmill -b /path/to/firefox/firefox -t ./tests/remote/testDiscoveryPane/ --show-errors

I get an error about the specified tab not being found. The test opens the Get Addons pane fine but fails soon after that.

I'm running this with Firefox 5.0b5 on Linux.

I can't help but wonder if I'm doing something wrong for executing "remote" tests...

Comment 19

[u279076]

Running the following command...

mozmill -b ~/Desktop/Firefox-Beta/firefox -a ./addons/noscript-2.1.1.1.xpi -t ./mozmill-tests/tests/remote/testDiscoveryPane/testInstallPickOfMonthAddon.js --show-errors

I still get 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}}

Testing with Firefox 5.0b5 using the mozilla-beta branch on Ubuntu 11.04.

Comment 20

[Alex Lakatos[:AlexLakatos]]

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

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

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

(In reply to comment #19)
> 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}}

Anthony, you are running an outdated version of Mozmill. Please update to the latest beta.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

Updated with checkSRC = (addonSRC !== -1)

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5

Ups! small nit fixed, forgot to add a new line.

Comment 26

[u279076]

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

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 27

[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/test1.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 28

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.6

The previous patch already contained a check if the addons is installed and present in the "extensions" section list. am.isAddonInstalled() takes care of this. Thanks for pointing this out, we wouldn't want to miss anything

Comment 29

[u279076]

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

Looks good to me, test passes, over to Henrik for pre-checkin review.

Comment 30

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

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

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

You have to use a specific name for the subfolder. See as example the restart tests in the functional test-run.

>+const EXT_TIMEOUT = 25000;

This should be TIMEOUT_DOWNLOAD

>+  // Go to Mozilla's pick of the Month panel
>+  var section = discovery.getSection("main-feature");
>+  var nextLink = discovery.getElement({type: "mainFeature_nextLink", parent: section});
>+
>+  for (var i = 0; i < CLICK_COUNT; i++) {
>+    controller.click(nextLink);
>+  }

Please add an XXX note here which we can use to query for once the dependency has been fixed.

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

Move down to md.start.

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

Same as on other patches. Should be an API method. Just to be curious why do we have to check for 'homepage' here and not the discovery pane?

>+  var addonName; 
>+  var addon = am.getAddons({attribute: "name", value: addonName})[0];

First line misses the code which retrieves the add-on name.

>+function handleInstallAddonDialog(controller, addonName) {

That will not work. We do not pass a second argument to the callback. If you have to exchange the data, you should use the persisted object and clear it in teardownModule afterward. See the install multiple extensions restart test in the mozilla-1.9.2 branch.

>+  // Get the addon's name
>+  var itemList = controller.window.document.getElementById("itemList");
>+  addonName = itemList.childNodes[0].name;

Hm, we don't use it. So why can't we delete that completely?
>+  controller.waitFor(function(){

You have to add a blank before the functions brackets, because it's an anonymous one.

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

With following comments:

src="homepage" should remain the same because it checks for the src of addToFirefox link so its not discovery pane.

>+  var itemList = controller.window.document.getElementById("itemList");
>+  addonName = itemList.childNodes[0].name;
We use addonName, this shouldn't be deleted.

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

Henrik here is the Addon src check code: 

var addonUrl = addToFirefox.getNode().href;
  var addonSRC = addonUrl.indexOf("src=homepage");
  var checkSRC = (addonSRC !== -1);

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

Comment 33

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

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

>+++ b/tests/remote/restartTests/testDiscoveryPane/testInstallPickOfMonthAddon/test1.js

I don't think that you have tested this patch. It doesn't work because we do not support another level below the testDiscoveryPane directory.

>+  // Store addonName in persisted.currentAddon
>+  persisted.currentAddon = addonName;

Not sure where you get addonName from. This variable hasn't been declared anywhere in this function. This code has to be in the callback handler and in the test function itself you access persisted.currentAddon.

Comment 34

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.1

Comment 35

[Maniac Vlad Florin (:vladmaniac)]

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

Review of attachment 543361 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

Asking for feedback? from Henrik since he is in mt timestamp and we can make usage of this time before patch is getting r

Comment 37

[Maniac Vlad Florin (:vladmaniac)]

Please also keep note that this patch will not work on aurora branch. because the subsection order in the promo pane is different than in the beta and release branch.

Comment 38

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

Vlad, does that also apply to Nightly builds?

Comment 39

[Maniac Vlad Florin (:vladmaniac)]

Yes, Nightly and aurora as well.

Comment 40

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

Ok, so in that case we have to do the following for those tests:

* Add a XXX comment with the dependency bug in question to get solved first
* For default and mozilla-aurora the test has to be marked as skipped immediately

Comment 41

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

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

>+  // Go to Mozilla's pick of the Month panel
>+  // XXX: Needs for Bug 666530 to land. In the meantime we use this workaround

XXX comment look like that:

// XXX: Bug 123456
//      Bug description and why it fails

The current comment doesn't tell me anything, even not what the workaround is here.

>+  var addonSRC = discovery.getInstallSource(addToFirefox);

It's not the addon source but the installation source.

>+  var checkSRC = (addonSRC === "homepage");

homepage should be a constant at the top of the file. Also no need to use SRC in all capital letters. It's not an acronym. 

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

Sorry, if we have to go back to a former version but for debugging purposes we should do the comparison inside the assert and the message should show the real src values. Please start the message with "Installation link has source set'.

>+function handleInstallAddonDialog(controller) {
[..]
>+  // Get the addon's name
>+  var itemList = controller.window.document.getElementById("itemList");
>+  addonName = itemList.childNodes[0].name;
>+
>+  // Store addonName in persisted.currentAddon
>+  persisted.currentAddon = addonName; 

Looks good now. I have checked the page and it would be nice if the discovery pane would also list the add-on id in the ".install featureaddon" div element, which we could use to check that the real add-on has been installed. Please file a bug in AMO:Discoverypane and add a XXX comment here. I would like to see the handling as how it has been done in the other tests.

Comment 42

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.0 for beta and release branches

Comment 43

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

Comment on attachment 543390 [details] [diff] [review]
patch v3.0 for beta and release branches

>+  for (var i = 0; i < CLICK_COUNT; i++) {
>+    controller.click(nextLink);
>+  }

Missed that before but please add a sleep of at least 100ms after each click. Otherwise a click on the install button could fail.

>+function handleInstallAddonDialog(controller) {
>+  // Get the addon's name
>+  // XXX: Bug 668767
>+  //      Discovery pane should list the add-on id in the ".install featureaddon" 
>+  //      div element
>+  var itemList = controller.window.document.getElementById("itemList");
>+  addonName = itemList.childNodes[0].name;
>+
>+  // Store addonName in persisted.currentAddon
>+  persisted.currentAddon = addonName; 

Please add to the comment that we have to use persisted because of that limitation.

Otherwise looks good.

Comment 44

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.1 for beta and release branches

Comment 45

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

Comment on attachment 543401 [details] [diff] [review]
patch v3.1 for beta and release branches

>+  for (var i = 0; i < CLICK_COUNT; i++) {
>+    controller.click(nextLink);
>+    controller.sleep(100);

Please have a look at our coding styles. We always use constants and no hard-coded numbers in the middle of tests.

Comment 46

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.2 for beta and release branches

Sorry, it skipped me

Comment 47

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

Comment on attachment 543408 [details] [diff] [review]
patch v3.2 for beta and release branches

>+const TIMEOUT_DOWNLOAD = 25000;
>+const CLICK_COUNT = 3; 
>+const INSTALL_SOURCE = "discovery-promo";
>+const TIMEOUT_SLEEP = 100;

Group timeouts and the remaining consts separately. Not simply add the line at the end. Also call the new const TIMEOUT_SWITCH or something like that, which includes the reason for the sleep.

Comment 48

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.3 for beta and release branches

Comment 49

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.4 (beta, release) [checked-in]

Comment 50

[u279076]

Comment on attachment 543432 [details] [diff] [review]
patch v3.4 (beta, release) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d987aa6931d5 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/89464d47c85a (mozilla-beta)

Please follow up with a patch for Aurora and Nightly

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.4 for nightly and aurora branches

Comment 52

[Maniac Vlad Florin (:vladmaniac)]

This will fail Nightly because the addon is incompatible

Comment 53

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

We have disabled compatibility checks in Nightly builds. Why can it fail? Or what fails? Please be a bit more specific on failures in the future.

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

the addToFirefox button is grayed out in the sub section panel.

Comment 55

[Maniac Vlad Florin (:vladmaniac)]

if you guys just edited the config for Compatibility check it won't be enough for this scenario because the test cannot click on the grayed out link

Comment 56

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

Comment on attachment 543737 [details] [diff] [review]
patch v3.4 for nightly and aurora branches

In that case we have to mark this test as skipped on default. For help how to do that see the test2.js of the master password dialog under restart tests. Please add a good comment so we are aware of it with the next merge.

Comment 57

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0 for nightly builds

We skip the test on default branch because the Mozilla's pick of the month addon is incompatible with nightly builds.

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.4 for aurora branch

Comment 59

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

Comment on attachment 544462 [details] [diff] [review]
patch v1.0 for nightly builds

>+// Skip because the Mozilla's pick of the month add-on is not compatible with 
>+// Nightly builds 
>+setupModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                             " compatible with Nighlty builds";
>+teardownModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                             " compatible with Nighlty builds";

Please add the number of this bug as usual so we can check back. I think for teardown you can simply assign true.

Comment 60

[Maniac Vlad Florin (:vladmaniac)]

Which bug exactly Henrik? This one? 

I am not aware of any filed bugs for this incompatibility issue, nor have found one while checking bugzilla. 

For the moment I think we can have this patch r, then make this change before landing, when we can clarify this. 

Thanks!

Comment 61

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

Yes, this one.

Comment 62

[u279076]

Comment on attachment 544462 [details] [diff] [review]
patch v1.0 for nightly builds

Please add a reference to this bug as Henrik already pointed out.

Thanks.

Comment 63

[u279076]

Comment on attachment 544464 [details] [diff] [review]
patch v3.4 for aurora branch

This test is timing out for me with the version bump to Firefox 7.0a2 on Aurora. Please retest and resubmit your patch.

Comment 64

[Maniac Vlad Florin (:vladmaniac)]

This is something new. Anthony, can this failure be reproduced? Otherwise I am thinking seriously to extend the timeout for waitForPageLoad() in the addons.js shared module. This timeout errors occur because the d. pane is loaded lazily. 

Henrik, what would be your advise here?

Comment 65

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

The pane has to load way under 10s. If that's not the case we should file a bug.

Comment 66

[u279076]

Attachment: Screencast

Here is a screencast showing the test timing out -- I assume waiting to click the addToFirefox button. The following is the failure:

ERROR | Test Failure: {"exception": {"stack": "TimeoutError(\"Modal dialog has been found and processed\")@resource://mozmill/modules/utils.js:429\nwaitFor([object Proxy],\"Modal dialog has been found and processed\",25000,100,[object Proxy])@resource://mozmill/modules/utils.js:467\n", "message": "Modal dialog has been found and processed", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 429}}

Comment 67

[Maniac Vlad Florin (:vladmaniac)]

Thanks! Now this will indeed help, seems like a problem when handling the modal dialog, or this error was caused by the addon itself, that's why it doesn't click on addToFirefox button. it's strange we do not get it here though. 

The exact same issue was fixed by bug 668835. 

Maybe we missed something there. Thanks again

Comment 68

[Maniac Vlad Florin (:vladmaniac)]

Seen the screencast. It's obvious now that the addon is not compatible your version of aurora. If this is the case, we'll have to skip this test.

Comment 69

[u279076]

(In reply to comment #67)
> it's strange we do not get it here though. 
> 
> The exact same issue was fixed by bug 668835. 
> 
> Maybe we missed something there. Thanks again

Perhaps this is a geographic issue...does AMO present different add-ons based on IP geolocation? If so, this is something we have not considered in ANY of our Discovery Pane tests and a potential uncaught failure point.

Comment 70

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.5 for aurora branch

We'll have to skip the test for aurora branch, since we have no compatible addon for Mozilla's pick of the month subsection

Comment 71

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1 for default branch

We'll skip this test on default branch (nightly builds) because the Mozilla's pick of the month subsection addon is incompatible

Comment 72

[u279076]

Comment on attachment 545329 [details] [diff] [review]
patch v3.5 for aurora branch

>+// XXX: Bug 657492
>+//      Skip because the Mozilla's pick of the month add-on is not compatible with 
>+//      Aurora builds 
>+setupModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                             " compatible with Aurora builds";
>+teardownModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                                " compatible with Aurora builds";

nit: Can you reword the skip message -- it reads like "Mozilla Pick of the Month" is an add-on itself.

Something like:
"'Pick of the Month' add-ons are not compatible with Firefox Aurora"

Comment 73

[u279076]

Comment on attachment 545331 [details] [diff] [review]
patch v1.1 for default branch

>+// XXX: Bug 657492
>+//      Skip because the Mozilla's pick of the month add-on is not compatible with 
>+//      Nightly builds 
>+setupModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                             " compatible with Nighlty builds";
>+teardownModule.__force_skip__ = "The Mozilla's pick of the month add-on is not" + 
>+                                " compatible with Nighlty builds";

Please reword the skip message to be something like:

"'Pick of the Month' add-ons are not compatible with Firefox Nightly builds"

Comment 74

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.6 for aurora branch

Added patch with requested changes

Comment 75

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2 for default branch

Added patch for default branch with requested changes

Comment 76

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2 for default branch

Added patch for default branch with requested changes. Fixed one small nit

Comment 77

[u279076]

Comment on attachment 545887 [details] [diff] [review]
patch v1.2 for default branch

Looks fine to me -- over to Henrik for pre-check-in review.

Comment 78

[u279076]

Comment on attachment 545884 [details] [diff] [review]
patch v3.6 for aurora branch

Looks fine to me -- over to Henrik for pre-check-in review.

Comment 79

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

Comment on attachment 545884 [details] [diff] [review]
patch v3.6 for aurora branch

>+// XXX: Bug 657492
>+//      Skip because the Mozilla's pick of the month add-on is not compatible with 
>+//      Aurora builds 
>+setupModule.__force_skip__ = "'Pick of the Month' add-ons " + 
>+                             "are not compatible with Firefox Aurora";
>+teardownModule.__force_skip__ = "'Pick of the Month' add-ons " + 
>+                                "are not compatible with Firefox Aurora";

The bug number has to be the prefix of the skip message, so it will appear in the dashboard. Otherwise it looks fine.

Comment 80

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

Comment on attachment 545887 [details] [diff] [review]
patch v1.2 for default branch

>+// XXX: Bug 657492
>+//      Skip because the Mozilla's pick of the month add-on is not compatible with 
>+//      Nightly builds 
>+setupModule.__force_skip__ = "'Pick of the Month' add-ons " + 
>+                             "are not compatible with Firefox Nightly builds";
>+teardownModule.__force_skip__ = "'Pick of the Month' add-ons " + 
>+                                "are not compatible with Firefox Nightly builds";

Same as for the other patch. Also please strip 'Nightly' and 'Aurora' from the patches. It's not necessary to call this out. It's clear from the named branch the test is contained in.

Comment 81

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Added patch for default branch with requested change

Comment 82

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.7

Added patch with requested change for aurora branch

Comment 83

[Maniac Vlad Florin (:vladmaniac)]

Henrik, did you mean deleting the 'Nightly' and 'Aurora' from the skip messages?

Comment 84

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.8 for aurora branch

Added patch with requested changes

Comment 85

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4 for default branch

Added patch with requested changes

Comment 86

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

Vlad, please name the patches correctly so we can distinct between different branches. It's way complicated with the last ones now. Thanks.

Comment 87

[u279076]

> Created attachment 546760 [details] [diff] [review] [diff] [details] [review]
> patch v3.8 for aurora branch
>
> Created attachment 546762 [details] [diff] [review] [diff] [details] [review]
> patch v1.4 for default branch

Are these patches ready to be checked in? I'm a bit confused by comment 86.

Comment 88

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

Yes, those are ready. My comment only belongs to the naming of the patches on this bug.

Comment 89

[u279076]

Attachment: Patch v3.8.1 (aurora) [checked-in]

Modified the skip message:
> "'Pick of the Month' add-ons are not compatible with Firefox builds"
"'Pick of the Month' add-ons are not compatible with Firefox Aurora"

Comment 90

[u279076]

Comment on attachment 554906 [details] [diff] [review]
Patch v3.8.1 (aurora) [checked-in]

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

Comment 91

[u279076]

Attachment: Patch v1.4.1 (default) [checked-in]

Reworded skip message to read "Firefox Nightly" instead of "Firefox builds"

Comment 92

[u279076]

Comment on attachment 554910 [details] [diff] [review]
Patch v1.4.1 (default) [checked-in]

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

Comment 93

[u279076]

I believe this can now be called FIXED. Please reopen if that is incorrect.

Comment 94

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #89)
> Modified the skip message:
> > "'Pick of the Month' add-ons are not compatible with Firefox builds"
> "'Pick of the Month' add-ons are not compatible with Firefox Aurora"

Anthony, please do not do this. It was intended to have 'Firefox builds' in there. A change like you did will cause us a major trouble when we merge branches. We do not want to have to update each individual test with the new name. That would be a major pain. Can you please fix that? If you have a better wording feel free to propose one but it should be version independent.

Comment 95

[u279076]

I can submit a revision patch with the phrasing:
"'Pick of the Month' add-ons are only compatible with Release and Beta builds"

Does this sound okay?

Comment 96

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

Well, in this case it should be fine. That makes it clear for a merge that those skipped lines should not be merged to the new branch. For me it sounds way better.

Comment 97

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

Agreed. Also gives us a nice sanity check as to whether it should be skipped in a given branch (i.e. if it's release, it's obviously a mistake that it's skipped).

Comment 98

[u279076]

Attachment: Patch v3.8.2 (aurora) [checked-in]

Revise wording of SKIP message.

Comment 99

[u279076]

Comment on attachment 555022 [details] [diff] [review]
Patch v3.8.2 (aurora) [checked-in]

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

Comment 100

[u279076]

Attachment: Patch v1.4.2 (default) [checked-in]

Revised SKIP message

Comment 101

[u279076]

Comment on attachment 555024 [details] [diff] [review]
Patch v1.4.2 (default) [checked-in]

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

Comment 102

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

Thanks Anthony!

Comment 103

[u415141]

Anthony Hughes changed story state to delivered in Pivotal Tracker

Comment 104

[u415141]

Anthony Hughes changed story state to accepted in Pivotal Tracker

Comment 105

[u415141]

Anthony Hughes changed story state to accepted in Pivotal Tracker