Closed Bug 657492 Opened 10 years ago Closed 9 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: u279076, Assigned: vladmaniac)

References

()

Details

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

Attachments

(6 files, 27 obsolete files)

5.88 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
3.55 MB, application/x-shockwave-flash
Details
5.24 KB, patch
u279076
: review+
Details | Diff | Splinter Review
5.21 KB, patch
u279076
: review+
Details | Diff | Splinter Review
1.37 KB, patch
u279076
: review+
Details | Diff | Splinter Review
1.23 KB, patch
u279076
: review+
Details | Diff | Splinter Review
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
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13465885
Assignee: nobody → vlad.maniac
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #537101 - Flags: review?(anthony.s.hughes)
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.
Attachment #537101 - Flags: review?(anthony.s.hughes) → review-
Status: NEW → ASSIGNED
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();
(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.
Sure, asap
Depends on: 662284
Depends on: 662284
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #537101 - Attachment is obsolete: true
Attachment #537745 - Flags: review?(anthony.s.hughes)
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).
Attachment #537745 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v1.2 (obsolete) — Splinter Review
Uploaded v1.2 with requested changes
Attachment #537745 - Attachment is obsolete: true
Attachment #537990 - Flags: review?(anthony.s.hughes)
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.
Attachment #537990 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v1.3 (obsolete) — Splinter Review
Attachment #537990 - Attachment is obsolete: true
Attachment #538195 - Flags: review?(anthony.s.hughes)
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?
Attachment #538195 - Flags: review?(anthony.s.hughes)
Attached patch patch v1.3 (obsolete) — Splinter Review
Worked fine for me. Please try this one, maybe it's just good old hg playing games again
Attachment #538195 - Attachment is obsolete: true
Attachment #538443 - Flags: review?(anthony.s.hughes)
(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?
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.
Attached patch patch v1.4 (obsolete) — Splinter Review
Final patch, adding test for addon url src
Attachment #538443 - Attachment is obsolete: true
Attachment #538443 - Flags: review?(anthony.s.hughes)
Attachment #538840 - Flags: review?(anthony.s.hughes)
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...
Attachment #538840 - Flags: review?(anthony.s.hughes) → review-
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.
>  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.
(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.
Attached patch patch v1.5 (obsolete) — Splinter Review
Attachment #538840 - Attachment is obsolete: true
Attachment #540729 - Flags: review?(anthony.s.hughes)
Updated with checkSRC = (addonSRC !== -1)
Attached patch patch v1.5 (obsolete) — Splinter Review
Ups! small nit fixed, forgot to add a new line.
Attachment #540729 - Attachment is obsolete: true
Attachment #540729 - Flags: review?(anthony.s.hughes)
Attachment #540731 - Flags: review?(anthony.s.hughes)
Depends on: 666530
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.
Attachment #540731 - Flags: review?(anthony.s.hughes) → review-
Depends on: 666245
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.
Attached patch patch v1.6 (obsolete) — Splinter Review
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
Attachment #542741 - Flags: review?(anthony.s.hughes)
Comment on attachment 542741 [details] [diff] [review]
patch v1.6

Looks good to me, test passes, over to Henrik for pre-checkin review.
Attachment #542741 - Flags: review?(hskupin)
Attachment #542741 - Flags: review?(anthony.s.hughes)
Attachment #542741 - Flags: review+
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.
Attachment #542741 - Flags: review?(hskupin) → review-
Attached patch patch v2.0 (obsolete) — Splinter Review
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.
Attachment #543107 - Flags: feedback?(hskupin)
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 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.
Attachment #543107 - Flags: feedback?(hskupin) → feedback-
Depends on: 668489
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #543361 - Flags: review?(anthony.s.hughes)
Comment on attachment 543361 [details] [diff] [review]
patch v2.1

Review of attachment 543361 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543361 - Flags: feedback?(hskupin)
Asking for feedback? from Henrik since he is in mt timestamp and we can make usage of this time before patch is getting r
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.
Vlad, does that also apply to Nightly builds?
Yes, Nightly and aurora as well.
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 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.
Attachment #543361 - Flags: review?(anthony.s.hughes)
Attachment #543361 - Flags: feedback?(hskupin)
Attachment #543361 - Flags: feedback-
Depends on: 668767
Attachment #540731 - Attachment is obsolete: true
Attachment #542741 - Attachment is obsolete: true
Attachment #543107 - Attachment is obsolete: true
Attachment #543361 - Attachment is obsolete: true
Attachment #543390 - Flags: review?(hskupin)
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.
Attachment #543390 - Flags: review?(hskupin) → review-
Attachment #543390 - Attachment is obsolete: true
Attachment #543401 - Flags: review?(hskupin)
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.
Attachment #543401 - Flags: review?(hskupin) → review-
Sorry, it skipped me
Attachment #543401 - Attachment is obsolete: true
Attachment #543408 - Flags: review?(hskupin)
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.
Attachment #543408 - Flags: review?(hskupin) → review-
Attachment #543408 - Attachment is obsolete: true
Attachment #543422 - Flags: review?(hskupin)
Attachment #543422 - Attachment is obsolete: true
Attachment #543422 - Flags: review?(hskupin)
Attachment #543432 - Flags: review?(hskupin)
Attachment #543432 - Attachment description: patch v3.4 for beta and release branches → patch v3.4 (beta, release)
Attachment #543432 - Flags: review?(hskupin) → review+
Depends on: 668835
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
Attachment #543432 - Attachment description: patch v3.4 (beta, release) → patch v3.4 (beta, release) [checked-in]
Attachment #543737 - Flags: review?(hskupin)
This will fail Nightly because the addon is incompatible
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.
the addToFirefox button is grayed out in the sub section panel.
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 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.
Attachment #543737 - Flags: review?(hskupin) → review-
Attached patch patch v1.0 for nightly builds (obsolete) — Splinter Review
We skip the test on default branch because the Mozilla's pick of the month addon is incompatible with nightly builds.
Attachment #544462 - Flags: review?(anthony.s.hughes)
Attachment #544462 - Flags: feedback?(hskupin)
Attached patch patch v3.4 for aurora branch (obsolete) — Splinter Review
Attachment #543737 - Attachment is obsolete: true
Attachment #544464 - Flags: review?(anthony.s.hughes)
Attachment #544464 - Flags: feedback?(hskupin)
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.
Attachment #544462 - Flags: feedback?(hskupin) → feedback+
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!
Yes, this one.
Attachment #544464 - Flags: feedback?(hskupin) → feedback+
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.
Attachment #544462 - Flags: review?(anthony.s.hughes) → review-
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.
Attachment #544464 - Flags: review?(anthony.s.hughes) → review-
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?
The pane has to load way under 10s. If that's not the case we should file a bug.
Attached file 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}}
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
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.
(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.
Attached patch patch v3.5 for aurora branch (obsolete) — Splinter Review
We'll have to skip the test for aurora branch, since we have no compatible addon for Mozilla's pick of the month subsection
Attachment #544464 - Attachment is obsolete: true
Attachment #545329 - Flags: review?(anthony.s.hughes)
Attached patch patch v1.1 for default branch (obsolete) — Splinter Review
We'll skip this test on default branch (nightly builds) because the Mozilla's pick of the month subsection addon is incompatible
Attachment #544462 - Attachment is obsolete: true
Attachment #545331 - Flags: review?(anthony.s.hughes)
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"
Attachment #545329 - Flags: review?(anthony.s.hughes) → review-
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"
Attachment #545331 - Flags: review?(anthony.s.hughes) → review-
Depends on: 671059
Attached patch patch v3.6 for aurora branch (obsolete) — Splinter Review
Added patch with requested changes
Attachment #545329 - Attachment is obsolete: true
Attachment #545884 - Flags: review?(anthony.s.hughes)
Attached patch patch v1.2 for default branch (obsolete) — Splinter Review
Added patch for default branch with requested changes
Attachment #545331 - Attachment is obsolete: true
Attachment #545886 - Flags: review?(anthony.s.hughes)
Attached patch patch v1.2 for default branch (obsolete) — Splinter Review
Added patch for default branch with requested changes. Fixed one small nit
Attachment #545886 - Attachment is obsolete: true
Attachment #545886 - Flags: review?(anthony.s.hughes)
Attachment #545887 - Flags: review?(anthony.s.hughes)
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.
Attachment #545887 - Flags: review?(hskupin)
Attachment #545887 - Flags: review?(anthony.s.hughes)
Attachment #545887 - Flags: review+
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.
Attachment #545884 - Flags: review?(hskupin)
Attachment #545884 - Flags: review?(anthony.s.hughes)
Attachment #545884 - Flags: review+
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.
Attachment #545884 - Flags: review?(hskupin) → review-
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.
Attachment #545887 - Flags: review?(hskupin) → review-
Attached patch patch v1.3 (obsolete) — Splinter Review
Added patch for default branch with requested change
Attachment #545887 - Attachment is obsolete: true
Attachment #546755 - Flags: review?(hskupin)
Attached patch patch v3.7 (obsolete) — Splinter Review
Added patch with requested change for aurora branch
Attachment #546756 - Flags: review?(hskupin)
Henrik, did you mean deleting the 'Nightly' and 'Aurora' from the skip messages?
Attached patch patch v3.8 for aurora branch (obsolete) — Splinter Review
Added patch with requested changes
Attachment #546756 - Attachment is obsolete: true
Attachment #546756 - Flags: review?(hskupin)
Attachment #546760 - Flags: review?(hskupin)
Attached patch patch v1.4 for default branch (obsolete) — Splinter Review
Added patch with requested changes
Attachment #545884 - Attachment is obsolete: true
Attachment #546755 - Attachment is obsolete: true
Attachment #546755 - Flags: review?(hskupin)
Attachment #546762 - Flags: review?(hskupin)
Vlad, please name the patches correctly so we can distinct between different branches. It's way complicated with the last ones now. Thanks.
Attachment #546760 - Flags: review?(hskupin) → review+
Attachment #546762 - Flags: review?(hskupin) → review+
Attachment #546762 - Attachment description: patch v1.4 → patch v1.4 for default branch
Attachment #546762 - Attachment filename: patch_file_3 → patch_file_nightly
Attachment #546760 - Attachment description: patch v3.8 → patch v3.8 for aurora branch
Attachment #546760 - Attachment filename: patch_file_3 → patch_file_aurora
> 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.
Yes, those are ready. My comment only belongs to the naming of the patches on this bug.
Depends on: 679330
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"
Attachment #546760 - Attachment is obsolete: true
Attachment #554906 - Flags: review+
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)
Attachment #554906 - Attachment description: Patch v3.8.1 (aurora) → Patch v3.8.1 (aurora) [checked-in]
Reworded skip message to read "Firefox Nightly" instead of "Firefox builds"
Attachment #546762 - Attachment is obsolete: true
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)
Attachment #554910 - Attachment description: Patch v1.4.1 (default) → Patch v1.4.1 (default) [checked-in]
Attachment #554910 - Flags: review+
I believe this can now be called FIXED. Please reopen if that is incorrect.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
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.
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).
Revise wording of SKIP message.
Attachment #555022 - Flags: review+
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)
Attachment #555022 - Attachment description: Patch v1.4.2 (aurora) → Patch v1.4.2 (aurora) [checked-in]
Revised SKIP message
Attachment #555024 - Flags: review+
Attachment #555022 - Attachment description: Patch v1.4.2 (aurora) [checked-in] → Patch v3.8.2 (aurora) [checked-in]
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)
Attachment #555024 - Attachment description: Patch v1.4.2 (default) → Patch v1.4.2 (default) [checked-in]
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Anthony Hughes changed story state to delivered in Pivotal Tracker
Anthony Hughes changed story state to accepted in Pivotal Tracker
Anthony Hughes changed story state to accepted in Pivotal Tracker
Blocks: 688146
No longer blocks: 688146
Whiteboard: [aom-discovery] → [mozmill-remote][aom-discovery]
Whiteboard: [mozmill-remote][aom-discovery] → [mozmill-restart][aom-discovery]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.