Closed
Bug 657492
Opened 13 years ago
Closed 13 years ago
Mozmill test for installing an add-on from "Mozilla's pick of the month!"
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: u279076, Assigned: vladmaniac)
References
()
Details
(Whiteboard: [mozmill-restart][aom-discovery] )
Attachments
(6 files, 27 obsolete files)
(deleted),
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/x-shockwave-flash
|
Details | |
(deleted),
patch
|
u279076
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u279076
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u279076
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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 | ||
Updated•13 years ago
|
Assignee: nobody → vlad.maniac
Assignee | ||
Comment 2•13 years ago
|
||
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-
Updated•13 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
Sure, asap
Assignee | ||
Comment 8•13 years ago
|
||
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-
Assignee | ||
Comment 10•13 years ago
|
||
Uploaded v1.2 with requested changes
Attachment #537745 -
Attachment is obsolete: true
Attachment #537990 -
Flags: review?(anthony.s.hughes)
Reporter | ||
Comment 11•13 years ago
|
||
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-
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #537990 -
Attachment is obsolete: true
Attachment #538195 -
Flags: review?(anthony.s.hughes)
Reporter | ||
Comment 13•13 years ago
|
||
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)
Assignee | ||
Comment 14•13 years ago
|
||
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)
Reporter | ||
Comment 15•13 years ago
|
||
(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?
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
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)
Reporter | ||
Comment 18•13 years ago
|
||
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-
Reporter | ||
Comment 19•13 years ago
|
||
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•13 years ago
|
||
Here is the screencast
http://dl.dropbox.com/u/22633148/Mozmill/screencast657492.ogv
Reporter | ||
Comment 21•13 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.
Comment 22•13 years ago
|
||
(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.
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #538840 -
Attachment is obsolete: true
Attachment #540729 -
Flags: review?(anthony.s.hughes)
Assignee | ||
Comment 24•13 years ago
|
||
Updated with checkSRC = (addonSRC !== -1)
Assignee | ||
Comment 25•13 years ago
|
||
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)
Reporter | ||
Comment 26•13 years ago
|
||
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-
Reporter | ||
Comment 27•13 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/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.
Assignee | ||
Comment 28•13 years ago
|
||
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)
Reporter | ||
Comment 29•13 years ago
|
||
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 30•13 years ago
|
||
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-
Assignee | ||
Comment 31•13 years ago
|
||
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)
Assignee | ||
Comment 32•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Comment 34•13 years ago
|
||
Attachment #543361 -
Flags: review?(anthony.s.hughes)
Assignee | ||
Comment 35•13 years ago
|
||
Comment on attachment 543361 [details] [diff] [review]
patch v2.1
Review of attachment 543361 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543361 -
Flags: feedback?(hskupin)
Assignee | ||
Comment 36•13 years ago
|
||
Asking for feedback? from Henrik since he is in mt timestamp and we can make usage of this time before patch is getting r
Assignee | ||
Comment 37•13 years ago
|
||
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•13 years ago
|
||
Vlad, does that also apply to Nightly builds?
Assignee | ||
Comment 39•13 years ago
|
||
Yes, Nightly and aurora as well.
Comment 40•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Comment 42•13 years ago
|
||
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 43•13 years ago
|
||
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-
Assignee | ||
Comment 44•13 years ago
|
||
Attachment #543390 -
Attachment is obsolete: true
Attachment #543401 -
Flags: review?(hskupin)
Comment 45•13 years ago
|
||
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-
Assignee | ||
Comment 46•13 years ago
|
||
Sorry, it skipped me
Attachment #543401 -
Attachment is obsolete: true
Attachment #543408 -
Flags: review?(hskupin)
Comment 47•13 years ago
|
||
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-
Assignee | ||
Comment 48•13 years ago
|
||
Attachment #543408 -
Attachment is obsolete: true
Attachment #543422 -
Flags: review?(hskupin)
Assignee | ||
Comment 49•13 years ago
|
||
Attachment #543422 -
Attachment is obsolete: true
Attachment #543422 -
Flags: review?(hskupin)
Attachment #543432 -
Flags: review?(hskupin)
Updated•13 years ago
|
Attachment #543432 -
Attachment description: patch v3.4 for beta and release branches → patch v3.4 (beta, release)
Attachment #543432 -
Flags: review?(hskupin) → review+
Reporter | ||
Comment 50•13 years ago
|
||
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]
Assignee | ||
Comment 51•13 years ago
|
||
Attachment #543737 -
Flags: review?(hskupin)
Assignee | ||
Comment 52•13 years ago
|
||
This will fail Nightly because the addon is incompatible
Comment 53•13 years ago
|
||
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.
Assignee | ||
Comment 54•13 years ago
|
||
the addToFirefox button is grayed out in the sub section panel.
Assignee | ||
Comment 55•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Comment 57•13 years ago
|
||
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)
Assignee | ||
Comment 58•13 years ago
|
||
Attachment #543737 -
Attachment is obsolete: true
Attachment #544464 -
Flags: review?(anthony.s.hughes)
Attachment #544464 -
Flags: feedback?(hskupin)
Comment 59•13 years ago
|
||
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+
Assignee | ||
Comment 60•13 years ago
|
||
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•13 years ago
|
||
Yes, this one.
Updated•13 years ago
|
Attachment #544464 -
Flags: feedback?(hskupin) → feedback+
Reporter | ||
Comment 62•13 years ago
|
||
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-
Reporter | ||
Comment 63•13 years ago
|
||
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-
Assignee | ||
Comment 64•13 years ago
|
||
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•13 years ago
|
||
The pane has to load way under 10s. If that's not the case we should file a bug.
Reporter | ||
Comment 66•13 years ago
|
||
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}}
Assignee | ||
Comment 67•13 years ago
|
||
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
Assignee | ||
Comment 68•13 years ago
|
||
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.
Reporter | ||
Comment 69•13 years ago
|
||
(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.
Assignee | ||
Comment 70•13 years ago
|
||
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)
Assignee | ||
Comment 71•13 years ago
|
||
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)
Reporter | ||
Comment 72•13 years ago
|
||
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-
Reporter | ||
Comment 73•13 years ago
|
||
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-
Assignee | ||
Comment 74•13 years ago
|
||
Added patch with requested changes
Attachment #545329 -
Attachment is obsolete: true
Attachment #545884 -
Flags: review?(anthony.s.hughes)
Assignee | ||
Comment 75•13 years ago
|
||
Added patch for default branch with requested changes
Attachment #545331 -
Attachment is obsolete: true
Attachment #545886 -
Flags: review?(anthony.s.hughes)
Assignee | ||
Comment 76•13 years ago
|
||
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)
Reporter | ||
Comment 77•13 years ago
|
||
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+
Reporter | ||
Comment 78•13 years ago
|
||
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 79•13 years ago
|
||
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 80•13 years ago
|
||
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-
Assignee | ||
Comment 81•13 years ago
|
||
Added patch for default branch with requested change
Attachment #545887 -
Attachment is obsolete: true
Attachment #546755 -
Flags: review?(hskupin)
Assignee | ||
Comment 82•13 years ago
|
||
Added patch with requested change for aurora branch
Attachment #546756 -
Flags: review?(hskupin)
Assignee | ||
Comment 83•13 years ago
|
||
Henrik, did you mean deleting the 'Nightly' and 'Aurora' from the skip messages?
Assignee | ||
Comment 84•13 years ago
|
||
Added patch with requested changes
Attachment #546756 -
Attachment is obsolete: true
Attachment #546756 -
Flags: review?(hskupin)
Attachment #546760 -
Flags: review?(hskupin)
Assignee | ||
Comment 85•13 years ago
|
||
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)
Comment 86•13 years ago
|
||
Vlad, please name the patches correctly so we can distinct between different branches. It's way complicated with the last ones now. Thanks.
Updated•13 years ago
|
Attachment #546760 -
Flags: review?(hskupin) → review+
Updated•13 years ago
|
Attachment #546762 -
Flags: review?(hskupin) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•13 years ago
|
Attachment #546762 -
Attachment description: patch v1.4 → patch v1.4 for default branch
Attachment #546762 -
Attachment filename: patch_file_3 → patch_file_nightly
Assignee | ||
Updated•13 years ago
|
Attachment #546760 -
Attachment description: patch v3.8 → patch v3.8 for aurora branch
Attachment #546760 -
Attachment filename: patch_file_3 → patch_file_aurora
Reporter | ||
Comment 87•13 years ago
|
||
> 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•13 years ago
|
||
Yes, those are ready. My comment only belongs to the naming of the patches on this bug.
Reporter | ||
Comment 89•13 years ago
|
||
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+
Reporter | ||
Comment 90•13 years ago
|
||
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]
Reporter | ||
Comment 91•13 years ago
|
||
Reworded skip message to read "Firefox Nightly" instead of "Firefox builds"
Attachment #546762 -
Attachment is obsolete: true
Reporter | ||
Comment 92•13 years ago
|
||
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+
Reporter | ||
Comment 93•13 years ago
|
||
I believe this can now be called FIXED. Please reopen if that is incorrect.
Comment 94•13 years ago
|
||
(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 → ---
Reporter | ||
Comment 95•13 years ago
|
||
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•13 years ago
|
||
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).
Reporter | ||
Comment 98•13 years ago
|
||
Revise wording of SKIP message.
Attachment #555022 -
Flags: review+
Reporter | ||
Comment 99•13 years ago
|
||
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]
Attachment #555022 -
Attachment description: Patch v1.4.2 (aurora) [checked-in] → Patch v3.8.2 (aurora) [checked-in]
Reporter | ||
Comment 101•13 years ago
|
||
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: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 102•13 years ago
|
||
Thanks Anthony!
Comment 103•13 years ago
|
||
Anthony Hughes changed story state to delivered in Pivotal Tracker
Comment 104•13 years ago
|
||
Anthony Hughes changed story state to accepted in Pivotal Tracker
Comment 105•13 years ago
|
||
Anthony Hughes changed story state to accepted in Pivotal Tracker
Whiteboard: [aom-discovery] → [mozmill-remote][aom-discovery]
Whiteboard: [mozmill-remote][aom-discovery] → [mozmill-restart][aom-discovery]
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•