670904 - Mozmill test for uninstalling an extension

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Tracking creation of new mozmill test for uninstalling an extension. 

Litmus test: https://litmus.mozilla.org/show_test.cgi?id=15629

Comment 1

[u415141]

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

Comment 2

[u279076]

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?

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP v1.0

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 4

[u279076]

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");
}

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

>+  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

Comment 6

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

(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.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0 for release and beta

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

Thanks!

Comment 8

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

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.

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1 for release and beta branch

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 10

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

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 = "[e-mail]";
>+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 = "[e-mail]";
>+const SECOND_ADDON_ID = "[e-mail]"; 

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

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

>+  // 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.

Comment 12

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

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

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

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?

Comment 14

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

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

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2 all branches

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 16

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

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.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 18

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

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

Comment 19

[u279076]

Please prioritize the dependency so we can get this fixed.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0 all branches

Dependency fixed. 
Patch fixed. 

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

Comment 21

[u279076]

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?

Comment 22

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

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

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.1 all branches

Proposed fix

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.1 all branches

Proposed fix

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

(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.

Comment 26

[u279076]

(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.

Comment 27

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

(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.

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 29

[u279076]

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

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

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?

Comment 31

[u279076]

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.

Comment 32

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

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

Comment 33

[Maniac Vlad Florin (:vladmaniac)]

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 :)

Comment 34

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.0 - Anthony's proposal

Anthony's request addressed in this patch. 

Need to follow up with Henrik's proposal

Comment 35

[u279076]

Comment on attachment 564809 [details] [diff] [review]
patch v3.0 - Anthony's proposal

Just submit the patch for Henrik's suggested workflow.

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.0 all branches

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

Comment 37

[u279076]

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?

Comment 38

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

(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 39

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

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.

Comment 40

[Maniac Vlad Florin (:vladmaniac)]

> 
> // 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.

Comment 41

[Maniac Vlad Florin (:vladmaniac)]

> 
> >+    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 :)

Comment 42

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.0 all branches

New proposed patch with nits addressed

Comment 43

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

(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.

Comment 44

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 45

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.1 all branches

Ups. Forgot one nit: here is the final patch

Comment 46

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

(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.

Comment 47

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 48

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

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.

Comment 49

[Maniac Vlad Florin (:vladmaniac)]

http://pastebin.mozilla.org/1352769 

I've commented on the piece of code

Comment 50

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

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

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.2 all branches

Fixed

Comment 52

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

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: "[e-mail]", 
>+   url: "extensions/icons.xpi"},
>+  {id: "[e-mail]",
>+   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+

Comment 53

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.3 all branches

Fixed 

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

Comment 54

[u279076]

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.

Comment 55

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v6 all branches

Fixed. 

Thanks for being patient for this late coming patch

Comment 56

[u279076]

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"

Comment 57

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v7 all branches

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 58

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

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.

Comment 59

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v8 all branches

Changes addressed

Comment 60

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

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.

Comment 61

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v9 all branches

fixed

Comment 62

[u279076]

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.

Comment 63

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 64

[u279076]

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

Comment 65

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

(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.

Comment 66

[Maniac Vlad Florin (:vladmaniac)]

> 
> >+  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?

Comment 67

[u279076]

Fair enough, please re-request review.

Comment 68

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v10 all branches

Final fixes

Comment 69

[u279076]

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.

Comment 70

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v11 [checked-in]

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 71

[u279076]

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)

Comment 72

[u279076]

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.

Comment 73

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 74

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 75

[u279076]

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.

Comment 76

[Maniac Vlad Florin (:vladmaniac)]

(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?

Comment 77

[u279076]

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

Comment 79

[u279076]

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?

Comment 80

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 81

[u279076]

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.

Comment 82

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 83

[u279076]

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

Comment 84

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

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?

Comment 85

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 86

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 87

[Maniac Vlad Florin (:vladmaniac)]

Attachment: skip patch v1.0

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

Comment 88

[Dave Townsend [:mossop]]

(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

Comment 89

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

(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. :/

Comment 90

[Dave Townsend [:mossop]]

(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.

Comment 91

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

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?

Comment 92

[Dave Townsend [:mossop]]

(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

Comment 93

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

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.

Comment 94

[u279076]

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.

Comment 95

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

(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 96

[u279076]

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 97

[u279076]

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)

Comment 98

[u279076]

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

Comment 99

[u279076]

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