Last Comment Bug 687823 - Mozmill test for Disabled state of Plugin remembered after a Restart
: Mozmill test for Disabled state of Plugin remembered after a Restart
Status: RESOLVED FIXED
[mozmill-functional][mozmill-aom]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Alex Lakatos[:AlexLakatos]
:
Mentors:
Depends on: 687450
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-20 06:27 PDT by Remus Pop (:RemusPop)
Modified: 2012-08-06 05:45 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch v1 (7.18 KB, patch)
2011-10-14 05:18 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Review
patch v2 (7.18 KB, patch)
2011-10-14 06:05 PDT, Remus Pop (:RemusPop)
alex.lakatos.qa: review-
Details | Diff | Review
patch v3 (7.14 KB, patch)
2011-10-19 06:38 PDT, Remus Pop (:RemusPop)
alex.lakatos.qa: review+
anthony.s.hughes: review-
Details | Diff | Review
patch v4 (7.20 KB, patch)
2011-10-31 08:17 PDT, Remus Pop (:RemusPop)
anthony.s.hughes: review-
Details | Diff | Review
patch v5 (7.38 KB, patch)
2011-11-15 02:06 PST, Remus Pop (:RemusPop)
anthony.s.hughes: review-
Details | Diff | Review
patch v6.0 (5.76 KB, patch)
2012-07-23 07:00 PDT, Alex Lakatos[:AlexLakatos]
hskupin: review-
Details | Diff | Review
patch v7.0 (5.64 KB, patch)
2012-07-27 11:28 PDT, Alex Lakatos[:AlexLakatos]
dave.hunt: review-
Details | Diff | Review
patch v8.0 (5.70 KB, patch)
2012-07-31 10:17 PDT, Alex Lakatos[:AlexLakatos]
dave.hunt: review-
Details | Diff | Review
patch v9.0 (5.68 KB, patch)
2012-08-01 09:28 PDT, Alex Lakatos[:AlexLakatos]
dave.hunt: review+
Details | Diff | Review

Description Remus Pop (:RemusPop) 2011-09-20 06:27:57 PDT
Tracking creation of a mozmill test that checks if a disabled plugin remains disabled after browser restart.
Comment 1 Remus Pop (:RemusPop) 2011-09-20 06:31:12 PDT
The testcase can be found here:
https://litmus.mozilla.org/show_test.cgi?id=16022
Comment 2 u415141 2011-09-20 23:14:57 PDT
Remus Pop changed story state to started in Pivotal Tracker
Comment 3 Remus Pop (:RemusPop) 2011-10-14 05:18:08 PDT
Created attachment 567057 [details] [diff] [review]
patch v1

In relation with bug 687449 I still think it is best to use the for loop.
The test is skipped if there are no plugins or plugins are not enabled.
I use persisted.installedPlugins to store the number of plugins before disabling one in case there would be only 1 plugin installed and enabled. Otherwise test2 will be skipped.
Comment 4 Alex Lakatos[:AlexLakatos] 2011-10-14 05:30:05 PDT
>+  if (controller.window.navigator.plugins.length < 1)
>+    testDisablePlugin.__force_skip__= "At least 1 plugin must be installed";
The message should read "At least 1 plugin must be enabled", as controller.window.navigator.plugins only lists enabled plugins.
Comment 5 Remus Pop (:RemusPop) 2011-10-14 06:05:34 PDT
Created attachment 567066 [details] [diff] [review]
patch v2

Moved where we get the number of installed plugins. This was done previosuly in the test function and now it's in the setupModule. If the test got skipped, the variable would have been undefined for test2.js.

Also addressed Alex's request.
Comment 6 Alex Lakatos[:AlexLakatos] 2011-10-18 07:53:28 PDT
Comment on attachment 567066 [details] [diff] [review]
patch v2


>+  // XXX: If a plugin is disabled the total number of plugins will decrease
>+  persisted.installedPlugins = controller.window.navigator.plugins.length;
This should be persisted.enabledPlugins as controller.window.navigator.plugins only returns enabled plugins.

>+  // Select the first plugin that is installed and which is enabled
Reword to something like "Select the first enabled plugin"
Comment 7 Remus Pop (:RemusPop) 2011-10-19 06:38:31 PDT
Created attachment 568031 [details] [diff] [review]
patch v3

Addressed all issues.
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-27 17:16:18 PDT
Comment on attachment 568031 [details] [diff] [review]
patch v3

>+/**
>+ * Test that disables a plugin
>+ */

nit: Test disabling a plugin

>+  // Select the first enabled plugin
>+  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];
>+      break;
>+      }
>+
>+  persisted.pluginID = plugin.getNode().getAttribute("value");

This should be refactored and put into a helper function in this test so that...

persisted.pluginID = getFirstPluginID(plugins);

>+  // Check that the plugin is disabled
>+  expect.equal(plugin.getNode().getAttribute("active"), "false", "Plugin is disabled");

This needs to be an assert.

>+  // Skip test if we have no plugins
>+  if ((controller.window.navigator.plugins.length < 1) && 
>+      (persisted.enabledPlugins < 1))
>+    testDisablePlugin.__force_skip__= "At least 1 plugin must be enabled";

This is not necessary as the this is already checked in test1.js
We can't get this far without at least one plugin being enabled.
Also, this test will be skipped in a case where we only have one plugin and it has been disabled.

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

Fix alignment.

>+  // Check that the plugin is enabled
>+  expect.equal(plugin.getNode().getAttribute("active"), "true", "Plugin is enabled");

Make this an assert.
Comment 9 Remus Pop (:RemusPop) 2011-10-31 08:14:44 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #8)
> Comment on attachment 568031 [details] [diff] [review] [diff] [details] [review]
> patch v3
> 
> >+  // Select the first enabled plugin
> >+  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];
> >+      break;
> >+      }
> >+
> >+  persisted.pluginID = plugin.getNode().getAttribute("value");
> 
> This should be refactored and put into a helper function in this test so
> that...
> 
> persisted.pluginID = getFirstPluginID(plugins);

The best I can do is refactor this into getFirstPlugin() and persisted.pluginID stays the same because I would need 2 things from it: ID and the plugin object. Only one can be returned. If it would be for the ID, I would need plugin = getAddons(pluginID) call which I think gets this code inefficient.
 
> >+  // Skip test if we have no plugins
> >+  if ((controller.window.navigator.plugins.length < 1) && 
> >+      (persisted.enabledPlugins < 1))
> >+    testDisablePlugin.__force_skip__= "At least 1 plugin must be enabled";
> 
> This is not necessary as the this is already checked in test1.js
> We can't get this far without at least one plugin being enabled.
> Also, this test will be skipped in a case where we only have one plugin and
> it has been disabled.

The test would not be skipped. Both conditions must be true in order to skip the test. If we had 1 plugin in test1, persisted.enabledPlugins would be 1. Anyway, persisted.enabledPlugins < 1 condition should be enough if the test must be skipped. Also added the skip for the teardownModule.
Comment 10 Remus Pop (:RemusPop) 2011-10-31 08:17:14 PDT
Created attachment 570712 [details] [diff] [review]
patch v4
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-14 09:53:45 PST
Comment on attachment 570712 [details] [diff] [review]
patch v4

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

Please use the plugin name or ID in the message.

>+function getFirstEnabledPlugin() {
>+  var plugins = addonsManager.getAddons({attribute: "type", value: "plugin"});
>+
>+  for (var i = 0; i < plugins.length; i++) 
>+    if (plugins[i].getNode().getAttribute("active") === "true")
>+      return plugins[i];
>+}

Conceptually, I don't like having nested returns. Can you refactor this to assign to a variable and return the variable outside the loop?

>+  // Check that the plugin is disabled
>+  expect.equal(plugin.getNode().getAttribute("active"), "false", "Plugin is disabled");

This should be assert since you can't enable an add-on which is not disabled.
Comment 12 Remus Pop (:RemusPop) 2011-11-15 02:06:26 PST
Created attachment 574556 [details] [diff] [review]
patch v5

All requests have been satisfied.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-22 15:36:00 PST
Comment on attachment 574556 [details] [diff] [review]
patch v5

Test is failing for me on Nightly on my Mac:

Flip4Mac Windows Media Plugin 2.3.3 is enabled
'false' should equal 'true'
testAddons_pluginDisabledAfterRestart/test2.js: 80
Comment 14 Remus Pop (:RemusPop) 2011-11-24 06:42:33 PST
I installed the latest Flip4Mac Windows Media Plugin version, 2.4.0.11 and I cannot reproduce the fail. Judging by the line in which the error appears means somehow the plugin doesn't get enabled.
Can you please investigate this?

Also, if it matters, I am running this test on OSX 10.6.8 with Snow Leopard.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 11:18:34 PST
Seems like what is happening is that the plugin is failing to re-enable. Here is a flow of what I am seeing:

* 1st Plugin = Flip4Mac
* Flip4Mac disabled
* Restart Firefox
* Flip4Mac moved to the bottom of the list and is still disabled
* Try to enable Flip4Mac
* Flip4Mac still disabled
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 11:20:59 PST
There is a variable of weirdness happening here. Even if I point Mozmill to Nightly, it starts Firefox 8.0.1. Here is the command:

mozmill-restart /Applications/Nightly.app/ -t ./tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/ --show-errors

If I run /Applications/Nightly.app/Contents/MacOS/firefox from terminal it starts in Nightly. Seems like something might be screwed up locally which causes Mozmill to default to starting with Firefox 8.0.1, which is currently installed to /Applications/Firefox.app/ locally.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 11:53:00 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> There is a variable of weirdness happening here. Even if I point Mozmill to
> Nightly, it starts Firefox 8.0.1. 

Okay, this seems to have resolved itself. The error reported in comment 13 still exists for me. I'm not sure what else I can do to help you. I cannot check this in until this test runs error free.
Comment 18 Henrik Skupin (:whimboo) 2011-11-25 01:03:52 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> There is a variable of weirdness happening here. Even if I point Mozmill to
> Nightly, it starts Firefox 8.0.1. Here is the command:
> 
> mozmill-restart /Applications/Nightly.app/ -t
> ./tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/
> --show-errors

It has to be mozmill-restart -b %APP_PATH%. Otherwise the default browser is used.
Comment 19 Remus Pop (:RemusPop) 2011-11-28 05:18:05 PST
Anthony, can you please update your version of Flip4Mac to the latest and try to run this test again?
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-28 08:33:01 PST
The last time I tested this (comment 17), I had updated to the latest. I still get this error.
Comment 21 Remus Pop (:RemusPop) 2011-11-28 08:38:07 PST
Anthony, please share the version of OSX that you're using, along with the mozmill version (in terminal: mozmill --version), just to rule out this things.

I will try this in a different Mac machine and also investigate if a waitFor is involved after changing the status of an addon.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-28 08:48:22 PST
Mac OSX 10.6.8
Mozmill 1.5.7
Flip4Mac 2.4.0.11
Comment 23 Henrik Skupin (:whimboo) 2012-07-19 23:11:01 PDT
This test has been lost in the nirvana. Alex, please take care of it.
Comment 24 Alex Lakatos[:AlexLakatos] 2012-07-23 02:24:30 PDT
Ran this on Mac OSX 10.6.8 with Mozmill 1.5.15 and Flip4Mac 2.4.4.2(latest), and it passes. 
If the fail was all that stopped the landing,  and given comment 12, I think's it's ok to land on default and watch for the results in CI. 
If Henrik wants to have another go at reviewing, I'll make the requested edits.
Comment 25 Henrik Skupin (:whimboo) 2012-07-23 05:56:09 PDT
So yeah, please request review but check the patch for updates first. We should at least change the license to MPL2. Not sure what else has to be done.
Comment 26 Alex Lakatos[:AlexLakatos] 2012-07-23 07:00:56 PDT
Created attachment 644921 [details] [diff] [review]
patch v6.0

*added MPL2
*deleted all elements of the persisted element
*added and updated manifest files
*split long lines
Comment 27 Henrik Skupin (:whimboo) 2012-07-24 03:52:58 PDT
Comment on attachment 644921 [details] [diff] [review]
patch v6.0

>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+
>+  // Skip test if we have no plugins
>+  if (controller.window.navigator.plugins.length < 1)
>+    testDisablePlugin.__force_skip__= "At least 1 plugin must be enabled";
>+
>+  // XXX: If a plugin is disabled the total number of plugins will decrease
>+  persisted.enabledPlugins = controller.window.navigator.plugins.length;

This is not a 'XXX' comment. Just remove the prefix.

>+function testDisablePlugin() {
>+  var plugin = getFirstEnabledPlugin();

I do not understand why we need another method here. Move it's code in here please.

>+  persisted.pluginID = plugin.getNode().getAttribute("value");
>+  persisted.pluginName = plugin.getNode().getAttribute("name");

Please use a single object on the persisted module like persisted.plugin = { }.

>+function getFirstEnabledPlugin() {
>+  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];
>+      break;
>+    }

I would do this via a back-end call and a filter by getInstalledAddons(). It can even be located in the setupModule() because its a setup step. In the test itself we can then directly query for this addon.

>+  //Enable the plugin

nit: missing blank after comment prefix.
Comment 28 Alex Lakatos[:AlexLakatos] 2012-07-27 11:28:10 PDT
Created attachment 646643 [details] [diff] [review]
patch v7.0

(In reply to Henrik Skupin (:whimboo) [away 07/27 - 08/05] from comment #27)
> 
> I would do this via a back-end call and a filter by getInstalledAddons(). It
> can even be located in the setupModule() because its a setup step. In the
> test itself we can then directly query for this addon.
I'm not sure I get this. Maybe Dave can explain it further?
Comment 29 Dave Hunt (:davehunt) 2012-07-30 03:57:10 PDT
Comment on attachment 646643 [details] [diff] [review]
patch v7.0

>Bug 687823 - Disabled state of Plugin is remembered after a Restart r=vlad.maniac, r=ashughes, r=hskupin r=dhunt

Missing comma between hskupin and dhunt.

>+  persisted.plugin = {id: "", name: ""};

>+  persisted.plugin.id = plugin.getNode().getAttribute("value");
>+  persisted.plugin.name = plugin.getNode().getAttribute("name");

You shouldn't need to instantiate this in advance. You could just use:

  persisted.plugin = {
    id: plugin.getNode().getAttribute("value"),
    name: plugin.getNode().getAttribute("name")
  };

> (In reply to Henrik Skupin (:whimboo) [away 07/27 - 08/05] from comment #27)
> > 
> > I would do this via a back-end call and a filter by getInstalledAddons(). It
> > can even be located in the setupModule() because its a setup step. In the
> > test itself we can then directly query for this addon.

> I'm not sure I get this. Maybe Dave can explain it further?

You can see an example of getInstalledAddons that filters by active plugins here: http://hg.mozilla.org/qa/mozmill-tests/file/b7b3a873054d/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js#l25
Comment 30 Alex Lakatos[:AlexLakatos] 2012-07-31 10:17:53 PDT
Created attachment 647587 [details] [diff] [review]
patch v8.0

fixed commit message. used getInstalledAddons()
Comment 31 Dave Hunt (:davehunt) 2012-08-01 06:35:15 PDT
Comment on attachment 647587 [details] [diff] [review]
patch v8.0

>+function setupModule(aModule) {
>+  controller = mozmill.getBrowserController();
>+
>+    // Skip test if we don't have enabled plugins

Nit: Fix the identation here

>+  if (activePlugins.length !== 0) {
>+    aModule.plugin = activePlugins[0];
>+  } else {
>+    testDisablePlugin.__force_skip__= "At least 1 plugin must be enabled"

For consistency with other tests, please use the message "No enabled plugins detected"

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

Nit: Fix the indentation here

>+  // Skip test if we have no plugins
>+  if (persisted.enabledPlugins < 1) {
>+    testDisablePlugin.__force_skip__ = "At least 1 plugin must be enabled";
>+    teardownModule.__force_skip__ = "At least 1 plugin must be enabled";

For consistency with other tests, please use the message "No enabled plugins detected"
Comment 32 Alex Lakatos[:AlexLakatos] 2012-08-01 09:28:19 PDT
Created attachment 647988 [details] [diff] [review]
patch v9.0

fixed nits
Comment 33 Dave Hunt (:davehunt) 2012-08-01 13:08:34 PDT
Comment on attachment 647988 [details] [diff] [review]
patch v9.0

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/9eb71d44ba8b (default)
Comment 34 Henrik Skupin (:whimboo) 2012-08-06 05:45:17 PDT
Any reason why this bug is still open? Test landed 5 days ago and seems to stick passing. Lets close this bug out.

Note You need to log in before you can comment on or make changes to this bug.