687449 - Mozmill test for checking if enabling / disabling a plugin affects about:plugins

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Tracking creation of a mozmill test that will check if enabling or disabling a plugin will affect about:plugins page.

Litmus Testcase: https://litmus.mozilla.org/show_test.cgi?id=15968

Comment 1

[Remus Pop (:RemusPop)]

It needs this for the teardown module.

Comment 2

[u279076]

Pivotal Story:
https://www.pivotaltracker.com/story/show/18476093

Comment 3

[Remus Pop (:RemusPop)]

Attachment: patch v1

The test is skipped if no plugins are available. This is done in the setupModule.

Comment 4

[Remus Pop (:RemusPop)]

Attachment: patch v1

Sorry for that. Forgot to declare plugin.
Also this test uses persisted to pass the id of the disabled plugin to the teardown module. Persisted is deleted after that.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

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


>+
>+function testEnableDisablePlugin () {
>+  // Open the Add-ons Manager and select the Plugins pane
>+  addonsManager.open();
>+  addonsManager.setCategory({
>+    category: addonsManager.getCategoryById({id: "plugin"})
>+  });
>+

I would change this indentation with 

function testEnableDisablePlugin () {
  addonsManager.open();

  // Select the Plugins category
  addonsManager.setCategory({
    category: addonsManager.getCategoryById({id: "plugin"})
  });

It's done this way in all other add-ons manager tests - so i propose it also here for consistency 

>+  // Select the first plugin that is installed and which is enabled
>+  var plugins = addonsManager.getAddons({attribute: "type", value: "plugin"});
>+  var plugin;
>+
>+  for (var i = 0; i < plugins.length; i++) 
>+    if (plugins[i].getNode().getAttribute("active") === "true") {
>+      plugin = plugins[i];
>+
>+      // Store the plugin id to be used in teardownModule
>+      persisted.pluginID = plugin.getNode().getAttribute("value");
>+      break;
>+      }

I think it's best and more elegant here to change the for loop with a forEach and just pass the plugin var 
as a parameter. use other tests for code reference 

>+
>+  // Store the plugin id to be used in teardownModule
>+  persisted.pluginID = plugin.getNode().getAttribute("value");
>+
>+  // Disable the plugin
>+  addonsManager.disableAddon({addon: plugin});
>+
>+  // Check that the plugin is disabled
>+  assert.equal(plugin.getNode().getAttribute("active"), "false", "Plugin has been disabled");
>+
>+  // Check that the plugin disappeared from about:plugins
>+  tabBrowser = new tabs.tabBrowser(controller);
>+  tabBrowser.openTab();
>+  controller.open("about:plugins");
>+  controller.waitForPageLoad();
>+
>+  var nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab);
>+  var pluginNames = nodeCollector.queryNodes(".plugname").elements;
>+  var found = false;
>+
>+  for (var i = 0; i < pluginNames.length; i++) 
>+    if (pluginNames[i].getNode().innerHTML === plugin.node.mAddon.name) {
>+      found = true;
>+      break;
>+    }
>+
>+  expect.equal(found, false, "Plugin does not appear in about:plugins");
>+
>+  // Open the Add-ons Manager
>+  addonsManager.open();
>+
>+  //Enable the plugin
>+  addonsManager.enableAddon({addon: plugin});
>+
>+  // Check that the plugin is enabled
>+  assert.ok(plugin.getNode().getAttribute("active"), "Plugin has been enabled");
>+
>+  // Check that the plugin appears in about:plugins
>+  controller.open("about:plugins");
>+  controller.waitForPageLoad();
>+
>+  nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab);
>+  pluginNames = nodeCollector.queryNodes(".plugname").elements;
>+  found = false;
>+
>+  for (var i = 0; i < pluginNames.length; i++) 
>+    if (pluginNames[i].getNode().innerHTML === plugin.node.mAddon.name) {
>+      found = true;
>+      break;
>+    }
>+
>+  expect.ok(found, "Plugin appears in about:plugins");
>+}

Well i tend to think this code-approach makes the test look fragile. 
Besides my suggestions so far, i believe the best person to consult here is Henrik

Comment 6

[Remus Pop (:RemusPop)]

Using forEach I won't be able to stop the loop. Break does not work here. But if that is fine with everyone, I'll use forEach.

Comment 7

[Remus Pop (:RemusPop)]

Attachment: patch v2

Here is an updated patch with all fors changed to forEach. Logic stays the same.

Comment 8

[Remus Pop (:RemusPop)]

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

Henrik's review/feedback for patch v1 will also help here.

Comment 9

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

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

>+++ b/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js

Do we cover enabling/disabling or only disabling?

>+function setupModule () {
>+  controller = mozmill.getBrowserController();
>+
>+  // Skip test if we have no plugins
>+  if (controller.window.navigator.plugins.length < 1)
>+    testEnableDisablePlugin.__force_skip__ = "At least 1 plugin must be installed";
>+
>+  addonsManager = new addons.AddonsManager(controller);
>+  tabs.closeAllTabs(controller);
>+  
>+}

You are creating a couple of global variables here. Please use the aModule parameter of the method. Also we should decide immediately which plugin we want to work with and store related information in the persisted object. The navigator.plugins array has an alphabetical sorted list of plugins, or?

>+function teardownModule () {

nit: There has to be no blank between the function name and the brackets.

>+function testEnableDisablePlugin () {

>+  for (var i = 0; i < plugins.length; i++) 
>+    if (plugins[i].getNode().getAttribute("active") === "true") {
>+      plugin = plugins[i];

We should already be able to check this in setupModule via a backend call or the status of the navigator.plugins entry. It's really a setup step for the test function.

>+      // Store the plugin id to be used in teardownModule
>+      persisted.pluginID = plugin.getNode().getAttribute("value");
>+      break;
>+      }
>+
>+  // Store the plugin id to be used in teardownModule
>+  persisted.pluginID = plugin.getNode().getAttribute("value");

Not sure why this exists twice. The latter would even not work if plugin hasn't been updated in the loop.

>+  // Check that the plugin disappeared from about:plugins
>+  tabBrowser = new tabs.tabBrowser(controller);

You are creating a global variable here again. Please stay consistent with other tests and move it into setupModule.

>+  // Check that the plugin appears in about:plugins
>+  controller.open("about:plugins");
>+  controller.waitForPageLoad();

You can switch the tabs and simply reload?

>+  nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab);

No need to reinstantiate nodeCollector again. Just use the previously created instance and don't forget 'var'.

>+  pluginNames = nodeCollector.queryNodes(".plugname").elements;
>+  found = false;
>+
>+  for (var i = 0; i < pluginNames.length; i++) 
>+    if (pluginNames[i].getNode().innerHTML === plugin.node.mAddon.name) {
>+      found = true;
>+      break;
>+    }

Looks like we need a helper function here. Duplicating code is not that good.

Also I would like to see a check for about:plugins before you disable a plugin. Just to ensure it is listed on that page.

Comment 10

[Remus Pop (:RemusPop)]

Anthony, Alex and Henrik, I would like to hear your opinion in using forEach or for in this test. There are several loops in patch v1. Patch v2 has been modified in regards to Vlad's review to use forEach but I'm not sure if that is the best approach.

In my opinion for is better because I can break the loop when I obtain what I want.

Comment 11

[u279076]

Remus, I would prefer that you use forEach whenever iterating array or list elements.

Comment 12

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Remus, I would prefer that you use forEach whenever iterating array or list
> elements.

Here is a reference in our styleguide:
https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Iteration

Comment 13

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #12)
> Here is a reference in our styleguide:
> https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Iteration

It doesn't help if you need a break. That statement will not work with forEach, as Remus already pointed out. Workaround would be to throw a custom exception instead and catch it right after again.

So I don't think forEach is the solution here.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: patch v3

This is almost a complete refactoring of my previous patch.

It addresses Henrik's concerns.

Other than that, in the setup module we store the name of the first plugin using backend code (navigator.plugins) if there is at least 1 enabled plugin.

In the first part we store the whole plugin object to the same persisted.plugin object in which we store the name before. This is helpful. The name isn't.

We also have a helper function which checks if our plugin (persisted.plugin) is present in the about:plugins page. It returns true or false.

For tab browsing I have used tabBrowser.selectedIndex to switch between the tabs. We have to tabs in which we work. First is the about:plugins and the second is the Addons Manager.

Also I have decided to use for because I can break the loop. Iterating through the whole array isn't helpful.

Comment 15

[Alex Lakatos[:AlexLakatos]]

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

>+  // Skip test if we don't have enabled plugins
>+  if (controller.window.navigator.plugins.length < 1) {
>+    testEnableDisablePlugin.__force_skip__ = "At least 1 plugin must be enabled";
>+    teardownModule.__force_skip__ = "At least 1 plugin must be enabled";
>+  } else {
>+      persisted.plugin = controller.window.navigator.plugins[0];
>+  }
Remus, please take a look at our style guide to see how conditionals must be indented https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Conditionals

>+  assert.equal(persisted.plugin.getNode().getAttribute("active"), "false", "Plugin has been disabled");
To be consistent add a comment here "// Check that the plugin is disabled"

>+    if (pluginNames[i].getNode().innerHTML === persisted.plugin.node.mAddon.name) {
I think you should use "pluginNames[i].getNode().textContent" here.

>+      found = true;
>+      break;
>+    }
>+  }
>+
>+  return found;
>+}
I think we can refactor this to:
>+      return true;
>+    }
>+  }
>+
>+  return false;
>+}

Comment 16

[Remus Pop (:RemusPop)]

Attachment: patch v4

Addressed all Alex's requests.

Comment 17

[Remus Pop (:RemusPop)]

Attachment: patch v4

Sorry about that. I reviewed the use of expects and assertions. Using an assert means that it invalidates the rest of the test. Also updated the commit info.

Comment 18

[u279076]

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

>+function setupModule(aModule) {
>+  controller = mozmill.getBrowserController();
>+
>+  aModule.tabBrowser = new tabs.tabBrowser(controller);

Don't use aModule, just tabBrowser is good enough.

>+
>+  // Skip test if we don't have enabled plugins
>+  if (controller.window.navigator.plugins.length < 1) {
>+    testEnableDisablePlugin.__force_skip__ = "At least 1 plugin must be enabled";
>+    teardownModule.__force_skip__ = "At least 1 plugin must be enabled";
>+  }

Please update the skip message to read "No enabled plugins detected"

>+  else {

Please put this on the same line as your }

>+      persisted.plugin = controller.window.navigator.plugins[0];
>+  }

Fix indentation.

>+
>+  aModule.addonsManager = new addons.AddonsManager(controller);

Again, don't use aModule


>+
>+function testEnableDisablePlugin() {

Please add missing comment

>+
>+function pluginExistsInAboutPlugins() {

Please add missing comment

Comment 19

[Remus Pop (:RemusPop)]

Attachment: patch v5

Addressed Anthony's requests.

Comment 20

[u279076]

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

>+  // Check that the plugin is listed on the about:plugins page
>+  assert.ok(pluginExistsInAboutPlugins(), "Plugin is listed on the about:plugins page");

Please indicated the plugin name or ID in all assert messages.

>+/**
>+ * Checks that the plugin appears in about:plugins
>+ *
>+ * @returns True if the plugin appears in about:plugins

@returns {boolean} True if the plugin appears in about:plugins 

>+ */
>+function pluginExistsInAboutPlugins() {
>+  tabBrowser.selectedIndex = 0;
>+  controller.open("about:plugins");
>+  controller.waitForPageLoad();
>+
>+  var nodeCollector = new domUtils.nodeCollector(controller.tabs.activeTab);
>+  pluginNames = nodeCollector.queryNodes(".plugname").elements;
>+
>+  for (var i = 0; i < pluginNames.length; i++) {
>+    if (pluginNames[i].getNode().textContent === persisted.plugin.node.mAddon.name)
>+      return true;
>+  }
>+
>+  return false;
>+}

Please refactor this so you return a check variable instead of a nested return.

Comment 21

[Remus Pop (:RemusPop)]

Attachment: patch v6 [backed-out]

Addressed all concerns.

Comment 22

[u279076]

Comment on attachment 574558 [details] [diff] [review]
patch v6 [backed-out]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d7cf7db0872f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/c23ce2095452 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/296a4a10c8cb (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/9519468c81d0 (mozilla-release)

Comment 23

[u279076]

Please mark VERIFIED and update the spreadsheet if this is passing tomorrow morning.

Comment 24

[u279076]

This is causing some failures today which were not visible locally:

http://mozmill-release.brasstacks.mozilla.com/#/functional/failure?branch=All&platform=All&from=2011-11-16&to=2011-11-23&test=%2FtestAddons%2FtestPluginDisableAffectsAboutPlugins.js&func=testPluginDisableAffectsAboutPlugins.js%3A%3AsetupModule

I'm going to back out the test. Please investigate.

Comment 25

[u279076]

Comment on attachment 574558 [details] [diff] [review]
patch v6 [backed-out]

Backed-out:
http://hg.mozilla.org/qa/mozmill-tests/rev/02ab1ac4f009 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/8903a1d524da (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/a8496f0d05eb (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/100111045df4 (mozilla-release)

Comment 26

[Remus Pop (:RemusPop)]

Attachment: patch v7

My mistake actually, but I fixed it now. The name of the function that we skipped was not correct

It's good we had this fail because this tells us that there were no plugins installed. Can someone check this?

We are good to go now.

Comment 27

[u279076]

Comment on attachment 577233 [details] [diff] [review]
patch v7

Since we are back in review, let's fix these long lines...

>+  persisted.plugin = addonsManager.getAddons({attribute: "name", value: persisted.plugin.name})[0];

Wrap on "value:"

>+  // Check that the plugin is listed on the about:plugins page
>+  assert.ok(pluginExistsInAboutPlugins(), persisted.plugin.name + " is listed on the about:plugins page");

Wrap on "persisted"

>+  // Check that the plugin is disabled
>+  assert.equal(persisted.plugin.getNode().getAttribute("active"), "false", persisted.plugin.name + " has been disabled");

Wrap on "false"

>+  // Check that the plugin disappeared from about:plugins
>+  expect.ok(!pluginExistsInAboutPlugins(), persisted.plugin.name + " does not appear in about:plugins");

Wrap on "persisted"

>+  // Check that the plugin is enabled
>+  assert.ok(persisted.plugin.getNode().getAttribute("active"), persisted.plugin.name + " has been enabled");

Wrap on "persisted.plugin.name"

>+
>+  // Check that the plugin appears in about:plugins
>+  expect.ok(pluginExistsInAboutPlugins(), persisted.plugin.name + " appears in about:plugins");

Wrap on "persisted.plugin.name"

Comment 28

[Remus Pop (:RemusPop)]

Attachment: patch v8 [checked-in:default]

Addressed all requested changes.

Comment 29

[u279076]

Comment on attachment 579006 [details] [diff] [review]
patch v8 [checked-in:default]

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

Please check tomorrow's results to see if this fails -- if not, will land on other branches.

Comment 30

[u279076]

Using checkin-needed keyword for the time being to track landing needed to other branches.

Comment 31

[Remus Pop (:RemusPop)]

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

Comment 32

[u279076]

I tried to land this on Aurora and I get the following error...

Error:
"undefined has been disabled - 'true' should equal 'false'"
testPluginDisableAffectsAboutPlugins.js:92

Command:
mozmill -b /Applications/Aurora.app/Contents/MacOS/firefox -t ./tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js --show-errors

Environment:
Mozmill 1.5.7 on Firefox 10.0a2 2011-12-13 on Mac OSX 10.6.8