680918 - Mozmill test for checking if restartless extension works after restart

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Tracking creation of new mozmill test that check if a restartless extension works after restart.

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

Comment 1

[u415141]

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

Comment 2

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

We can use the following test extension as example:
http://www.mozqa.com/data/firefox/addons/extensions/restartless.xpi

Probably we will have to add some more code to ensure the extension is really working, e.g. a menubar entry.

Comment 3

[Remus Pop (:RemusPop)]

We need the local extension.

Comment 4

[Remus Pop (:RemusPop)]

Attachment: initial patch

In test1.js we install the extension and check if it has been installed.
In test2.js we right-click in a blank page to get to the context menu which holds an additional item created by our extension. Clicking the item takes us to the QMO page and then we check if the loaded page is the QMO one.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 562387 [details] [diff] [review]
initial patch

>+const LOCAL_INSTALL_SCRIPT = "addons/install.html?addon=";
It's not a script you're pointing to, it's a file. Please rename the const

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../../data/');
Can't we add data/addons directly here? Why are we adding the whole data directory? Do we need it further in the test?

>+const TIMEOUT_DOWNLOAD = 25000;
You don't need extended download time here, you're not using an internet connection when downloading the addon.

>+const ADDON = {
>+  name: "Restartless Addon",
Where are you using the name in the test? I can't see it.

>+  // Set category to 'Appearance'
>+  addonsManager.setCategory({
>+    category: addonsManager.getCategoryById({id: "extension"})
>+  });
You're not setting the category to appearance here. Modify the comment appropriately.

>+  expect.ok(addonIsInstalled, "The addon is successfully installed");
>+}
I feel this should be an assert, the test should fail immediatly if the addon did not install. Anthony, what's your opinion here?

>+  // Open content area context menu
>+  controller.rightClick(new elementslib.XPath(controller.tabs.activeTab,
>+                        "/html"));
Assign the new element here before using it.

Comment 6

[Remus Pop (:RemusPop)]

Attachment: patch v2

Addressed all requested changes, including change of expect to assert.

Comment 7

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 563328 [details] [diff] [review]
patch v2

>+const TIMEOUT_DOWNLOAD = 5000;
you don't need this here. waitForDialog has a default timeout of 5000.

>+  md.waitForDialog(TIMEOUT_DOWNLOAD); 
Your specified timeout is equal to the default. Just use "md.waitForDialog();"

>+  // Click the item from the context menu to open QMO page and close the 
>+  // context menu
>+  controller.click(contextMenuItem);
>+  utils.closeContentAreaContextMenu(controller);
>+  controller.waitForPageLoad();
I think you should split this in two blocks like this:
>+  // Click the item from the context menu to open QMO page 
>+  controller.click(contextMenuItem);
>+
>+  // Close the context menu
>+  utils.closeContentAreaContextMenu(controller);
>+  controller.waitForPageLoad();

>+  // Verify that the loaded is contained within QMO url
>+  controller.waitFor(function () { 
>+    return EXPECTED_URL.indexOf(locationBar.getNode().value) !== -1;
>+  }, "Current URL should be contained within QMO url - got " +
>+     locationBar.getNode().value + ", expected " + EXPECTED_URL);
>+}
Why are you using here a waitFor and not an assert?

Comment 8

[Remus Pop (:RemusPop)]

Attachment: patch v3

Addressed all issues and used expect instead of waitFor. Also used prefs to show the full url in the location bar.

Comment 9

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 563400 [details] [diff] [review]
patch v3

I'm going to r+ on this but hold off until the restartless addon lands in the repo before submitting for review further to Anthony.

Comment 10

[u279076]

Comment on attachment 563400 [details] [diff] [review]
patch v3

>+++ b/tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test1.js
>+/**
>+ * Test installing a restartless addon
>+ */
>+function testInstallAddon() {

testInstallRestartlessExtension()

>+  assert.ok(addonIsInstalled, "The addon is successfully installed");

Please include the add-on ID in the assertion.

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

>+var EXPECTED_URL = "http://quality.mozilla.org/";

>+/**
>+ * Test that verifies the addon works after browser restart
>+ */
>+function testThemeIsInstalled() {

testRestartlessExtensionWorksAfterRestart()

>+  // Context menu item that is provided by the restartless extension
>+  var contextMenuItem = new elementslib.ID(controller.window.document,
>+                                       "[e-mail]" +
>+                                       "-context-menu-item-0");

Use the addon ID instead of a hard-coded string.

>+  // Verify that the loaded url matches QMO url
>+  expect.equal(locationBar.getNode().value, EXPECTED_URL,
>+               "Current URL should match QMO url");

Use the EXPECTED_URL in the message so we know the value we expect.
Also, since it's our final check, this should be an assert.

Comment 11

[Remus Pop (:RemusPop)]

Attachment: patch v4

Addressed all requests except the last one. The message already includes EXPECTED_URL: "Current URL should match QMO url - '\"https://quality.mozilla.org/\" should equal '\"https://quality.mozilla.org/\"

Comment 12

[u279076]

Comment on attachment 574593 [details] [diff] [review]
patch v4

Canceling review of this patch due to bug 684867 comment 36

Comment 13

[Remus Pop (:RemusPop)]

Attachment: patch v5

All dependencies were resolved so I updated the patch.

Comment 14

[u279076]

Comment on attachment 577260 [details] [diff] [review]
patch v5

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

We've used "addon" in the past (instead of "anAddon") -- let's not change that.

Comment 15

[Remus Pop (:RemusPop)]

Attachment: patch v6:all [checked-in]

We're now using addon instead of anAddon variable.

Comment 16

[u279076]

Comment on attachment 579007 [details] [diff] [review]
patch v6:all [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d63e3d87117a (default)

Please check tomorrow's testrun to verify if this can be ported to other branches.

Comment 17

[Remus Pop (:RemusPop)]

Can be landed on all branches because it hasn't failed.

Comment 18

[u279076]

Comment on attachment 579007 [details] [diff] [review]
patch v6:all [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/17dad272c91f (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/d251fde20f08 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/9fff7143f494 (mozilla-release)

Comment 19

[u279076]

Patch has been landed across all branches -- please verify with tomorrow's testruns.

Comment 20

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

I assume this will also fail regarding all the other test failures we currently have for add-ons tests.

Comment 21

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> I assume this will also fail regarding all the other test failures we
> currently have for add-ons tests.

Potentially. Remus keep an eye on it and block this on bug 710143 if it does.

Comment 22

[Remus Pop (:RemusPop)]

Hasn't failed since 17th of Decemeber so marking this as VERIFIED.