599865 - Update restart test module testExtensionInstallUninstall to reflect the new Add-ons API

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Comment 2

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

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

>+  persisted.extensionURL = "https://addons.mozilla.org/en-US/firefox/addon/5428/";
>+  persisted.extensionName = "Mozilla QA Companion";
>+  persisted.extensionId = "{667e9f3d-0096-4d2b-b171-9a96afbabe20}";

Use preview.addons.mozilla.org for the installation. Also please bundle it under a single object.

>+  controller.waitForEval("subject.installAddonButtonClass.indexOf('installer') != -1", TIMEOUT, 100, {
>+    installAddonButtonClass: installAddonButton.getNode().getAttribute('class')
>+  });

Please also use waitFor.

>+  // Wait until the extension is installed notification
>+  var installNotification = new elementslib.ID(
>+                              controller.window.document, 
>+                              "addon-install-complete-notification"
>+                            );
>+  controller.waitForElement(installNotification);

Please add a note that it depends on the implementation of the door hanger for the tabbrowsing API. Also add this bug to the dependency list.

>+  // Switch to the Extensions pane
>+  var category = addonsManager.getCategoryById({id: "extensions"});
>+  addonsManager.setCategory({category: category});

Please use setCategoryById.

>+  // Check some properties of the ready to be installed extension
>+  var addon = addonsManager.getAddons({
>+                attribute: "name", 
>+                value: persisted.extensionName
>+              })[0];

Use the guid here.

>+  // Check if the restart and undo links and remove button are present
>+  // XXX: Bug 599771
>+  //var restartLink = am.getAddonLink({addon: addon, link: "restart"});
>+  //controller.assertNodeExist(restartLink);
>+  var undoLink = addonsManager.getAddonLink({
>+                   addon: addon,
>+                   link: "undo"
>+                 });
>+  controller.assertNode(undoLink);

We will check the undo link in another test. Please also note that we want to restart Mozmill from the test once the above bug has been fixed.

>+  controller.assertJS("subject.extensions[0].name == subject.extensionName", {
>+    extensions: itemElem.childNodes, 
>+    extensionName: persisted.extensionName
>+  });

While we are modifying those instances please use assert overall for those tests. 

> /**
>  * Map test functions to litmus tests
>  */

Please remove those lines.

>+++ b/firefox/restartTests/testExtensionInstallUninstall/test2.js

>+  // Switch to the extensions category
>+  var category = addonsManager.getCategoryById({id: "extensions"});
>+  addonsManager.setCategory({category: category});

setCategoryById again please.

>+                value: persisted.extensionId
>+              })[0];
>+  // Verify that the Add-on is installed and enabled

nit: please add a blank line.

>+var testUninstallExtension = function() {
[..]
>+  // Switch to the extensions category
>+  var category = addonsManager.getCategoryById({id: "extensions"});
>+  addonsManager.setCategory({category: category});

snip

>+  // Verify that a remove button is visible on the installed extension
>+  var addon = addonsManager.getAddons({
>+                attribute: "name", 
>+                value: persisted.extensionName
>+              })[0];

Please use the guid.

>+  var removeButton = addonsManager.getAddonButton({
>+                       addon: addon,
>+                       button: "remove"
>+                     });
>+  controller.assertNode(removeButton);
>+  
>+  // Click the remove button
>+  controller.click(removeButton);

No need to assertNode when you click on it in either way.

>+  // Verify that the restart and undo links are visible
>+  var undoLink = addonsManager.getAddonLink({
>+                    addon: addon,
>+                    link: "undo"
>+                 });
>+  //controller.waitForElement(undoLink); 'elem error'
>+  //controller.assertNode(undoLink);
>+  
>+  //var restartLink = am.getAddonLink({addon: addon, link: "restart"});
>+  //controller.waitForElement(restartLink);
>+  //controller.assertNodeExist(restartLink);

Please add the bug numbers as reference.

>-/**
>  * Map test functions to litmus tests
>  */
> // testCheckInstalledExtension.meta = {litmusids : [7972]};

Please remove

>+var testCheckUninstalledExtension = function() {
[..]
>+  // Switch to the extensions category
>+  var category = addonsManager.getCategoryById({id: "extensions"});
>+  addonsManager.setCategory({category: category});

snip

>+  // Confirm that the extension is not installed
>+  var addonLength = addonsManager.getAddons({
>+                          attribute: "value", 
>+                          value: persisted.extensionId
>+                          })[0];
>+  controller.assertNodeNotExist(addonLength);

Have you run this too? The [0] definitely throws an error. There is no child to index if the add-on hasn't been found. getAddons().length is simply 0.

> /**
>  * Map test functions to litmus tests
>  */

Remove this too.

Comment 3

[Aaron Train [:aaronmt]]

Changes made. Patch coming pending this question. The reason I switched from preview to AMO was because I could not find a compatible add-on for the test (4.0b7pre). On Preview, they don't have 4.0b7pre as a selectable option yet. What do you suggest? On AMO, I changed QAC to support 4.0b7pre and was using that in the test.

Comment 4

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

Ok, so lets use the public site for now. But we should create and upload our own test add-ons we can use for testing and which do not increase the download count. That was one of the reasons we switched to preview.amo. While working on AOM Litmus tests I will have to create some sample extensions/themes. We can flip later.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Comment 6

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

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

>+const RELATIVE_ROOT = '../../../shared-modules';
>+const MODULE_REQUIRES = ['AddonsAPI', 'ModalDialogAPI', 'TabbedBrowsingAPI'];

>+const ADDON = {

Please use EXTENSION.

>+  url: "https://addons.mozilla.org/en-US/firefox/addon/5428/",
>+  name: "Mozilla QA Companion",
>+  id: "{667e9f3d-0096-4d2b-b171-9a96afbabe20}"

Can you order it alphabetically please?

>+var setupModule = function() {
[..]
>+  addonsManager = new AddonsAPI.addonsManager(controller);
>+  
>   TabbedBrowsingAPI.closeAllTabs(controller);
>+  
>+  // Store the addon object in 'persisted.addon'
>+  persisted.addon = ADDON;

Please put code in logical blocks. closeAllTabs interrupts the flow. 

>+  // Open the web page for the Mozilla QA Companion extension directly

Don't name the extension itself. It will make more work once we have to update the test.

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

function should be moved up behind waitFor.

>+  // Wait until the extension is installed notification
>+  // XXX: Depends on the implementation of the door hanger API in the TabbedBrowsingAPI 
>+  // Bug 599682

Ok, looks like I was wrong by reviewing the theme installation patch. I will have to take a look at this.

>+  controller.waitForElement(installNotification);

Here we need the download timeout.

>+  // 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);

Flip those checks. Once restart is available we will trigger a restart.

>   controller.waitForElement(itemList, TIMEOUT);
> 
>   // There should be listed only one extension
>-  controller.assertJS("subject.extensions.length == 1",
>-                      {extensions: itemElem.childNodes});

Can we please get rid of itemElem in all of the tests? It's confusing. That means:

>  var itemElem = controller.window.document.getElementById("itemList");
>  var itemList = new elementslib.Elem(controller.window.document, itemElem);

Directly instantiate itemList via elementslib.ID and name the element installItems.

>+  controller.assert(function() { 
>+    return (itemElem.childNodes.length == 1) == true;
>+  }, "Listed extensions should only be one" );

No need for "== true". Same for other lines.

>   controller.waitForEval("subject.disabled != true", 7000, 100, installButton.getNode());
>   controller.click(installButton);

Please use waitFor for all instances.

>+  // Verify that the extension installed and enabled
>+  controller.assert(function() {
>+    return addonsManager.isAddonInstalled({addon: addon}) == true;                  
>+  }, "Add-on should be installed");
>+  
>+  controller.assert(function() { 
>+    return addonsManager.isAddonEnabled({addon: addon}) == true;                  
>+  }, "Add-on should be enabled");

"true"

>+  // Click the remove button on the installed extension

nit: s/on/of/

>+  var removeButton = addonsManager.getAddonButton({
>+                       addon: addon,
>+                       button: "remove"
>+                     });
>+  // Click the remove button
>+  controller.click(removeButton);

Use .removeAddon()

>+  // Verify that the restart and undo links are visible
>+  var undoLink = addonsManager.getAddonLink({
>+                    addon: addon,
>+                    link: "undo"
>+                 });

Btw. those are buttons and are only styled as links in the default theme.

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

We will not only check for presence but also click on it.

>-/**
>- * Handle the Software Un-installation dialog
>- */
>-var handleTriggerDialog = function(controller)

Yay!

>+var testCheckUninstalledExtension = function() {
[..]
>+  // Confirm that the extension is not installed
>+  addonsManager.search({value: persisted.addon.name});
>+  addonsManager.selectedSearchFilter = "remote";

Wow, please save this step for another test. :) Please really use the extensions pane to check if the extension is not listed there.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Fixes from prior review, and simplified the uninstall check.

Comment 8

[Aaron Train [:aaronmt]]

Comment on attachment 481936 [details] [diff] [review]
Patch v3 - (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 9

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

[Aaron Train [:aaronmt]]

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

Comment 11

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

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