Tracking of mozmill test creation for disabling a plugin Litmus test: https://litmus.mozilla.org/show_test.cgi?id=15972

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

As a sidenote, we can't automate this test as long as we cannot come up with an example plugin. As a workaround we could check in setupModule if plugins are installed. If not, mark the test function as skipped.

The plugin most common to all platforms is Flash. Is there a way we can skip a test if parsing for Flash fails (ie. not in the list)?

Yeah. Good call. As far as I can remember there should be a property on the navigator or window object which lists available plugins. We should check for that in setupModule and simply skip the test function if the plugin is not available.

How do we skip a test on a conditional?

See http://hg.mozilla.org/qa/mozmill-tests/file/3aa1c10a2d1f/tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js#l42

I'm currently checking for a Flash plugin only, as instructed in comment 3. But wouldn't be easy to just check that "controller.window.navigator.plugins.length != 0" ? That would mean some plugin is installed at least and I could still run the test. Also, enableAddon and disableAddon do not work for plugins and I need similar methods in the shared module. I logged bug 678486 for that. Is it ok if I enable the plugin in teardownModule in the same test file, as plugins enable/disable do not require a restart? Any other feedback is welcomed.

Working patch. Though the following questions from comment 7 still apply: > I'm currently checking for a Flash plugin only, as instructed in comment 3. > But wouldn't be easy to just check that > "controller.window.navigator.plugins.length != 0" ? That would mean some > plugin is installed at least and I could still run the test. > Is it ok if I enable the plugin in teardownModule in the same test file, as > plugins enable/disable do not require a restart? > > Any other feedback is welcomed.

Sounds totally reasonable for me. If we can select a random plugin it raises the chance that we can run this test. In most of the cases at least one plugin will hopefully be installed.

Comment on attachment 554107 [details] [diff] [review] patch v1.0 >+ // Check for Flash plugin >+ var installedPlugins = controller.window.navigator.plugins; >+ var installedPluginsCount = installedPlugins.length; >+ var isFlashInstalled = false; >+ >+ if (installedPluginsCount != 0) { >+ for (i=0; i<installedPluginsCount; i++) { >+ if (installedPlugins[i].description.indexOf("Flash") != -1) { >+ isFlashInstalled = true; >+ } >+ } >+ }; >+ >+ // Skip test if Flash is not installed >+ if (!isFlashInstalled) { >+ testDisablePlugin.__force_skip__ = "No Flash plugin installed."; >+ }; Move this to a helper function below the test. >+/** >+ * Disables a plugin >+ */ "Test disabling a plugin" >+ addonsManager.open(); >+ addonsManager.setCategory({category: addonsManager.getCategoryById({id: "plugin"})}); >+ >+ var plugins = addonsManager.getAddons({attribute: "type", value: "plugin"}); >+ var randomIndex = Math.floor(Math.random() * plugins.length); >+ var randomPlugin = plugins[randomIndex]; >+ >+ addonsManager.disableAddon({addon: randomPlugin}); >+ >+ expect.equal(randomPlugin.getNode().getAttribute("active"), "false", "Plugin is disabled"); >+ >+ persisted.plugin = randomPlugin; >+} Please comment the code.

Created a helper method, changed the test comment and commented the test code.

Comment on attachment 556545 [details] [diff] [review] patch v2.0 >+ // Skip test if Flash is not installed >+ if (!checkFlashIsInstalled(controller)) { >+ testDisablePlugin.__force_skip__ = "No Flash plugin installed."; >+ }; No ; necessary here. >+ >+/** >+ * Helper function to check if a Flash plugin is installed >+ * >+ *@param controller {MozMillController} Controller of the browser window >+ */ Space between * and @param. Order should be @keyword {type} variable Description See http://hg.mozilla.org/qa/mozmill-tests/file/ae28872c0b17/lib/addons.js#l115 >+function checkFlashIsInstalled(controller) { >+ // Retrieve installed plugins in an array >+ var installedPlugins = controller.window.navigator.plugins; >+ var installedPluginsCount = installedPlugins.length; You can simply use installedPlugins.length in the following code, no need for a separate variable here. >+ // If plugins are installed check to see if Flash is amongst them >+ if (installedPluginsCount != 0) { !== >+ for (i=0; i<installedPluginsCount; i++) { One space between operators and left/right-side (i = 0, i < installed) >+ if (installedPlugins[i].description.indexOf("Flash") != -1) { !== >+ }; No ; needed for closing bracket of IFs >+ // If there are no plugins installed this will still return false If no "Flash" plugins are installed, this will return false. >+ return isFlashInstalled; >+} You could also refactor this to take a string so that we could disable other plugins by passing in the string without having to add other helpers.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #12) > Comment on attachment 556545 [details] [diff] [review] > patch v2.0 > You could also refactor this to take a string so that we could disable other > plugins by passing in the string without having to add other helpers. Refactored it to take a string as param. Maybe we should be putting this in utils.js? And add another helper that checks that any plugins are installed?

Comment on attachment 558285 [details] [diff] [review] patch v3.0 >+function teardownModule() { >+ addonsManager.enableAddon({addon: persisted.plugin}); >+ >+ addonsManager.close(); >+ >+ delete persisted; >+} I'd like to see teardown code commented so people (especially new contributors) can understand what is happening. >+ // If plugins are installed check to see if Flash is amongst them >+ if (installedPlugins.length !== 0) { I'm actually thinking '> 0' would be better here to avoid cases where length might be negative. I'm not sure under what conditions a negative length would occur but >0 seems like the safest condition. >+ for (i = 0; i < installedPlugins.length; i++) { >+ if (installedPlugins[i].description.indexOf(plugin) !== -1) { >+ isPluginInstalled = true; You can add a break after this is set to true. We don't need to parse the array once the first instance is found.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14) > >+function teardownModule() { > >+ addonsManager.enableAddon({addon: persisted.plugin}); Teardown code should not use the UI to reset specific settings made by the test. Instead please use back-end code to ensure that the plugin has been enabled. > >+ delete persisted; Wow, please never delete persisted! Only delete the property you have added to this object.

Comment on attachment 558285 [details] [diff] [review] patch v3.0 >+ // Skip test if Flash is not installed >+ if (!checkPluginIsInstalled(controller, "Flash")) { >+ testDisablePlugin.__force_skip__ = "No Flash plugin installed."; Why are we still trying to only use Flash for this test? The question came already up a while back but hasn't been answered yet. Anthony, can you please respond to it? IMO the current handling is a huge limitation for the test.

We are using Flash because it is a plugin common to all platforms. We have modified the helper to take a string so it is a lot more robust. We could change this test to simply parse the about:plugins list and disable the first plugin found.

Wouldn't it be easier to just check to see if any plugin is installed via "installedPlugins.length > 0" and then just disable a random plugin from the list in aom?

(In reply to Alex Lakatos from comment #18) > Wouldn't it be easier to just check to see if any plugin is installed via > "installedPlugins.length > 0" and then just disable a random plugin from the > list in aom? Yes, that's what I would propose here. Just take the first or a random available plugin. That way we could broaden the coverage of existent plugins.

If no plugins are enabled, the test will be skipped. If plugins are enabled, it will disable a random one.

Comment on attachment 568395 [details] [diff] [review] patch v4.0 >+ // Disable the random plugin and check that it's disabled >+ addonsManager.disableAddon({addon: randomPlugin}); >+ expect.equal(randomPlugin.getNode().getAttribute("active"), "false", "Plugin is disabled"); >+ >+ // Store the plugin id in persisted object >+ persisted.pluginId = randomPlugin.getNode().getAttribute("value"); >+} Forgive me, but why do we need to store anything in the persisted object if it's the end of the test?

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #21) > > Forgive me, but why do we need to store anything in the persisted object if > it's the end of the test? Because I need the pluginId in teardownModule. Should I use a global variable in the test instead, without using the "var" keyword? >pluginId = randomPlugin.getNode().getAttribute("value")

Comment on attachment 568395 [details] [diff] [review] patch v4.0 >+function setupModule() { >+ controller = mozmill.getBrowserController(); For new tests we should really take care of to not create global constants here. Please use the aModule parameter for setupModule and teardownModule. Inside those functions any variable will be created as 'aModule.controller'. Then the variable will be accessible from within the test directly. >+function teardownModule() { >+ // Enable the plugin using back-end code >+ addons.enableAddon(persisted.pluginId); >+ >+ addonsManager.close(); >+ >+ delete persisted.pluginId; Same as for setupModule. Also please move up the delete line right below enableAddon. Both are tight together. >+ // Disable the random plugin and check that it's disabled >+ addonsManager.disableAddon({addon: randomPlugin}); >+ expect.equal(randomPlugin.getNode().getAttribute("active"), "false", "Plugin is disabled"); >+ >+ // Store the plugin id in persisted object >+ persisted.pluginId = randomPlugin.getNode().getAttribute("value"); First, you have to store the plugin ID before you are trying to disable it. If the disable action fails we do not restore the initial state. Second, please use a module wide variable as described above. As Anthony said there is no need for persisted in a single test.

No work happened here for a long time. I just checked our existing tests and found that bug 687449 seems to completely cover the scenario here. It's hard to predict given that Litmus as reference does no longer exist, but I would say that no work needs to happen here.