674220 - Mozmill test for installing a multi-package xpi

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

Tracking of mozmill test creation for installing multi-package xpi

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

Comment 1

[u415141]

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

Comment 2

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

A multi-package add-on is already available in the litmus-data repository.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

The elementslib.Link will change once Bug 672763 lands. Is everything else ok?

Comment 4

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

Comment on attachment 549115 [details] [diff] [review]
patch v1.0

Alex, it's still Anthony who is the first reviewer of all your tests.

Comment 5

[u279076]

Comment on attachment 549115 [details] [diff] [review]
patch v1.0

>+const SITE = "http://mozqa.com";

Please call this ADDON_DOMAIN

>+/**
>+ * Installs multipackage addons
>+ */

"Test installing a multi-package add-on"

>+++ b/tests/functional/restartTests/testAddons_installMultipackage/test2.js

testAddons_installMultipackageAddon

>+/**
>+ * Verifies the addons are installed
>+ */

"Tests post-installation of a multi-package add-on"

>+  var theme = addonsManager.getAddons({attribute: "value", value: ADDONS[0].id})[0];

Are you trying to get the first theme in the list? If so, I think we should move this into a helper method in the API. Something like addonsManager.getFirstAddon(aAddonID). What do you think?

>+  expect.ok(themeIsInstalled, "Theme has been installed");

I'd like to see the addonID integrated into the message. 
Something like addonName + " theme has been installed"

>+  //Verify the addon is installed
>+  addonsManager.setCategory({category: addonsManager.getCategoryById({id: "extension"})});
>+
>+  var addon = addonsManager.getAddons({attribute: "value", value: ADDONS[1].id})[0];
>+  var addonIsInstalled = addonsManager.isAddonInstalled({addon: addon});
>+
>+  expect.ok(addonIsInstalled, "Add-on has been installed");

Same comments as the previous two.

Comment 6

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> >+  var theme = addonsManager.getAddons({attribute: "value", value: ADDONS[0].id})[0];
> 
> Are you trying to get the first theme in the list? If so, I think we should
> move this into a helper method in the API. Something like
> addonsManager.getFirstAddon(aAddonID). What do you think?

It wouldn't be such a helpful method because:

* It limits the queries to only the ids
* It would not scale. Using an index should be the reason why a new method is immediately needed
* We dont' want to rewrite the addons module in the old system. So we would like to keep it.

Does that sound ok?

> >+  //Verify the addon is installed
> >+  addonsManager.setCategory({category: addonsManager.getCategoryById({id: "extension"})});

This should be setCategoryById

Comment 7

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> > >+  var theme = addonsManager.getAddons({attribute: "value", value: ADDONS[0].id})[0];
> > 
> > Are you trying to get the first theme in the list? If so, I think we should
> > move this into a helper method in the API. Something like
> > addonsManager.getFirstAddon(aAddonID). What do you think?
> 
> It wouldn't be such a helpful method because:
> 
> * It limits the queries to only the ids
> * It would not scale. Using an index should be the reason why a new method
> is immediately needed
> * We dont' want to rewrite the addons module in the old system. So we would
> like to keep it.
> 
> Does that sound ok?

I can agree with this, as long as we comment the code so that it is clear what the code does. If I have to ask what the code does, chances are a newcomer will be clueless as well.

> > >+  //Verify the addon is installed
> > >+  addonsManager.setCategory({category: addonsManager.getCategoryById({id: "extension"})});
> 
> This should be setCategoryById

What should? "setCategory" or "getCategoryById"?

Comment 8

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Changed to use local addons and made the previous requested changes, where still applicable

Comment 9

[u279076]

Comment on attachment 566561 [details] [diff] [review]
patch v2.0

>+const LOCAL_INSTALL_FILE = "install.html?addon=extensions/multipackage.xpi"
>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/addons/");
>+const MULTIPACKAGE_URL = LOCAL_TEST_FOLDER + LOCAL_INSTALL_FILE;

No need for the LOCAL_INSTALL_FILE constant here -- just use the string.

>+const ADDON_ID = "[e-mail]";
>+const THEME_ID = "[e-mail]";

Put the URL, ADDON_ID, and THEME_ID into an array

>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  addonsManager = new addons.AddonsManager(controller);
>+
>+  // Whitelist add localhost
>+  addons.addToWhiteList(LOCAL_TEST_FOLDER);
>+
>+  // Store the ids in persisted
>+  persisted.addonId = ADDON_ID;
>+  persisted.themeId = THEME_ID;
>+
>+  tabs.closeAllTabs(controller);
>+}
>+
>+/**
>+ * Test installing a multi-package add-on
>+ */
>+function testInstallMultipackageExtension() {

Since this "extension" contains an add-on and a theme, let's use "extension" in the naming of files, functions, and header comments.

Comment 10

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

made changes as requested

Comment 11

[u279076]

Comment on attachment 568361 [details] [diff] [review]
patch v3.0

>+const MULTIPACKAGE = [

Call this MULTIPACKAGE_ADDON

>+  var aAddon = addonsManager.getAddons({attribute: "value", value: persisted.extension.addonId})[0];

Call this addon

>+  var aTheme = addonsManager.getAddons({attribute: "value", value: persisted.extension.themeId})[0];

Call this theme

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

Renamed variables and const

Comment 13

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

Comment on attachment 570273 [details] [diff] [review]
patch v4.0

>+const MULTIPACKAGE_ADDON = [
>+  {url: LOCAL_TEST_FOLDER + "install.html?addon=extensions/multipackage.xpi",
>+   addonId: "[e-mail]",
>+   themeId: "[e-mail]"}

Similar to other tests we should install the XPI directly and make not use of the install page. 

>+  // Whitelist add localhost
>+  addons.addToWhiteList(LOCAL_TEST_FOLDER);

Why the whole folder and not only the domain vs. 'localhost'?

>+function teardownModule() {
>+  addonsManager.close();
>+
>+  delete persisted.extension;

Please flip both lines. .close() could fail and skip the removal of the persisted property. I would even say lets skip the close call. It's really not necessary here, because it's a restart test.

>+  // Verify the addon is installed
>+  var addon = addonsManager.getAddons({attribute: "value", value: persisted.extension.addonId})[0];
>+  var addonIsInstalled = addonsManager.isAddonInstalled({addon: addon});

This is not only an add-on but explicitly an extension.

>+  expect.ok(addonIsInstalled, "Add-on " + persisted.extension.addonId + " has been installed");

Please add single quotes around the extension and theme id.

Comment 14

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

Also please check the expected results from the Litmus test again. It seems like we miss some parts. If those are necessary or not please clarify with Dave Townsend.

Comment 15

[u279076]

Can we get a status update on this bug?

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.