Tracking creation of new mozmill test for uninstalling an extension. Litmus test: https://litmus.mozilla.org/show_test.cgi?id=15629

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

Some discussion about scope has come up on IRC in regards to this test. https://litmus.mozilla.org/show_test.cgi?id=15629 This test, in my opinion, should be split into two. It's really testing two very unique browser behaviours: 1) Install then Uninstall an Extension 2) Install then Disabled then Uninstall an Extension Henrik, can we split this into two Litmus tests?

uploaded WIP for testing remove enabled extension Is this approach correct? I'm having an issue with getting all add-on IDs from an addon list. There are some solutions, but need to find the most elegant one. Thanks!

Comment on attachment 545950 [details] [diff] [review] WIP v1.0 >+ var notifyRemoved = new elementslib.Selector(controller.window.document, >+ ".addon-view[pending='uninstall']"); I've asked Alex to file a bug to get all Selectors moved into their respective shared module. Please add the Selectors used in all your open tests to this bug. >+ for (var i = 0; i < addonsList.length; i++) { >+ var addonIds = addonsList[i].getNode().getAttribute("value"); >+ expect.notEqual(addonIds, ADDON_ID, >+ "The removed addon is no longer present in the extensions pane list"); >+ } I think this is probably the best way to handle checking if an add-on is indeed removed. We may want to move this into a helper method in the shared module though. Just a side note, you can use array.forEach() instead of a for loop to simplify the code... addonsList.forEach( function(element) { var addonID = element.getNode().value; expect.notEqual(addonID, ADDON_ID, "The addon has been removed"); }

>+ for (var i = 0; i < addonsList.length; i++) { >+ var addonIds = addonsList[i].getNode().getAttribute("value"); >+ expect.notEqual(addonIds, ADDON_ID, >+ "The removed addon is no longer present in the extensions pane list"); >+ } I believe the shared module will not need this, only the removed notification element, this is just not to have elementslib.Selector in the tests, this can be get with nodeCollector in the API. As long as for the approach, modified it to get proper messages in the log. It looks even better now. Fixed patch will be up when we get this api situations resolved

(In reply to comment #2) > https://litmus.mozilla.org/show_test.cgi?id=15629 > This test, in my opinion, should be split into two. It's really testing two > very unique browser behaviours: > > 1) Install then Uninstall an Extension > 2) Install then Disabled then Uninstall an Extension > > Henrik, can we split this into two Litmus tests? I wouldn't split this test up into two tests. The difference is only the enabled state, which the test would be immediately be able to set when installing the add-on (probably back-end call). Otherwise it's just another restart we would have to perform.

Merged the two scenarios into one test, as Henrik advised. Please test this against beta or release branch. Thanks!

Comment on attachment 546551 [details] [diff] [review] patch v1.0 for release and beta +function testEnableExtension() { ... + // Pass if the add-on list is empty + if (!addonsList.length) { + expect.pass("There are no add-ons in the extensions pane list"); + } Don't use expect.pass() here. There are almost no situations where pass()/fail() is the right way to go, and if any other way works without having to do serious code gymnastics you should use it instead. This is pretty explicit on the docs page at: https://developer.mozilla.org/en/Mozmill_Tests#Logging_Test_Results Also, if you mean to be doing a numeric comparison, you should do a numeric comparison. There are times to treat numbers as true/false values, but don't do it just for brevity, especially in an assertion. A tip here: code doesn't just work, it communicates like any other language. You have to take that into account when choosing between syntactically-equivalent structures. Choose the one that tells the reader what you mean. This should be: expect.equal(addonsList.length, 0, "There are no add-ons in the extensions pane list"); ...and if you do that, you won't need the explanatory comment re: passing if empty. :) Otherwise looks pretty good. I wasn't able to give it a test run (same reason as bug 669286) but it needs the touchups anyway.

Added patch with requested changes. Using expect.notEqual(), i don't want my test to fail on this. This is a 'positive' check and should pass. Thanks for r geo!

Comment on attachment 546725 [details] [diff] [review] patch v1.1 for release and beta branch >+++ b/tests/functional/restartTests/testUninstallExtension/test1.js Please add the subgroup name of Litmus as prefix to the folder name which is 'testAddons_'. Similar to what we have done for the discovery pane tests. >+const ADDON_ID = "compatibility@addons.mozilla.org"; >+const TIMEOUT_USER_SHUTDOWN = 2000; >+ // Get the addon by name >+ var addon = addonsManager.getAddons({attribute: "value", value: ADDON_ID})[0]; >+ >+ // Remove the addon >+ addonsManager.removeAddon({addon: addon}); How can you remove an add-on which hasn't been installed yet? You cannot work with the --addon option here, because that's not the way the option exists. You will have to install the ACR extension first. >+ // Verify the addon entry list is replaced by the removed add-on notification >+ var removedAddon = addonsManager.getAddons({attribute: "value", >+ value: ADDON_ID})[0]; Why do you have to retrieve the element again? Do we replace the list entry? >+ var notifyRemoved = new elementslib.Selector(controller.window.document, >+ ".addon-view[pending='uninstall']"); That's nothing for the test. It has to be part of the addons.js module. >+ expect.equal(notifyRemoved.getNode().getAttribute("pending"), "uninstall", >+ "The removed add-on notification is visible"); For attributes and properties we still want to use the assertJSProperty or assertDOMProperty methods of the controller object. >+ // Click on the list view restart link >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", >+ parent: addon}); >+ controller.click(restartLink); >+ >+ // User initiated restart >+ controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); startUserShutdown has to be called before you click onto the restart link. Otherwise Firefox can close before we notify Jsbridge about a user shutdown. >+ controller.waitFor(function () { >+ controller.window.Application.restart(); >+ }, "Application has been restarted"); This code is not necessary and can be removed. I wonder why you have to wait here. >+const ADDON_ID = "compatibility@addons.mozilla.org"; >+const SECOND_ADDON_ID = "test-icons@quality.mozilla.org"; Those ids should not be constants in each test but assigned to the persisted object, so those can be forwarded to the next test. For example see the old test: http://hg.mozilla.org/qa/mozmill-tests/file/efb11a741a54/tests/functional/restartTests/testMultipleExtensionInstallation/test1.js#l47 >+ // The add-ons list should not be empty >+ expect.notEqual(addonsList.length, 0, >+ "The extension pane list contains at least one add-on"); Why do we need this check? >+ // Check that the removed add-on is not present within the list >+ installedAddons.forEach(function (installedAddons) { >+ var addonIds = installedAddons.getNode().getAttribute("value"); >+ expect.notEqual(addonIds, ADDON_ID, "The addon has been removed"); >+ }); It will become way simpler if you directly query the addonList for the addon id. In that case the returned list of add-ons will be empty. A zero check would be everything which is necessary here. >+ // Get the second installed add-on >+ var secondAddon = addonsManager.getAddons({attribute: "value", >+ value: SECOND_ADDON_ID})[0]; As said above, it's not installed yet. >+ // Open and close the add-ons manager to dispose of the undo link >+ addonsManager.close(); >+ addonsManager.open(); Not sure why this is necessary. Can you please explain? In general you want to do the following steps: test1: * Install both extensions test2: * Remove extension 1 * Disable extension 2 test3: * Check for extension 1 * Remove extension 2 * Check for extension 2

>+ // Open and close the add-ons manager to dispose of the undo link >+ addonsManager.close(); >+ addonsManager.open(); Otherwise we cannot get rid of the "undo" label and cannot test whether the add-on has been removed or not. It's either this or move the focus away from it and wait (don't know what time) for it to disappear. But this is way more simple and elegant.

Switching categories should be faster. Just temporary select the appearances category and switch back to extensions.

This patch will suffer lots of modification - besides the nits, we have to change the way we get the addons. I don't really like installing them from amo preview site, since amo is changing a lot lately, will definitely brake the test. I'll install the extensions from mozmill-tests/data/addons. Henrik? any better ideas on this one?

No, that's absolutely fine. If we would need SSL we can use http://mozqa.com/ instead.

This patch is a re-make of the test considering Henrik's requests and advice - sorry for the huge binary content - those are the addons which need to go in mozmill-tests/data/addons

Comment on attachment 558468 [details] [diff] [review] patch v1.2 all branches >diff --git a/data/addons/add_on_compatibility_reporter-0.8.5-fx+tb+sm.xpi b/data/addons/add_on_compatibility_reporter-0.8.5-fx+tb+sm.xpi Don't use the ACR for testing purposes. We now have a couple of example extensions available in the litmus-data repository we can use for testing. If you have special needs we have to create a new one. Also please stick with the folder structure which we have in that other repository. It should be a 1-1 mapping. Thanks.

Well, in this case we'll need another extension in the litmus-data repo. Right now we only have icons.xpi which will fit in this test. We need another one in this manner. Folder structure will not be a problem. Thanks

See bug 684843. We will have some more extensions soon.

Please prioritize the dependency so we can get this fixed.

Dependency fixed. Patch fixed. Pass for me http://mozmill-crowd.brasstacks.mozilla.com/db/18fd2c24fc56c33feb3c8a50f4ecc6da

Comment on attachment 563721 [details] [diff] [review] patch v2.0 all branches >+/** >+ * Test removing the first extension and disabling the second >+ */ >+function testEnableExtension() { >+ addonsManager.open(); 1) The function name and comment describing it seem to contradict 2) You should have an assert before testing removal or disabling as we can't test these scenarios if an extension failed to install. We should only check one specific behaviour per function. I know the litmus test combines them in one test, but disabling and removing are really two different things. Can you break this into two separate tests? The Litmus test should be split in two and this Mozmill test should follow. Henrik, as co/ex-owner of Add-ons Manager, do you agree?

We already had this question. Please see my comment 6. If you disagree please start a conversation in an appropriate newsgroup or email.

Proposed fix

Proposed fix

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #21) > Comment on attachment 563721 [details] [diff] [review] [diff] [details] [review] > patch v2.0 all branches > > >+/** > >+ * Test removing the first extension and disabling the second > >+ */ > >+function testEnableExtension() { > >+ addonsManager.open(); > > 1) The function name and comment describing it seem to contradict Renamed to function testRemoveAndDisableExtension() > 2) You should have an assert before testing removal or disabling as we can't > test these scenarios if an extension failed to install. Addressed this revision. Now both add-ons are checked for a successful installation > We should only check one specific behaviour per function. I know the litmus > test combines them in one test, but disabling and removing are really two > different things. Can you break this into two separate tests? > > The Litmus test should be split in two and this Mozmill test should follow. > > Henrik, as co/ex-owner of Add-ons Manager, do you agree? Even if this contradicts a bit with our workflow, i believe i need to agree with Henrik on this one. If decide otherwise, let me know and will change the test approach.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #25) > > We should only check one specific behaviour per function. I know the litmus > > test combines them in one test, but disabling and removing are really two > > different things. Can you break this into two separate tests? > > > > The Litmus test should be split in two and this Mozmill test should follow. > > > > Henrik, as co/ex-owner of Add-ons Manager, do you agree? > > Even if this contradicts a bit with our workflow, i believe i need to agree > with Henrik on this one. If decide otherwise, let me know and will change > the test approach. I'm not sure what you are agreeing to since Henrik has not answered the question yet. Please wait until he adds his feedback and proceed as he decides. Thanks.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26) > I'm not sure what you are agreeing to since Henrik has not answered the > question yet. Please wait until he adds his feedback and proceed as he > decides. Thanks. I haven't? Please check comment 6 and comment 22.

Well guys - I'm confused right now :) Please add your decisions and let me know the directions for this test. Thanks

Thanks Henrik. I've reviewed the comments and I have a fundamental disagreement with this being in a single test, but I'm not prepared to block this bug on a fundamental discussion. Vlad, please go ahead and write this test given the following workflow: * Install Add-on * Verify installation and Disable * Verify disabled and Enable * Verify enabled and Uninstall * Verify uninstalled

Sure thing - but please keep in mind that the given workflow will not address the litmus test expected results accurately meaning: we miss testing uninstall of a disabled add-on because when we uninstall, our add-on is enabled. (that was the reason we needed two add-ons for this test) well, i guess with the new given workflow, we'll need to change the litmus test right?

Thanks Vlad. That's something I had not considered. So let me propose a follow up to the work flow: * Install 3 add-ons * Verify installed and disable 2 add-ons * Verify disabled and re-enable 1 add-on * Verify re-enabled and uninstall re-enabled add-on * Verify uninstalled and uninstall disabled add-on * Verify uninstalled and uninstall first add-on * Verify uninstalled This will ensure verification of uninstalling an unchanged, disabled, and re-enabled add-on.

There is no need to do the re-enable part here. It's part of another test. As what we have had before only 2 extensions are necessary here: * Install 2 extensions * Verify installed and disable 1 extension * Verify disabled and uninstall extension * Verify uninstalled and uninstall the enabled extension * Verify uninstalled

Well I have a patch ready for Anthony;s workflow request. I'll do the test in Henrik's manner as well and upload both for r. Then you guys can decide which approach we want better :)

Anthony's request addressed in this patch. Need to follow up with Henrik's proposal

Comment on attachment 564809 [details] [diff] [review] patch v3.0 - Anthony's proposal Just submit the patch for Henrik's suggested workflow.

Henrik can you please step in for a feedback since it was your approach idea? Thanks

Comment on attachment 565474 [details] [diff] [review] patch v4.0 all branches >+ // Restart the browser using restart prompt >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", >+ parent: secondAddon}); >+ >+ controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); >+ >+ controller.click(restartLink); I don't believe we want a user initiated restart as this leads to a race condition in Mozmill. We should let Mozmill restart the browser. Henrik, please correct me if I am wrong. // Check that the second extension was successfully uninstalled >+ var secondAddonIdList = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id}); >+ >+ expect.equal(secondAddonIdList.length, 0, >+ "The second add-on is no longer present in the add-on list"); Is this true? Do we not have to restart to do a full uninstallation of an add-on? (disabled or not) >+ // Restart the browser using restart prompt >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", >+ parent: firstAddon}); >+ >+ controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); >+ >+ controller.click(restartLink); >+} Same as before. >+ // Check that the first extension was successfully uninstalled >+ var addonIdList = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[0].id}); >+ >+ expect.equal(addonIdList.length, 0, >+ "The first add-on is no longer present in the add-on list"); >+} Just for good measure, can you check to ensure both add-ons included in your first test are no longer installed?

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #37) > >+ controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > >+ > >+ controller.click(restartLink); > > I don't believe we want a user initiated restart as this leads to a race > condition in Mozmill. We should let Mozmill restart the browser. Henrik, > please correct me if I am wrong. This code is fully correct. We want to test the restart link here, so startUserShutdown has to be used to indicate Mozmill that the test itself is triggering a restart. Nit: please remove the empty line in-between the lines.

Comment on attachment 565474 [details] [diff] [review] patch v4.0 all branches >+ url: LOCAL_TEST_FOLDER + LOCAL_INSTALL_SCRIPT + "extensions/icons.xpi"}, I would only specify the relative path to the extension here and concatenate the strings when needed. >+function testInstallAddon() { >+ // Update extensions.blocklist.url pref to our blocklist >+ prefs.preferences.setPref("security.dialog_enable_delay", >+ INSTALL_DIALOG_DELAY); This has to be part of setupModule and the comment doesn't apply here. >+ persisted.addons.forEach(function(addon) { nit: space between function and the brackets please. >+ var md = new modalDialog.modalDialog(addonsManager.controller.window); Move the declaration out of the loop. Is has to be done only once. >+ // Install the addon >+ md.start(addons.handleInstallAddonDialog); >+ controller.click(addToFirefox); >+ md.waitForDialog(TIMEOUT_DOWNLOAD); >+ controller.keypress(null , 'VK_ESCAPE', {}); Why do we need the keypress? Please comment and separate it from the installation block. >+ // Check that the extensions were successfully installed >+ assert.ok(addonsManager.isAddonInstalled({addon: firstAddon}), >+ "The first add-on was successfully installed"); >+ assert.ok(addonsManager.isAddonInstalled({addon: secondAddon}), >+ "The second add-on was successfully installed"); Please use expect here. We want to know that both extensions are installed if something failed for the first extension. >+ // Disable the second extension >+ addonsManager.disableAddon({addon: secondAddon}); Before restarting I would already add another check if it has been marked to be disabled. Otherwise it looks fine.

> > // Check that the second extension was successfully uninstalled > >+ var secondAddonIdList = addonsManager.getAddons({attribute: "value", > >+ value: persisted.addons[1].id}); > >+ > >+ expect.equal(secondAddonIdList.length, 0, > >+ "The second add-on is no longer present in the add-on list"); > > Is this true? Do we not have to restart to do a full uninstallation of an > add-on? (disabled or not) Yes Anthony. Disabled add-ons do no require restart to uninstall - we can do the check in the same test as long as we get rid of the 'undo' link (which can be seen in the test code) Just to be sure, please check the litmus test also to confirm. > > Just for good measure, can you check to ensure both add-ons included in your > first test are no longer installed? I'm not sure we need this. due to the fact that there are failing tests for this component, i want to keep tests as simple as possible and avoid extra checks if can. On the other end, if this is a must from your side, it's no problem at all code-wise to add another check.

> > >+ var md = new modalDialog.modalDialog(addonsManager.controller.window); > Moving this out of the loop will be with the price of having a global variable or another instance of the persisted object to be used. The loop uses an anonymous function which carries the 'addon' parameter and the need of it is justified. IMO, i would leave it as it is. > >+ controller.keypress(null , 'VK_ESCAPE', {}); > > Why do we need the keypress? Please comment and separate it from the > installation block. > We install multiple extensions and the keypress is used to dispose the restart prompt of the first add-on we install, and let the second one install also in the same test. then we restart one single time for two add-ons in this manner. With all proposals i agree and will be addressed in a follow up patch Thanks :)

New proposed patch with nits addressed

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #41) > > > > >+ var md = new modalDialog.modalDialog(addonsManager.controller.window); > > > > Moving this out of the loop will be with the price of having a global > variable or another instance of the persisted object to be used. The loop > uses an anonymous function which carries the 'addon' parameter and the need > of it is justified. IMO, i would leave it as it is. I have absolutely no idea what you are referring here. Which other global variable? I don't see anything. We really only need one single instance of the modalDialog class, which can be reused in each iteration. > We install multiple extensions and the keypress is used to dispose the > restart prompt of the > first add-on we install, and let the second one install also in the same > test. then we restart one single time for two add-ons in this manner. Does it mean we can't access the location bar and open the new location until the notification bar has been closed? I only ask because I'm not sure we really need this step.

(In reply to Henrik Skupin (:whimboo) from comment #43) > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #41) > > > > > > >+ var md = new modalDialog.modalDialog(addonsManager.controller.window); > > > > > > > Moving this out of the loop will be with the price of having a global > > variable or another instance of the persisted object to be used. The loop > > uses an anonymous function which carries the 'addon' parameter and the need > > of it is justified. IMO, i would leave it as it is. > > I have absolutely no idea what you are referring here. Which other global > variable? I don't see anything. We really only need one single instance of > the modalDialog class, which can be reused in each iteration. Well I am saying that if we move the 'md' variable outside the loop it will not be seen because it's used inside the anonymous function. > > We install multiple extensions and the keypress is used to dispose the > > restart prompt of the > > first add-on we install, and let the second one install also in the same > > test. then we restart one single time for two add-ons in this manner. > > Does it mean we can't access the location bar and open the new location > until the notification bar has been closed? I only ask because I'm not sure > we really need this step. Exactly

Ups. Forgot one nit: here is the final patch

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #44) > > > Moving this out of the loop will be with the price of having a global > > > variable or another instance of the persisted object to be used. The loop > > > uses an anonymous function which carries the 'addon' parameter and the need > > > of it is justified. IMO, i would leave it as it is. > > > > I have absolutely no idea what you are referring here. Which other global > > variable? I don't see anything. We really only need one single instance of > > the modalDialog class, which can be reused in each iteration. > > Well I am saying that if we move the 'md' variable outside the loop it will > not be seen > because it's used inside the anonymous function. Why that? The variable will be in the function scope and accessible from any closure within it. Not sure why you think that will not work.

> > Why that? The variable will be in the function scope and accessible from any > closure within it. Not sure why you think that will not work. Well the loop has an anonymous function there which carries the addon parameter. the 'md' var is inside this function so when taking it out will cause error

I still don't see why it should give errors when you move out the declaration of the modal dialog instance. Please provide some code.

http://pastebin.mozilla.org/1352769 I've commented on the piece of code

Tried your latest patch and it works fine for me. So removing feedback request until it has been sorted out.

Fixed

Comment on attachment 566220 [details] [diff] [review] patch v5.2 all branches >+const LOCAL_INSTALL_SCRIPT = "addons/install.html?addon="; >+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/"); >+ >+const ADDONS = [ >+ {id: "test-icons@quality.mozilla.org", >+ url: "extensions/icons.xpi"}, >+ {id: "test-long-name@quality.mozilla.org", >+ url: "extensions/long-name.xpi"}, As discussed on the other bug please revert this change and set the full url for each extension. We could even by-pass the install page by directly opening the xpi files. >+ // Check that the extensions were successfully installed >+ expect.ok(addonsManager.isAddonInstalled({addon: firstAddon}), >+ "The first add-on was successfully installed"); >+ expect.ok(addonsManager.isAddonInstalled({addon: secondAddon}), >+ "The second add-on was successfully installed"); Instead of first/second you should use the extension id in the message. >+ assert.ok(!addonsManager.isAddonEnabled({addon: secondAddon}), >+ "The add-on is successfully disabled"); Same here and probably other instances. Also there is no need for 'successfully'. With those changes across files you have my f+

Fixed Addressed Henrik's feedback Not asking again for f? since it was a + last time

Comment on attachment 566467 [details] [diff] [review] patch v5.3 all branches >+ // Check that the extensions were installed >+ expect.ok(addonsManager.isAddonInstalled({addon: iconsAddon}), >+ "The icons add-on was installed"); >+ expect.ok(addonsManager.isAddonInstalled({addon: longNameAddon}), >+ "The long-name add-on was installed"); I believe what Henrik was suggesting, and what I would like to see here, is that we use the physical ID (as above). For example: "Addon " + iconsAddon.id + " is installed." >+/** >+ * Tests for disabling and uninstall of an add-on >+ */ >+function testUninstallDisabledAddon() { Please revise the comment to "Test for uninstalling a disabled add-on" >+ addonsManager.open(); >+ >+ // Check that the long-name extension was disabled >+ var longNameAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id})[0]; >+ >+ assert.ok(!addonsManager.isAddonEnabled({addon: longNameAddon}), >+ "The add-on is disabled"); This should be "expect" as we cannot test uninstalling a disabled add-on if it has not been disabled. >+/** >+ * Verifies uninstalling of an enabled add-on >+ */ >+function testVerifyUninstallAddon() { Just some minor rewording... "Verifies enabled add-on was uninstalled" and testEnabledAddonUninstalled >+ expect.equal(addonIdList.length, 0, >+ "The first add-on is no longer present in the add-on list"); Please use the add-on ID in the message.

Fixed. Thanks for being patient for this late coming patch

Comment on attachment 568540 [details] [diff] [review] patch v6 all branches >+/* >+ * Install the add-ons from data/ folder >+ */ Install some add-ons to test uninstallation >+function testInstallAddon() { testInstallAddons() { >+ var iconsAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[0].id})[0]; >+ var longNameAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id})[0]; >+ >+ // Check that the extensions were installed >+ expect.ok(addonsManager.isAddonInstalled({addon: iconsAddon}), >+ "Add-on " + persisted.addons[0].id + " was installed"); >+ expect.ok(addonsManager.isAddonInstalled({addon: longNameAddon}), >+ "Add-on " + persisted.addons[1].id + " was installed"); You can simplify this code by using a forEach >+ // Check that the long-name extension was marked for disable >+ expect.equal(longNameAddon.getNode().getAttribute("pending"), "disable"); Make this an assert >+ // Check that the long-name extension was uninstalled >+ var addonIdList = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id}); Fix alignment >+ >+ expect.equal(addonIdList.length, 0, >+ "The add-on " + persisted.addons[1].id + >+ " is no longer present in the add-on list"); "has been uninstalled" >+/** >+ * Removes an enabled extension >+ */ "Test uninstalling an enabled extension" >+ expect.equal(addonIdList.length, 0, >+ "Add-on " + persisted.addons[0].id + >+ " is no longer present in the add-on list"); "has been uninstalled"

Addressed all revisions except the forEach having the forEach will simplify the code but in the expense of another persisted initialization, which i do not recommend we do for just 3 lines of code. this is because we need to use one add-on separately after, to disable it, and i would have to create another instance of the object anyway, outside the forEach(), or just create another persisted instance again, which i don't really consider the best practice. I would pretty much rely on your feedback here

Comment on attachment 571037 [details] [diff] [review] patch v7 all branches >+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/addons/"); Please use the same convention for local test files as we have in other tests. Having the base separated out is important in the future. >+function setupModule() { >+ controller = mozmill.getBrowserController(); >+ addonsManager = new addons.AddonsManager(controller); As mentioned on the other bug please use aModule in setupModule and teardownModule. >+ prefs.preferences.setPref("security.dialog_enable_delay", >+ INSTALL_DIALOG_DELAY); The pref name should be a constant. >+ // Whitelist add the local test folder >+ addons.addToWhiteList(LOCAL_TEST_FOLDER); We want to whitelist the domain and not the whole URL. >+function testInstallAddon() { Anthony's point hasn't been addressed. Also it should contain extension and not addon. >+ // Dispose of the restart prompt by keyboard event >+ controller.keypress(null , 'VK_ESCAPE', {}); nit: It's not a prompt but a doorhanger. >+ var iconsAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[0].id})[0]; >+ var longNameAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id})[0]; Don't use names of the extensions for variables. Instead make use of enabled/disable. We don't want to change massive lines of code if we have to switch extensions. >+ expect.ok(addonsManager.isAddonInstalled({addon: iconsAddon}), >+ "Add-on " + persisted.addons[0].id + " was installed"); Use single quotes around the extension id. Also use extension in the message. >+ // Check that the long-name extension was marked for disable >+ assert.equal(longNameAddon.getNode().getAttribute("pending"), "disable"); I miss the assertion message here. >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", >+ parent: longNameAddon}); >+ >+ controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); >+ >+ controller.click(restartLink); No need to separate the code here. >+ assert.ok(!addonsManager.isAddonEnabled({addon: longNameAddon}), >+ "The add-on is disabled"); Update the message to it is similar to the other ones and contains the id. >+function testUninstallEnabledAddon() { [..] >+ addonsManager.removeAddon({addon: iconsAddon}); Before we restart I would like to see a test that the extension has been marked to get uninstalled after the restart. Also, please note that if you request feedback, there is no need to also ask for review at the same time.

Changes addressed

Comment on attachment 571914 [details] [diff] [review] patch v8 all branches >+const INSTALL_DIALOG_PREF = "security.dialog_enable_delay"; [..] >+const TIMEOUT_DOWNLOAD = 25000; >+const INSTALL_DIALOG_DELAY = 1000; Please use PREF as prefix and not suffix. That directly visualizes what the constant stands for. Also move it down to the second const block. >+ // Whitelist add the local test folder >+ addons.addToWhiteList("http://localhost:43336"); As mentioned in the review for changing a theme, we don't want to hardcode the hostname and port. Get this information from the test folder constant as before but without the whole path. >+/* >+ * Install some add-ons to test uninstallation >+ */ >+function testInstallAddons() { 3rd reminder on updating the function name. Please re-read the review comments again before uploading a new patch version. >+ var enabledAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[0].id})[0]; >+ var disableAddon = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id})[0]; > >+ // Disable the extension >+ addonsManager.disableAddon({addon: disableAddon}); I would propose to use 'toDisableExtension' because at this time the extension is enabled. Also please remember to change Addon to Extension overall.

fixed

Comment on attachment 573784 [details] [diff] [review] patch v9 all branches Taking over review as I am back from PTO. >+function setupModule(aModule) { >+ aModule.controller = mozmill.getBrowserController(); >+ aModule.addonsManager = new addons.AddonsManager(controller); Not sure why you have started to add aModule to your tests. This is unnecessary AFAIK and not in line with other tests we have checked in. >+function setupModule(aModule) { >+ aModule.controller = mozmill.getBrowserController(); >+ aModule.addonsManager = new addons.AddonsManager(controller); Same here. >+ var enabledExtension = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[0].id})[0]; >+ var toDisableExtension = addonsManager.getAddons({attribute: "value", >+ value: persisted.addons[1].id})[0]; >+ >+ // Check that the extensions were installed >+ expect.ok(addonsManager.isAddonInstalled({addon: enabledExtension}), >+ "Extension '" + persisted.addons[0].id + "' was installed"); >+ expect.ok(addonsManager.isAddonInstalled({addon: toDisableExtension}), >+ "Extension '" + persisted.addons[1].id + "' was installed"); >+ >+ // Disable the extension >+ addonsManager.disableAddon({addon: toDisableExtension}); >+ >+ // Check that the extension was marked for disable >+ assert.equal(toDisableExtension.getNode().getAttribute("pending"), "disable", >+ "The extension '" + persisted.addons[1].id + "' was marked for disable"); >+ >+ // Restart the browser using restart prompt >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", >+ parent: toDisableExtension}); This whole code block can be refactored to use a simple array. This will simplify the code considerably. >+function setupModule(aModule) { >+ aModule.controller = mozmill.getBrowserController(); >+ aModule.addonsManager = new addons.AddonsManager(controller); Again, unnecessary use of aModule. >+function setupModule(aModule) { >+ aModule.controller = mozmill.getBrowserController(); >+ aModule.addonsManager = new addons.AddonsManager(controller); Again, unnecessary use of aModule. >+function setupModule(aModule) { >+ aModule.controller = mozmill.getBrowserController(); >+ aModule.addonsManager = new addons.AddonsManager(controller); Again, unnecessary use of aModule.

Usage of a parameter in setupModule and teardownModule, in our case 'aModule', avoids using global declarations which is a good practice. 2nd, it was a revision request from Henrik - please look at the previous reviews before requesting Initially, the test had not been using 'aModule', then changed to using, now I have to change back. I will address to the other requests, and wait for a response here. I suggest you and Henrik decide final about it. Thanks guys

It is my final say on tests and I have seen no one else use it, nor any requests from Henrik for me to have you guys use it. I am okay with including it as a parameter in the function signature, but please do not use it in variable declarations. While it doesn't break any tests, this is a fundamental change to our tests. I would rather something like this gets addressed in a refactor so as not to cause confusion for newcomers trying to learn by reading code. In short: function setupModule(aModule) { is okay. aModule.controller = mozmill.getBrowserController(); is not okay. Please make this change and adopt it for all tests going forward. Thanks

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #62) > >+function setupModule(aModule) { > >+ aModule.controller = mozmill.getBrowserController(); > >+ aModule.addonsManager = new addons.AddonsManager(controller); > > Not sure why you have started to add aModule to your tests. This is > unnecessary AFAIK and not in line with other tests we have checked in. It's not unnecessary because that avoids us from creating global variables instead of directly assigning it to the module. But I hear you, and you are probably right that we should wait for using aModule until we do the big refactoring of tests.

> > >+ var enabledExtension = addonsManager.getAddons({attribute: "value", > >+ value: persisted.addons[0].id})[0]; > >+ var toDisableExtension = addonsManager.getAddons({attribute: "value", > >+ value: persisted.addons[1].id})[0]; > >+ > >+ // Check that the extensions were installed > >+ expect.ok(addonsManager.isAddonInstalled({addon: enabledExtension}), > >+ "Extension '" + persisted.addons[0].id + "' was installed"); > >+ expect.ok(addonsManager.isAddonInstalled({addon: toDisableExtension}), > >+ "Extension '" + persisted.addons[1].id + "' was installed"); > >+ > >+ // Disable the extension > >+ addonsManager.disableAddon({addon: toDisableExtension}); > >+ > >+ // Check that the extension was marked for disable > >+ assert.equal(toDisableExtension.getNode().getAttribute("pending"), "disable", > >+ "The extension '" + persisted.addons[1].id + "' was marked for disable"); > >+ > >+ // Restart the browser using restart prompt > >+ var restartLink = addonsManager.getElement({type: "listView_restartLink", > >+ parent: toDisableExtension}); > > This whole code block can be refactored to use a simple array. This will > simplify the code considerably. .getAddons(aSpec) means that getAddons() does not accept an array as parameter, just one object, and can return a list of addons if its the case, which in our code is not, because our addons have different ids - this means using an array is impossible. can't use an array? use a forEach statement. when you try using forEach your bottleneck will be this line of code > addonsManager.disableAddon({addon: toDisableExtension}); a forEach will disable both add-ons, so if we were to disable one of them we can set a flag as parameter and switch values for it, and by the flag value decide which add-on to be disabled or not (extra if statements and so) - but this complicates the code and we want to be very pretty and readable for our contributors, won't we? third, we can always leave the code as it is - because IMO: 1. it's very similar to the approach of other tests (people can re-use this block of code) 2. it turns out to be the simplest way after all lets not exclude the fact that i can be missing something and there can be a better solution after all, so if you have that can you please provide a working example?

Fair enough, please re-request review.

Final fixes

Comment on attachment 576462 [details] [diff] [review] patch v10 all branches >+function setupModule(aModule) { Remove aModule from everywhere. >+ // Check that the extensions were installed >+ expect.ok(addonsManager.isAddonInstalled({addon: enabledExtension}), >+ "Extension '" + persisted.addons[0].id + "' was installed"); >+ expect.ok(addonsManager.isAddonInstalled({addon: toDisableExtension}), >+ "Extension '" + persisted.addons[1].id + "' was installed"); Should both be asserts. >+ expect.equal(addonIdList.length, 0, >+ "Extension '" + persisted.addons[1].id + >+ "' has been uninstalled"); Assert. >+ expect.equal(extensionIdList.length, 0, >+ "Extension '" + persisted.addons[0].id + >+ "' has been uninstalled"); Assert.

I have to check how patches are refreshed more often - it seems there was a problem with adding changes to patch revision. Sorry. Hope this time will be ok

Comment on attachment 576710 [details] [diff] [review] patch v11 [checked-in] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/740ff4a1df0b (default) http://hg.mozilla.org/qa/mozmill-tests/rev/72b43d3d60e0 (mozilla-aurora)

Patch fails on Beta: /lib/addons.js:415: "AddonsManager_getAddonChildElement: Add-on not specified." Please submit a follow-up patch which works for Beta and Release.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #72) > Patch fails on Beta: > /lib/addons.js:415: "AddonsManager_getAddonChildElement: Add-on not > specified." > > Please submit a follow-up patch which works for Beta and Release. Worked for me - thanks for catching this I will investigate asap and follow up this thread

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #72) > Patch fails on Beta: > /lib/addons.js:415: "AddonsManager_getAddonChildElement: Add-on not > specified." > > Please submit a follow-up patch which works for Beta and Release. Anthony, can you please provide some more details about the failure? Log maybe, build used. I tried to reproduce it but it does not fail for me on the latest beta Here are the results http://mozmill-crowd.brasstacks.mozilla.com/db/52005abeeeea9c93c7204b6c742521cd

There is nothing more I can tell you except that I am using Mac OSX 10.6.8 with Mozmill 1.5.7 and 9.0b4 in this case. The test works fine with latest-aurora and latest-mozilla-central.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #75) > There is nothing more I can tell you except that I am using Mac OSX 10.6.8 > with Mozmill 1.5.7 and 9.0b4 in this case. The test works fine with > latest-aurora and latest-mozilla-central. I was using a 9.0b3 can you point me to a download link for beta4?

Sorry, I meant 9.0b3, that was a typo.

With the merge today we were unable to merge this test forward to Beta. Please address this issue immediately, Vlad. Also, I noticed the final test (test5.js) has no teardown module. Should the last test in a restart test not have a teardown module?

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #79) Should the last test in a restart test > not have a teardown module? teardown is used to clean-up - its not mandatory only if you explicitly must clean up some memory or process, etc after you. With the merge today we were unable to merge this test forward to Beta. Please address this issue immediately, Vlad Can I please have some logs or what error did you get? I cannot address to the issue having only that information, since I cannot test moving tests across branches on Firefox releases. Having the logs might get me the clue of why it failed to move. thanks

There was not an error during the merge. I explicitly had to remove it from the merge because the failure I reported in comment 72 has since been unresolved.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #81) > There was not an error during the merge. I explicitly had to remove it from > the merge because the failure I reported in comment 72 has since been > unresolved. Does it still fail for you? - because I cannot reproduce it locally

The test does still fail for me locally. The failure is triggered during test4.js and actually fails within the addons.js API after attempting to remove the addon... ERROR: lib/addons.js:415 "AddonsManager_getAddonChildElement: Add-on not specified." ENVIRONMENT: I've reproduced this failure on 9.0b3, 9.0b6, and now 10.0b1. I'm running Mozmill 1.5.7 on Mac OS X 10.6.8. COMMAND: mozmill-restart -b /Applications/Beta.app/Contents/MacOS/firefox -t ./tests/functional/restartTests/testAddons_uninstallExtension/ --show-errors

This failure also happens constantly for Nightly and Aurora builds on OS X. See the results from our Mozmill CI test-runs: http://mozmill-archive.brasstacks.mozilla.com/#/functional/reports Can we please get the failure fixed or the test disabled?

(In reply to Henrik Skupin (:whimboo) from comment #84) > This failure also happens constantly for Nightly and Aurora builds on OS X. > See the results from our Mozmill CI test-runs: > > http://mozmill-archive.brasstacks.mozilla.com/#/functional/reports > > Can we please get the failure fixed or the test disabled? We are going to try a fix asap If unsuccessful, a skip patch will follow by eod

I was wondering which event gets fired for a category if it already has been selected? It is important for me to provide a robust patch for this fix. cc-ing Dave and Blair maybe they can help

skipping for now investigated the problem - we have the cause: we need to wait for the add-on list in the extension category. working with Henrik to find the best solution

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #86) > I was wondering which event gets fired for a category if it already has been > selected? > > It is important for me to provide a robust patch for this fix. > > cc-ing Dave and Blair maybe they can help If you select a category that is already selected then nothing happens, I don't think any event is fired in this case

(In reply to Dave Townsend (:Mossop) from comment #88) > If you select a category that is already selected then nothing happens, I > don't think any event is fired in this case So what would be the best way to wait until the pane has been finished loading? Is there any particular property we could check for? As workaround we would have to select another category and go back, but that's not optimal. :/

(In reply to Henrik Skupin (:whimboo) from comment #89) > (In reply to Dave Townsend (:Mossop) from comment #88) > > If you select a category that is already selected then nothing happens, I > > don't think any event is fired in this case > > So what would be the best way to wait until the pane has been finished > loading? Is there any particular property we could check for? As workaround > we would have to select another category and go back, but that's not > optimal. :/ Perhaps I'm not understanding. If the category is already selected and loaded then clicking on the same category again will do nothing and send no event. If the category is not yet loaded then it will send the usual even when it completes loading.

The use case here is the following: 1. Install an extension and select the extension pane (event gets sent) 2. Restart Firefox (extension pane will be auto-selected) 3. Access an add-on Step 3 currently fails because we don't wait until the pane has been finished loading. Given your last reply the 'ViewChanged' event will be sent once the pane (even it's already selected) has been finished loading?

(In reply to Henrik Skupin (:whimboo) from comment #91) > The use case here is the following: > > 1. Install an extension and select the extension pane (event gets sent) > 2. Restart Firefox (extension pane will be auto-selected) > 3. Access an add-on > > Step 3 currently fails because we don't wait until the pane has been > finished loading. Given your last reply the 'ViewChanged' event will be sent > once the pane (even it's already selected) has been finished loading? Ah I see, yes we still send the event when the first view loads

Thanks Dave. It works now, and I have attached a patch for our shared modules on bug 715022. Once it has been fixed the tests shouldn't fail anymore.

I'm going to delay landing of the check patch since I just checked in the fix for bug 715022. Henrik, please check CI tomorrow to see if this test is still failing.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #94) > I'm going to delay landing of the check patch since I just checked in the > fix for bug 715022. Henrik, please check CI tomorrow to see if this test is > still failing. I did a test-run some hours ago and the failure has gone on default now. Same would apply to aurora. So we should be ready to get the test landed for beta and release too. Please re-test them before landing, just to be on the safe side.

Comment on attachment 585385 [details] [diff] [review] skip patch v1.0 Test passes now on Beta so I will check it in; making this skip patch obsolete.

Comment on attachment 576710 [details] [diff] [review] patch v11 [checked-in] Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/5a279e705fae (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/ab129704ed71 (mozilla-release)

I will keep an eye out for this with 10.0b3 testing to see if it can be verified.

This did not fail in the 10.0b3 testrun this evening so I'm marking this verified.