599867 - Update restart test module testThemeInstallUninstall to reflect the new Add-ons API

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: WIP v1

Comment 2

[Aaron Train [:aaronmt]]

Attachment: WIP v2

This reverts the test back to downloading a theme through AMO.

There's an issue with the discover pane and the learn more button. I was expecting waitThenClick to work on this element, but it does not - as a temp I am sleeping for two seconds

Comment 3

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

(In reply to comment #2)
> This reverts the test back to downloading a theme through AMO.
> 
> There's an issue with the discover pane and the learn more button. I was
> expecting waitThenClick to work on this element, but it does not - as a temp I
> am sleeping for two seconds

Please don't go through the discovery pane. This is still a totally different path we will have to cover with other tests. So please really keep the original steps and don't modify those regarding the litmus test.

Comment 4

[Aaron Train [:aaronmt]]

(In reply to comment #3)
> (In reply to comment #2)
> > This reverts the test back to downloading a theme through AMO.
> > 
> > There's an issue with the discover pane and the learn more button. I was
> > expecting waitThenClick to work on this element, but it does not - as a temp I
> > am sleeping for two seconds
> 
> Please don't go through the discovery pane. This is still a totally different
> path we will have to cover with other tests. So please really keep the original
> steps and don't modify those regarding the litmus test.

As per the litmus instructions and original test, I am using that button because it's only way to get to AMO from the Addons Manager, thus is the reason I selected it. There is no "Browse All Add-ons" link anymore, it has been replaced with the Learn More button in the Get Add-ons pane.

Please see the litmus test: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7973

Comment 5

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

Oh, so my bad. Sorry. This part doesn't make sense at all in this test. I'm sure I commented on that already in the original implementation bug. Please leave this step out and directly go to AMO. Also please update the filename i.e. testThemeInstallUninstallFromAMO.js so path is clear. Thanks.

Comment 6

[Aaron Train [:aaronmt]]

Attachment: WIP v3

Removes the AOM part of the test and goes directly to AMO. Thanks for taking a look again.

Only two things, I question before I will post a review. A) I marked the verification for the restart link check as dependant on the bug 599771 which will land after b7, is that alright? and B) Since we are downloading from AMO, I just wait in the test for the door-hanger notification to appear, is that alright?

Comment 7

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

(In reply to comment #6)
> Only two things, I question before I will post a review. A) I marked the
> verification for the restart link check as dependant on the bug 599771 which
> will land after b7, is that alright?

That's fine. Eventually we should wait with the check-in or just do a follow-up patch later.

> B) Since we are downloading from AMO,
> I just wait in the test for the door-hanger notification to appear, is that
> alright?

I would say we can check this beside the feature you will get with the waitForDownloaded function. The door hanger notification is similar to the other ones we have, i.e. geolocation? Could we move stuff out to the tabbrowserapi as well?

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Comment 9

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

Comment on attachment 480218 [details] [diff] [review]
Patch v1 - (default)

Wow, haven't thought about that renaming the folder already gives us such a huge diff which is kinda hard to review. Aaron can you upload a patch without renaming the folder please? I can rename it right before the check-in. That makes is much easier for me. Thanks.

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (no rename)

Sure.

Comment 11

[Aaron Train [:aaronmt]]

Comment on attachment 480609 [details] [diff] [review]
Patch v1 - (no rename)

Going to make changes based on another review first

Comment 12

[Aaron Train [:aaronmt]]

Changes made. Patch pending my question here https://bugzilla.mozilla.org/show_bug.cgi?id=599865#c3

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Comment 14

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

Comment on attachment 480703 [details] [diff] [review]
Patch v1 - (default)

>-const gDownloadTimeout = 60000;

Don't we use this timeout? Strange. We should really make use of it.

>+var setupModule = function() {
>+  // Store the default theme guid in 'persisted.defaultThemeId'
>   persisted.defaultThemeId = "{972ce4c6-7e08-4474-a285-3208198ce6fd}";

Can we also make it an object with id and name like the theme to install? Then we can store it as persisted.defaultTheme.

>+  controller.waitFor(
>+    function() {
>+      return installAddonButton.getNode().getAttribute('class').indexOf('installer') != -1;
>+    }, TIMEOUT, 100, "'Download Now' button should be accessible"
>+  );

Please put the function head in the same line as waitFor.

>+  // Wait until the extension is installed notification
>+  // XXX: Depends on the implementation of the door hanger API in the TabbedBrowsingAPI 
>+  // Bug 599682
>+  var installNotification = new elementslib.ID(
>+                              controller.window.document, 
>+                              "addon-install-complete-notification"
>+                            );
>+  controller.waitForElement(installNotification);

Please add a new entry in the getElements function of the AddonsAPI for this notification, i.e. "notification_installComplete". Then you can use addonsManager.getElement() here.

>+  // Check if the restart link and remove button are present
>+  // XXX: Bug 599771 - Once resolved, the test will use this button to restart
>+  //var restartLink = am.getAddonLink({addon: addon, link: "restart"});
>+  //controller.assertNodeExist(restartLink);
>+  
>+  var removeButton = addonsManager.getAddonButton({
>+                       addon: addon,
>+                       button: "remove"
>+                     });
>+  controller.assertNode(removeButton);

Remove these lines. We will use them for the tests when installing from the search category. For tests related to AMO please click the Restart Now button of the notification. Make sure to tell Mozmill about a test triggered restart.

>-
>+  

nit: two spaces went in here? :)

>+  controller.assert(function() {
>+    return (itemElem.childNodes.length == 1) == true;

No need to add the "== true" part here.

>+  controller.assert(function() { 
>+    return (itemElem.childNodes[0].name == persisted.theme.name) == true;
>+  }, "Listed and selected theme name should be equal");

Same here.

>+  controller.assert(function() { 
>+    return (itemElem.childNodes[0].url.indexOf('addons.mozilla.org') != -1) == true;
>+  }, "Theme URL should be installed from addons.mozilla.org");

*snip*

>-
>+  

*snip*

>+var testThemeChange = function() {
[..]
>+  // Check if the restart link is present
>+  // XXX: Bug 599771
>+  //var restartLink = am.getAddonLink({addon: addon, link: "restart"});
>+  //controller.assertNodeExist(restartLink);
>+  
>+  // Check if the undo link  is present
>+  var undoLink = addonsManager.getAddonLink({
>+                   addon: theme, 
>+                   link: "undo"
>+                 });
>+  controller.assertNode(undoLink);

Switch the undo and restart tests. Once we can retrieve the restart button we will click on it for a restart.

>-// testCheckThemeChanged.meta = {litmusids : [8168]};

Looks like we miss the uninstallation of the theme. Can you please add those two functions to the test? That would make it 100% complete. Otherwise great work. It looks so much cleaner now.

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Fixes from prior review

Comment 16

[Aaron Train [:aaronmt]]

Comment on attachment 481938 [details] [diff] [review]
Patch v2 - (default)

Going to hold off on review and adjust the patch to include proper new API for door hanger notification check, as well as bug 599771

Comment 17

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 18

[Aaron Train [:aaronmt]]

Don't have time available to rewrite/unbitrot 4.0 addon tests. Opening up assignment.

Comment 19

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

This test has been rewritten on bug 677193 -> invalid.