684679 - Mozmill test for installing and uninstalling a hardblocked extension

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Remus Pop (:RemusPop)]

Tracking creation of a mozmill test for installing and uninstalling a blocklisted extension.

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

Comment 1

[u415141]

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

Comment 2

[Remus Pop (:RemusPop)]

We might have a problem regarding this testcase. 
When we first install an extension it is disabled by default until we restart the browser.
Even if we blocklist it the extension will be enabled after restart.

Comment 3

[u279076]

So is the Litmus testcase broken?

Comment 4

[Remus Pop (:RemusPop)]

I am not sure what's the normal behavior for stuff that is blocklisted. We need a confirmed opinion on this testcase.

Comment 5

[u279076]

(In reply to Remus Pop from comment #4)
> I am not sure what's the normal behavior for stuff that is blocklisted. We
> need a confirmed opinion on this testcase.

CCing Dave Townsend to see if he can help answer this question.

Comment 6

[Remus Pop (:RemusPop)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> CCing Dave Townsend to see if he can help answer this question.

Great!
I have the code for this test but I observed this in the last minute, so I will be really happy to deliver this as soon as possible.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> (In reply to Remus Pop from comment #4)
> > I am not sure what's the normal behavior for stuff that is blocklisted. We
> > need a confirmed opinion on this testcase.
> 
> CCing Dave Townsend to see if he can help answer this question.

Stuff that is blocklisted should never be enabled, if it is then it is a bug, please file it with STR

Comment 8

[Remus Pop (:RemusPop)]

Actually this is working just fine, but we forgot to ping the blocklist so no changes were made.

Comment 9

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

Dave, I have a question re the litmus test from comment 0. Does it make a difference when we ping the blocklist? Currently this test covers install and uninstall of a blocklisted extension. Realistically that doesn't work if we ping the blocklist before we install the example extension. So would it be enough to install the extension and then ping the blocklist? If not, we would have to create another Litmus test.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Dave, I have a question re the litmus test from comment 0. Does it make a
> difference when we ping the blocklist? Currently this test covers install
> and uninstall of a blocklisted extension. Realistically that doesn't work if
> we ping the blocklist before we install the example extension. So would it
> be enough to install the extension and then ping the blocklist? If not, we
> would have to create another Litmus test.

The litmus test doesn't seem right to me. If Firefox pings the blocklist before you attempt install, the add-on should fail to install completely. If not it will install and remain enabled until Firefox next pings the blocklist.

Comment 11

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

(In reply to Dave Townsend (:Mossop) from comment #10)
> The litmus test doesn't seem right to me. If Firefox pings the blocklist
> before you attempt install, the add-on should fail to install completely. If
> not it will install and remain enabled until Firefox next pings the
> blocklist.

Currently we ping the blocklist after the extension has been installed and before the browser gets restarted. This has not been correctly updated in the Litmus test. Remus, please ensure that this steps gets added so it is identical to the spreadsheet.

Dave, question is if the above case is enough, or if we should have another test, which pings the blocklist before the attempt to install an extension.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Dave Townsend (:Mossop) from comment #10)
> > The litmus test doesn't seem right to me. If Firefox pings the blocklist
> > before you attempt install, the add-on should fail to install completely. If
> > not it will install and remain enabled until Firefox next pings the
> > blocklist.
> 
> Currently we ping the blocklist after the extension has been installed and
> before the browser gets restarted. This has not been correctly updated in
> the Litmus test. Remus, please ensure that this steps gets added so it is
> identical to the spreadsheet.
> 
> Dave, question is if the above case is enough, or if we should have another
> test, which pings the blocklist before the attempt to install an extension.

Yeah I think we should have that too

Comment 13

[Remus Pop (:RemusPop)]

The Litmus testcase is not correct. After pinging the blocklist the extension is not disabled immediately, but instead we get a modal dialog with a restart later and restart now buttons. We have to restart the browser in order to disable it completely. 
Step 6 has the same problem: we need to restart after it in order to get the extension completely removed.

I would appreciate it if someone would get this testcase right.

Comment 14

[Remus Pop (:RemusPop)]

We need the local extension.

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Remus Pop (:RemusPop) from comment #13)
> The Litmus testcase is not correct. After pinging the blocklist the
> extension is not disabled immediately, but instead we get a modal dialog
> with a restart later and restart now buttons. We have to restart the browser
> in order to disable it completely. 
> Step 6 has the same problem: we need to restart after it in order to get the
> extension completely removed.
> 
> I would appreciate it if someone would get this testcase right.

Remus requested some help with modifying https://litmus.mozilla.org/show_test.cgi?id=16326 
This will not be a problem at all, but my concern is about what would be the correct behavior here. Can someone confirm that Remus's listed behavior in the comment is the right one ? 

Only then we will modify the testcase. Thanks

Comment 16

[u279076]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #15)
> Can someone confirm that Remus's listed behavior in
> the comment is the right one ? 

Henrik or Dave?

Comment 17

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

(In reply to Remus Pop (:RemusPop) from comment #13)
> The Litmus testcase is not correct. After pinging the blocklist the
> extension is not disabled immediately, but instead we get a modal dialog
> with a restart later and restart now buttons. We have to restart the browser
> in order to disable it completely. 
> Step 6 has the same problem: we need to restart after it in order to get the
> extension completely removed.

That's right. After you have selected that the extension will be disabled by this dialog, it will be noted in the list of extensions that it is blocklisted. A restart is necessary, that's correct.

Comment 18

[Remus Pop (:RemusPop)]

How do we approach the restart dialog?

Click the Restart Now button or click Restart Later and use restart from the mozmill when executing the next testX.js where X is a number?

Comment 19

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

If you would click on the restart button, we couldn't check the state of the extension in the add-ons manager. So no, don't restart immediately.

Comment 20

[Remus Pop (:RemusPop)]

Attachment: patch v1

test1.js installs an extensions (icons.xpi) and sets the preference for the blocklist to use our custom blocklist.xml. It needed TIMEOUT_DOWNLOAD because I would constantly get the error "Modal dialog has been found...".

test2.js checks if the addon has been installed and pings the blocklist. After pinging, a window will open telling us the our extension is blocklisted. We check if the extensions is listed in that window.

test3.js checks if the extensions is disabled and removes it. We have to wait for the last extension to be listed. Otherwise we would get an "addon not specified error".

test4.js checks if the extension has been completely removed.

Comment 21

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 569912 [details] [diff] [review]
patch v1

Sorry it took so long, this bug seems to have fallen through my review querry.


>+ * The Initial Developer of the Original Code is the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
It's 2012 now. Modify this for all test files in the patch.

>+  // Ping the blocklist
>+  var bs = Components.classes["@mozilla.org/extensions/blocklist;1"]
>+           .getService(Components.interfaces.nsIBlocklistService);
>+  bs.QueryInterface(Components.interfaces.nsITimerCallback).notify(null);
Make variable names more readable. Rename bs to blocklistService.

>+    try {
>+      bkController = new mozmill.controller.MozMillController(mozmill.utils.getWindowByType("Addons:Blocklist"));
>+      return !!bkController;
Why are you using double negative here? !!bkController = bkController

Comment 22

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Addressed all requests.

!! is used to cast to boolean type.

Comment 23

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 593431 [details] [diff] [review]
patch v2 (all branches)

>+function testAddonisUninstalled() {
Missed this, sorry. This should be "testAddonIsUninstalled"
After you have fixed this nit you can ask from r? from Anthony.

Comment 24

[Remus Pop (:RemusPop)]

Attachment: patch v3 (all branches)

Fixed the nit.

Comment 25

[u279076]

Comment on attachment 593765 [details] [diff] [review]
patch v3 (all branches)

>+++ b/tests/functional/restartTests/testAddons_installUninstallBlocklisted/test1.js

Call it testAddons_installUninstallBlocklistedExtension

>+ * Test that installs the extension

Too vague -- make it specific to this test

>+function testInstallBlocklistedAddon() {

testInstallBlocklistedExtension()

>+var addons = require("../../../../lib/addons");
>+var {assert, expect} = require("../../../../lib/assertions");

nit: put the {a,e} line first

>+ * Test that blocklists the extension

Test the extension has been blocklisted
 
>+  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
>+            persisted.addon.id + " is installed");

If the addon.id doesn't contain the string "Blocklisted" we should include the word in the message. I would expect the message to be something like "Blocklisted add-on 'addon name' is installed". 

>+
>+  // Ping the blocklist
>+  var blocklistService = Components.classes["@mozilla.org/extensions/blocklist;1"]
>+                         .getService(Components.interfaces.nsIBlocklistService);

Truncate after the '.', not before

>+  // Get the controller for the blocklist window
>+  var bkController;
>+  controller.waitFor(function () {
>+    try {
>+      bkController = new mozmill.controller.MozMillController(mozmill.utils.getWindowByType("Addons:Blocklist"));
>+      return !!bkController;
>+    }
>+    catch (err) {
>+      return false;
>+    }
>+  }, "Blocklist window has been opened");

I think all of this should be contained in a helper method. An argument could be made for adding it to the shared module, but simply move it to a helper function in this test for now.

>+  // Check if the extension name is shown
>+  var nodeCollector = new domUtils.nodeCollector(bkController.window);
>+  nodeCollector.queryNodes("#addonList");
>+  nodeCollector.root = nodeCollector.nodes[0];
>+  nodeCollector.queryNodes(".hardBlockedAddon");

Same as this -- we should have a helper method that returns the .hardBlockedAddon property, or at the very least the root node of the addonList.

>+  expect.equal(nodeCollector.elements[0].getNode().getAttribute("name"),
>+               persisted.addon.name,
>+               "Extension name is shown in the blocklisted window");
>+}

Include the add-on name in the message.

>+/**
>+ * Test that removes the extension
>+ */

Test uninstalling a blocklisted extension

>+function testRemoveAddon() {

testUninstallBlocklistExtension()

>+  addonsManager.open();
>+
>+  // Wait for the last addon to appear
>+  controller.waitFor(function() {
>+    try {
>+      var lastAddon = new elementslib.Selector(controller.window.document,
>+                                               ".addon:last-child");
>+      return lastAddon.getNode().getAttribute("type") == "extension";
>+    }
>+    catch (err) {
>+      return false;
>+    }
>+  }, "Last addon has been listed in Addons Manager");

Not sure how this is supposed to work. In the try you waitFor() a string, while in catch you waitFor() a boolean. In this case, just waitFor type == "extension" should suffice.

>+  var addon = addonsManager.getAddons({attribute: "name",
>+                                      value: persisted.addon.name})[0];
>+
>+
>+  // Check that the addon is disabled

Too many spaces.

>+  expect.ok(!addonsManager.isAddonEnabled({addon: addon}),
>+            persisted.addon.id + "is disabled");

Make sure there is space between "is" otherwise the otherwise the message will appear as "addonIDis". Also, I would like the message to appear like "Blocklisted 'addonID' Extension is disabled"


>+/**
>+ * Test that the extension has been uninstalled
>+ */

Test the blocklisted extension has been uninstalled

>+function testAddonIsUninstalled() {

testBlocklistExtensionUninstalled()

>+  addonsManager.open();
>+
>+  var theme = addonsManager.getAddons({attribute: "value", 
>+                                       value: persisted.addon.id});

Is this a theme or an add-on? Please be consistent.

>+  assert.equal(theme.length, 0, persisted.addon.id + " is uninstalled");

Please mention that we are dealing with a Blocklisted Extension in the message.

Comment 26

[Remus Pop (:RemusPop)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #25)
> Comment on attachment 593765 [details] [diff] [review]
> patch v3 (all branches)
> 
> If the addon.id doesn't contain the string "Blocklisted" we should include
> the word in the message. I would expect the message to be something like
> "Blocklisted add-on 'addon name' is installed". 

We can't include it because the addon is not blocklisted yet. It gets blocklisted after pinging.

> >+  // Wait for the last addon to appear
> >+  controller.waitFor(function() {
> >+    try {
> >+      var lastAddon = new elementslib.Selector(controller.window.document,
> >+                                               ".addon:last-child");
> >+      return lastAddon.getNode().getAttribute("type") == "extension";
> >+    }
> >+    catch (err) {
> >+      return false;
> >+    }
> >+  }, "Last addon has been listed in Addons Manager");
> 
> Not sure how this is supposed to work. In the try you waitFor() a string,
> while in catch you waitFor() a boolean. In this case, just waitFor type ==
> "extension" should suffice.

That is not waiting for a string, but a boolean too because I evaluate an expression with ==. Try-catch is needed because the Selector method does not wait for the element, so getNode() in lastAddon would output an error that getNode() is not defined because the node does not exist yet.

Comment 27

[Remus Pop (:RemusPop)]

Attachment: patch v4 (all branches)

Requests were fixed.

Comment 28

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

Selector nor any other element locator is intended to wait for a node. It simply offers an API to retrieve a reference of an element. If you want to wait for an element use controller.waitForElement() instead.

Comment 29

[Remus Pop (:RemusPop)]

Attachment: patch v4.1 (all branches)

Great idea Henrik!
This is an updated patch which uses waitForElement() in test3.

Comment 30

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

Comment on attachment 596020 [details] [diff] [review]
patch v4.1 (all branches)

Just skimmed over the test and I have two notes...

>+  var nodeCollector = new domUtils.nodeCollector(bkWindow);
>+  nodeCollector.queryNodes("#addonList");
>+  nodeCollector.root = nodeCollector.nodes[0];
>+  nodeCollector.queryNodes(".hardBlockedAddon");

Out of interest why is the usage of nodeCollector necessary here? Do we have anonymous elements?

>+function getBlocklistWindow() {
>+  var bkController;
>+  controller.waitFor(function () {
>+    try {
>+      bkController = new mozmill.controller.MozMillController(
>+                       mozmill.utils.getWindowByType("Addons:Blocklist"));
>+      return !!bkController;
>+    }
>+    catch (err) {
>+      return false;
>+    }
>+  }, "Blocklist window has been opened");

You want to use utils.handleWindow instead.

Comment 31

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> Comment on attachment 596020 [details] [diff] [review]
> patch v4.1 (all branches)
> 
> Just skimmed over the test and I have two notes...
> 
> >+  var nodeCollector = new domUtils.nodeCollector(bkWindow);
> >+  nodeCollector.queryNodes("#addonList");
> >+  nodeCollector.root = nodeCollector.nodes[0];
> >+  nodeCollector.queryNodes(".hardBlockedAddon");
> 
> Out of interest why is the usage of nodeCollector necessary here? Do we have
> anonymous elements?

That was the first method that came to my mind

> >+function getBlocklistWindow() {
> >+  var bkController;
> >+  controller.waitFor(function () {
> >+    try {
> >+      bkController = new mozmill.controller.MozMillController(
> >+                       mozmill.utils.getWindowByType("Addons:Blocklist"));
> >+      return !!bkController;
> >+    }
> >+    catch (err) {
> >+      return false;
> >+    }
> >+  }, "Blocklist window has been opened");
> 
> You want to use utils.handleWindow instead.

Thank you for the feedback.
I will update the patch with your suggestions after Anthony reviews this.
So Anthony, please have in mind Henrik's suggestions.

Comment 32

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

(In reply to Remus Pop (:RemusPop) from comment #31)
> > Out of interest why is the usage of nodeCollector necessary here? Do we have
> > anonymous elements?
> 
> That was the first method that came to my mind

As long as we do not have to get anonymous nodes there is no need to use nodeCollector. The elementslib.Selector class will be able to handle that perfectly. Once you can't use a CSS selector it will be the point to think about nodeCollector.

Comment 33

[u279076]

Comment on attachment 596018 [details] [diff] [review]
patch v4 (all branches)

Assuming patch v4.1 makes this one obsolete.

Comment 34

[u279076]

Comment on attachment 596020 [details] [diff] [review]
patch v4.1 (all branches)

>+  // Check if the add-on has been installed
>+  var addon = addonsManager.getAddons({attribute: "value",
>+                                      value: persisted.addon.id})[0];
>+  
>+  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
>+            persisted.addon.id + " is installed");

I think this should be an EXPECT.

>+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
>+               "in the blocklisted window. Expected: " + persisted.addon.name);

I think this should be an ASSERT.

>+/*
>+ * Gets the nodes for .hardBlockedAddon
>+ */
>+function getHardBlockedAddonNode(bkWindow) {
>+  var nodeCollector = new domUtils.nodeCollector(bkWindow);
>+  nodeCollector.queryNodes("#addonList");
>+  nodeCollector.root = nodeCollector.nodes[0];
>+  nodeCollector.queryNodes(".hardBlockedAddon");
>+  return nodeCollector;
>+}

If this is something other tests can benefit from, I would recommend moving it to the shared module.

>+/*
>+ * Gets the blocklist window object
>+ */
>+function getBlocklistWindow() {
>+  var bkController;
>+  controller.waitFor(function () {
>+    try {
>+      bkController = new mozmill.controller.MozMillController(
>+                       mozmill.utils.getWindowByType("Addons:Blocklist"));
>+      return !!bkController;
>+    }
>+    catch (err) {
>+      return false;
>+    }
>+  }, "Blocklist window has been opened");

Same with this, but try using utils.handleWindow() first; as Henrik suggests.

>+// Include required modules
>+var addons = require("../../../../lib/addons");
>+var {expect} = require("../../../../lib/assertions");

Switch these two around

>+// Include required modules
>+var addons = require("../../../../lib/addons");
>+var {assert} = require("../../../../lib/assertions");

Switch these two around

>+  assert.equal(addon.length, 0,
>+               "Blocklisted extension " + persisted.addon.id + " is uninstalled");

expect

Comment 35

[Remus Pop (:RemusPop)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #34)
> Comment on attachment 596020 [details] [diff] [review]
> patch v4.1 (all branches)
> 
> >+  // Check if the add-on has been installed
> >+  var addon = addonsManager.getAddons({attribute: "value",
> >+                                      value: persisted.addon.id})[0];
> >+  
> >+  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> >+            persisted.addon.id + " is installed");
> 
> I think this should be an EXPECT.

This would invalidate the rest of the test because it relies on the existence of the addon and assert stops the test.

> >+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
> >+               "in the blocklisted window. Expected: " + persisted.addon.name);
> 
> I think this should be an ASSERT.

This does not invalidate the rest of the test. The test can run safely if this fails.

Comment 36

[u279076]

Crap, you are right, sorry. I keep getting those backwards. I will re-review the patch. Please nominate it.

Comment 37

[Remus Pop (:RemusPop)]

No problem. I will soon update the patch and then ask for review.

Comment 38

[Remus Pop (:RemusPop)]

Attachment: patch v5 (all branches)

Used handleWindow and elementslib.Selector in test2 and changed the order of includes.
Asserts and expects are left as they were before.

Comment 39

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

Comment on attachment 597413 [details] [diff] [review]
patch v5 (all branches)

Sorry for stealing the review but there are some thing I want to mention...

>+const BLOCKLIST_URL = "http://www.mozqa.com/data/firefox/addons/blocklist/" + 
>+                      "softblock_extension/blocklist.xml";

Why do we make use of a remote URL? This should be a local one whenever we are able to. And this is the case here.

>+  persisted.blocklistPref = "extensions.blocklist.url"

Please define the value as a constant and assign the constant to the persisted object. Reason is that we only want to check the top of a test to make modifications. This is hidden.

>+  prefs.preferences.setPref(persisted.blocklistPref, BLOCKLIST_URL);

This should be moved to setupModule. It's a setup routine.

>+  md.waitForDialog(TIMEOUT_DOWNLOAD);

Why do we need this timeout? We have a locally hosted extension.

>+var {assert, expect} = require("../../../../lib/assertions");
>+var addons = require("../../../../lib/addons");

Anthony, any reason why you wanted to have this flipped? It's not in alphabetical order anymore.

>+var TIMEOUT = 10000;

>+  var blocklistService = Components.classes["@mozilla.org/extensions/blocklist;1"].
>+                         getService(Components.interfaces.nsIBlocklistService);
>+  blocklistService.QueryInterface(Components.interfaces.nsITimerCallback).notify(null);

Please make use of Cc and Ci instead of Components.classes and Components.interfaces. Also I would consider to move this into the addons module.

>+  var bkWindow = utils.handleWindow("type", "Addons:Blocklist", false, false).window;

Absolutely not sure what this code is trying to do. The handleWindow function does NOT return a window. This should always throw an exception in the next test:

>+  var extensionName = new elementslib.Selector(bkWindow.document, ".hardBlockedAddon").
>+                      getNode().getAttribute("name");
>+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
>+               "in the blocklisted window. Expected: " + persisted.addon.name);

Use a callback to handle that code.

>+var {expect} = require("../../../../lib/assertions");
>+var addons = require("../../../../lib/addons");

Same as above. Question for Anthony.

>+function testUninstallBlocklistExtension() {
>+  addonsManager.open();
>+
>+  // Wait for the last addon to appear
>+  controller.waitForElement(new elementslib.Selector(controller.window.document,
>+                                                     ".addon:last-child"));

So where is this element you are querying for? Add-ons Manager? Then add an element selector to getElement. Also this should be two separate commands.

>+  delete persisted.addon;
>+  delete persisted.blocklistPref;

I would propose to use an object in test1 to store all the data, so you only would have to delete a single element here.

>+  var addon = addonsManager.getAddons({attribute: "value", 
>+                                       value: persisted.addon.id});

The variable name should be 'addons' because getAddons returns a list.

Comment 40

[u279076]

(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #39)
> >+var {assert, expect} = require("../../../../lib/assertions");
> >+var addons = require("../../../../lib/addons");
> 
> Anthony, any reason why you wanted to have this flipped? It's not in
> alphabetical order anymore.

It's how we've done it for a long time. I guess it doesn't matter either way.

Remus, please follow up with all of Henrik's suggestions.

Comment 41

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #39)
> Comment on attachment 597413 [details] [diff] [review]
> patch v5 (all branches)
> 
> Sorry for stealing the review but there are some thing I want to mention...

No problem. Feedback is always welcomed.

> >+  var blocklistService = Components.classes["@mozilla.org/extensions/blocklist;1"].
> >+                         getService(Components.interfaces.nsIBlocklistService);
> >+  blocklistService.QueryInterface(Components.interfaces.nsITimerCallback).notify(null);
This will be moved to the utils module in a new bug.
> >+  // Wait for the last addon to appear
> >+  controller.waitForElement(new elementslib.Selector(controller.window.document,
> >+                                                     ".addon:last-child"));
> 
> So where is this element you are querying for? Add-ons Manager? Then add an
> element selector to getElement. Also this should be two separate commands.

I was getting constant failures because the last addon in the extension list was not listed when I was querying for our addon. So this was a workaround.

Comment 42

[Remus Pop (:RemusPop)]

Attachment: patch v6 (all branches)

Addressed all the requests.

I have added in test2 a addHttpResource call so httpd is started.

Comment 43

[Remus Pop (:RemusPop)]

Attachment: patch v6 (all branches)

Updated the function call that pings the blocklist as of the name change in bug 729036.

Comment 44

[u279076]

Comment on attachment 599502 [details] [diff] [review]
patch v6 (all branches)

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

>+  // XXX: Bug 727842 - httpd server in mozmill stops after Firefox restarts in 
>+  //      restart tests
nit: "Need to restart HTTPD server after Firefox restarts"

>+/*
>+ * Test that blocklists the extension
>+ */
>+function testExtensionIsBlocklisted() {

Make sure the comment and function name match

>+  // Set category to 'Extensions'
>+  addonsManager.setCategory({
>+    category: addonsManager.getCategoryById({id: "extension"})
>+  });

'Extensions' != "extension"

>+  // Check if the add-on has been installed
>+  var addon = addonsManager.getAddons({attribute: "value",
>+                                      value: persisted.addon.id})[0];
nit: alignment

>+  // Ping the blocklist
>+  utils.updateBlocklist();

Comment should be "Update the blocklist"

>+// Check if the extension name is shown in the blocklist window
>+function checkExtensionName(controller) {

Rename to checkExtensionNameInBlocklist

>+  var extensionName = new elementslib.Selector(controller.window.document,
>+                      ".hardBlockedAddon").getNode().getAttribute("name");

It might be easier to read this code if you split it into two declarations.
One for the Selector, another for the Attribute.

>+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
>+               "in the blocklisted window. Expected: " + persisted.addon.name);

The last assertion in any test should be fatal.
Also, simplify the wording of the message:
  "'extension' appears in the blocklist"

>+++ b/tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test3.js
>+  var addon = addonsManager.getAddons({attribute: "name",
>+                                      value: persisted.addon.name})[0];
nit: fix alignment to the first letter of each variable

>+++ b/tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test4.js

>+  var addons = addonsManager.getAddons({attribute: "value", 
>+                                       value: persisted.addon.id});
nit: align on the first letter of each variable

>+
>+  assert.equal(addons.length, 0,
>+               "Blocklisted extension " + persisted.addon.id + " is uninstalled");

Add ' ' around the addon ID in your message.

>+}

One final note. It's probably best to keep the terminology the same throughout your tests, ie. Extension vs addon.

Canceling feedback for Henrik as he is on PTO.

Comment 45

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #44)
> >+  var extensionName = new elementslib.Selector(controller.window.document,
> >+                      ".hardBlockedAddon").getNode().getAttribute("name");
> 
> It might be easier to read this code if you split it into two declarations.
> One for the Selector, another for the Attribute.

That's also what our guidelines say.

> >+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
> >+               "in the blocklisted window. Expected: " + persisted.addon.name);
> 
> The last assertion in any test should be fatal.

Huh? Why that? It's not a positional decision it's the fact if a test could continue or not. Here we only check the extension name which is clearly not a fatal assertion call.

I can give feedback on an updated version of the patch.

Comment 46

[u279076]

(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #45)
> > >+  expect.equal(extensionName, persisted.addon.name, "Extension name is shown " +
> > >+               "in the blocklisted window. Expected: " + persisted.addon.name);
> > 
> > The last assertion in any test should be fatal.
> 
> Huh? Why that? It's not a positional decision it's the fact if a test could
> continue or not. Here we only check the extension name which is clearly not
> a fatal assertion call.

We are checking the name here not simply to verify the name is correct, but to verify the add-on is blocklisted. That's a fatal condition in my opinion.

Comment 47

[Remus Pop (:RemusPop)]

Attachment: patch v7 (all branches)

Addressed all requests.
Tried to use addon/add-on as much as possible. Used the name of the add-on in all messages instead of the id. Also I have updated the license.

Comment 48

[u279076]

Comment on attachment 606518 [details] [diff] [review]
patch v7 (all branches)

This looks good to me -- I'll hold off landing until Henrik has given his feedback (as requested).

Comment 49

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

Comment on attachment 606518 [details] [diff] [review]
patch v7 (all branches)

>+const ADDON = {
>+    name: "Test Extension (icons)",
>+    id: "[e-mail]",
>+    url: LOCAL_TEST_FOLDER + "addons/extensions/icons.xpi",
>+    blocklistPref: "extensions.blocklist.url"
>+};

I don't see a reason why the blocklist pref is listed under the add-on. I just wonder if we have to add it to persisted at all because that stays the same. So just define it as constant in each test module.

>+const BLOCKLIST_URL = LOCAL_TEST_FOLDER + "addons/blocklist/"+
>+                      "softblock_extension/blocklist.xml";

Move this up to the LOCAL_TEST_FOLDER declaration.

>+function testBlocklistsAddon() {

'testBlocklistAddon'

>+// Check if the add-on name is shown in the blocklist window
>+function checkAddonName(controller) {
>+  var addonSelector = new elementslib.Selector(controller.window.document,
>+                                               ".hardBlockedAddon");
>+  var addonName = addonSelector.getNode().getAttribute("name");
>+  
>+  assert.equal(addonName, persisted.addon.name, "'" + persisted.addon.name +
>+               "' appears in the blocklist");

This is a new type of chrome window. So I would suggest you add a new class to the addons.js module like BlocklistWindow. It should include the element spec this selector. Also why is it necessary to use getAttribute? Is there no property defined for the name?

>+function testUninstallBlocklistAddon() {

'testBlocklistedAddonAndUninstall'

Comment 50

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v8.0 (all branches)

Addressed changes, except: 
Unfortunately there is no property for the addon name so using getAttribute("name") 
Re adding a new class in addons.js, IMHO that is out of scope for this bug and 
should be treated in another one. We have decided in the past that shared module new code should be addressed separately - did we changed that? If so, I'm happy to add the new class here, just let me know

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 644298 [details] [diff] [review]
patch v8.0 (all branches)

adding Dave

Comment 52

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 644298 [details] [diff] [review]
patch v8.0 (all branches)

>+function testInstallBlocklistedAddon() {

I would prefer to see consistency between the naming of the test folder and the tests. In which case, this would be named testInstallBlocklistedExtension. This would also be consistent with other tests. This contradicts Henrik's last comment though so please check with him before making any further changes here. Of course, this comment would apply to all test methods in this patch.

>+/**
>+ * Test the blocklisted add-om has been uninstalled
>+ */

Typo: add-om

> Re adding a new class in addons.js, IMHO that is out of scope for this bug
> and 
> should be treated in another one. We have decided in the past that shared
> module new code should be addressed separately - did we changed that? If so,
> I'm happy to add the new class here, just let me know

I think this would be small enough to ride-along with this patch. If you want to go ahead and work on it separately then please file a bug and make it a blocker.

Comment 53

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v8.1 (all branches)

fixed 

thanks Dave

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

> I think this would be small enough to ride-along with this patch. If you
> want to go ahead and work on it separately then please file a bug and make
> it a blocker.

It seems like Remus already filed a new bug for this, and him and Henrik suggested to wait for the module refactor for this one - please see https://bugzilla.mozilla.org/show_bug.cgi?id=760139#c1 

So there one thing to decide here: land the test as it is or block the test until mozmill module refactor.

Comment 55

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

Given that the module refactoring has been pushed out of Q3 lets continue with bug 760139 and get it implemented.

Comment 56

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #55)
> Given that the module refactoring has been pushed out of Q3 lets continue
> with bug 760139 and get it implemented.

Henrik, can you please take a look over comment 52? 
If you don't agree with Dave's suggestion about re-naming please comment here.

Comment 57

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 644892 [details] [diff] [review]
patch v8.1 (all branches)

Cancelling reviews since we decided to go on and implement a UI map for the blocklist window component

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v9.0

* now that all dependencies are fixed, we can let this test in
* fixed to use our new shared module under /lib/ui
* removed trailing whitespaces existent in the previous patch
* fixed existent nits to comply with style guide

Comment 59

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 654182 [details] [diff] [review]
patch v9.0

Review of attachment 654182 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test1.js
@@ +32,5 @@
> +
> +  // Update extensions.blocklist.url pref to our blocklist
> +  prefs.preferences.setPref(PREF_BLOCKLIST, BLOCKLIST_URL);
> +
> +  // Set pref for add-on installation dialog timer 

nit: trailing whitespace

@@ +34,5 @@
> +  prefs.preferences.setPref(PREF_BLOCKLIST, BLOCKLIST_URL);
> +
> +  // Set pref for add-on installation dialog timer 
> +  prefs.preferences.setPref(PREF_INSTALL_DIALOG, INSTALL_DIALOG_DELAY);
> +  

nit: trailing whitespace

@@ +45,5 @@
> +function testInstallBlocklistedExtension() {
> +  var md = new modalDialog.modalDialog(addonsManager.controller.window);
> +
> +  md.start(addons.handleInstallAddonDialog);
> +  controller.open(persisted.addon.url);  

nit: trailing whitespace

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test2.js
@@ +5,5 @@
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var addonsBlocklist = require("../../../../lib/ui/addons_blocklist");
> +var {assert} = require("../../../../lib/assertions");
> +var domUtils = require("../../../../lib/dom-utils");

domUtils isn't used.

@@ +6,5 @@
> +var addons = require("../../../../lib/addons");
> +var addonsBlocklist = require("../../../../lib/ui/addons_blocklist");
> +var {assert} = require("../../../../lib/assertions");
> +var domUtils = require("../../../../lib/dom-utils");
> +var utils = require("../../../../lib/utils");

Neither is this.

@@ +23,5 @@
> + */
> +function testBlocklistsExtension() {
> +  addonsManager.open();
> +
> +  // Set category to 'Extensions'

This comment isn't necessary.

@@ +28,5 @@
> +  addonsManager.setCategory({
> +    category: addonsManager.getCategoryById({id: "extension"})
> +  });
> +
> +  // Check if the add-on has been installed

Same for this comment.

@@ +43,5 @@
> +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> +
> +  assert.equal(hardBlockedAddon.getNode().getAttribute("name"),
> +               persisted.addon.name, "'" + persisted.addon.name +
> +               "' appears in the blocklist");  

nit: trailing whitespace

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test3.js
@@ +22,5 @@
> +
> +  // Check that the extension is disabled
> +  expect.ok(!addonsManager.isAddonEnabled({addon: addon}),
> +            persisted.addon.name + " is disabled");
> +  

nit: trailing whitespace

Comment 60

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v9.1

Took this since Vlad's on PTO this week.
Addressed Dave's requests and separated addonsBlocklist module in test2,js as we did in lib/tests/testAddonsBlocklist.js

Comment 61

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

Comment on attachment 655560 [details] [diff] [review]
patch v9.1

Review of attachment 655560 [details] [diff] [review]:
-----------------------------------------------------------------

If this test only covers hard-blocked extensions it should be made visual in the test name. I would suggest to file a follow-up for soft-blocked addons.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test2.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var addonsBlocklist = require("../../../../lib/ui/addons_blocklist");

Given that `BlocklistWindow` is the only class we need from this module just include only that one.

@@ +13,5 @@
> +  addonsManager = new addons.AddonsManager(controller);
> +  blocklistWindow = new addonsBlocklist.BlocklistWindow(controller);
> +
> +  // XXX: Bug 727842 - Need to restart httpd after Firefox restarts
> +  collector.addHttpResource("../../../../data/");

Can't this be done in the global scope?

@@ +30,5 @@
> +  var addon = addonsManager.getAddons({attribute: "value",
> +                                       value: persisted.addon.id})[0];
> +
> +  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> +            "'" + persisted.addon.name + "' is installed");

You probably want to do `addonsManager.isAddonInstalled({addon: addons[0]})` here, otherwise we already fail in the line before accessing the first element.

@@ +33,5 @@
> +  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> +            "'" + persisted.addon.name + "' is installed");
> +
> +  // Update the blocklist then open blocklist window
> +  blocklistWindow.open();

The comment is outdated and can be removed.

@@ +38,5 @@
> +
> +  // Check if the add-on name is shown in the blocklist window
> +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> +
> +  assert.equal(hardBlockedAddon.getNode().getAttribute("name"),

Please kill the blank line.

@@ +40,5 @@
> +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> +
> +  assert.equal(hardBlockedAddon.getNode().getAttribute("name"),
> +               persisted.addon.name, "'" + persisted.addon.name +
> +               "' appears in the blocklist");

No need to add `persisted.addon.name` to the message. It's already a to evaluate value.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test3.js
@@ +21,5 @@
> +                                       value: persisted.addon.name})[0];
> +
> +  // Check that the extension is disabled
> +  expect.ok(!addonsManager.isAddonEnabled({addon: addon}),
> +            persisted.addon.name + " is disabled");

Same here for the addon name.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test4.js
@@ +12,5 @@
> +
> +function setupModule() {
> +  controller = mozmill.getBrowserController();
> +
> +  addonsManager = new addons.AddonsManager(controller);

No need for the blank line.

@@ +34,5 @@
> +                                        value: persisted.addon.id});
> +
> +  assert.equal(addons.length, 0,
> +               "Blocklisted add-on '" + persisted.addon.name +
> +               "' is uninstalled");

Same as for comments before. Make the message more general.

Comment 62

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

This bug has been stalled for a long time. Lets get it finished.

Comment 63

[Andrei Eftimie]

Attachment: patch v9.2

Addressed Henriks comments.

Also changed the extension used in this test suite, as the tests would fail with our icons.xpi (which requires a restart):
The Blocklist Window would not show up for an extension requireing a restart.

As a fix I used the restartless extension.

Here is a testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/f25fe2f500e5e4086802832f520c741d

Comment 64

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

Comment on attachment 702370 [details] [diff] [review]
patch v9.2

Review of attachment 702370 [details] [diff] [review]:
-----------------------------------------------------------------

> Also changed the extension used in this test suite, as the tests would fail
> with our icons.xpi (which requires a restart):
> The Blocklist Window would not show up for an extension requireing a restart.
> 
> As a fix I used the restartless extension.

No, that's not what this test is about. Here we really want to test a non-restartless extension. See step 3 in the Litmus test which requires a restart before we update the blocklist. You might have missed that? With a restartless extension it wouldn't even have to be a restart test. So we should file a new bug for the restartless extension case.

Comment 65

[Andrei Eftimie]

Attachment: patch v9.3

Reverted to icons.xpi as the extension used for this test.
I was unaware that test needing a restart should be run with > mozmill-restart

I was running them with > mozmill and that's why they failed.
All good now.

Passing testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510750e3

Comment 66

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 704842 [details] [diff] [review]
patch v9.3

Review of attachment 704842 [details] [diff] [review]:
-----------------------------------------------------------------

This is no longer aplying cleanly because of the manifest.ini file that has changed yesterday with the skip patch for Restartless extension.
Other than that, we should change the name of this test, as Henrik said in comment 61. Here we test only hardblocked extension, we should file another bug for the softblocked one.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test1.js
@@ +14,5 @@
> +
> +const ADDON = {
> +    name: "Test Extension (icons)",
> +    id: "[e-mail]",
> +    url: LOCAL_TEST_FOLDER + "addons/extensions/icons.xpi"

This block should be indented with 2 spaces.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test2.js
@@ +15,5 @@
> +  addonsManager = new addons.AddonsManager(controller);
> +  blocklistWindow = new BlocklistWindow(controller);
> +}
> +
> +function teardownModule(){

Need a space between ')' and '{'.

At a second thought, we don't need teardown for each test but the last one where we clean everything.
If you remove them, you will see the browser starts with home page, not with addonsManager tab from the last test.

@@ +22,5 @@
> +
> +/*
> + * Test that the extension is blocklisted
> + */
> +function testBlocklistsExtension() {

Think testBlocklistExtension() sound better.

@@ +41,5 @@
> +  // Check if the add-on name is shown in the blocklist window
> +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> +  assert.equal(hardBlockedAddon.getNode().getAttribute("name"),
> +               persisted.addon.name, "The addon appears in the blocklist");
> +}

Extra line at the end of a file is needed here.

::: tests/functional/restartTests/testAddons_installUninstallBlocklistedExtension/test3.js
@@ +17,5 @@
> +
> +/**
> + * Test uninstalling the blocklisted extension
> + */
> +function testUninstallBlocklistedExtensionAndUninstall() {

This name is confusing, could you change it to testUninstallBlocklistedExtension() maybe?

@@ +21,5 @@
> +function testUninstallBlocklistedExtensionAndUninstall() {
> +  addonsManager.open();
> +
> +  var addons = addonsManager.getAddons({attribute: "name",
> +                                       value: persisted.addon.name});

One space shifted to the right please, so it's aligned with attribute.

Comment 67

[Andrei Eftimie]

Attachment: patch v9.4

Testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb511aa940

Created bug 833745 for Soft-Blocked addons.

Addressed review feedback.

> @@ +41,5 @@
> > +  // Check if the add-on name is shown in the blocklist window
> > +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> > +  assert.equal(hardBlockedAddon.getNode().getAttribute("name"),
> > +               persisted.addon.name, "The addon appears in the blocklist");
> > +}
> 
> Extra line at the end of a file is needed here.

Actually here was an extra EOL character on the other files which I removed.
This one was good.

Comment 68

[Andreea Matei [:AndreeaMatei]]

Andrei, this is why I ask for a blank line at the end of each test:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Whitespace

Comment 69

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 705326 [details] [diff] [review]
patch v9.4

Review of attachment 705326 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me now. Thanks Andrei!

Henrik, do we want this backported? The lib at that moment landed only on default, but that was version 17.

Comment 70

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #69)
> Henrik, do we want this backported? The lib at that moment landed only on
> default, but that was version 17.

If nothing has been changed we might want to backport it given that it is an important feature, which we do not want to regress.

Otherwise what's the status of this patch? Can it be landed?

Comment 71

[Andreea Matei [:AndreeaMatei]]

It no longer applies cleanly on the manifest file. Otherwise, works fine.
Andrei, mind updating the patch?

Comment 72

[Andrei Eftimie]

Attachment: patch v9.5

Applies cleanly to all branches.

Comment 73

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 708492 [details] [diff] [review]
patch v9.5

Review of attachment 708492 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Andrei!

Comment 74

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

Comment on attachment 708492 [details] [diff] [review]
patch v9.5

Review of attachment 708492 [details] [diff] [review]:
-----------------------------------------------------------------

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

Comment 75

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

I accidentally backported this patch instead the one from bug 836686. So Andreea will still be the author of this patch. For the future please make sure to correctly update the commit message before uploading the patch. 

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/51c524a52df7 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/a5626ba584a7 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f705d0cae6b3 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/82445281aed6 (esr17)