Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Mozmill test for Disabled state of Plugin remembered after a Restart

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: RemusPop, Assigned: AlexLakatos)

Tracking

unspecified

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [mozmill-functional][mozmill-aom])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Tracking creation of a mozmill test that checks if a disabled plugin remains disabled after browser restart.
(Reporter)

Comment 1

6 years ago
The testcase can be found here:
https://litmus.mozilla.org/show_test.cgi?id=16022
Summary: Mozmill test for checking if a disabled plugin is disabled after restart → Disabled state of Plugin is remembered after a Restart
Whiteboard: [mozmill-aom]
(Reporter)

Updated

6 years ago
Depends on: 687450

Comment 2

6 years ago
Remus Pop changed story state to started in Pivotal Tracker
(Reporter)

Comment 3

6 years ago
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.
Attachment #567057 - Flags: review?(vlad.mozbugs)
(Assignee)

Comment 4

6 years ago
>+  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.
(Reporter)

Comment 5

6 years ago
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.
Attachment #567057 - Attachment is obsolete: true
Attachment #567057 - Flags: review?(vlad.mozbugs)
Attachment #567066 - Flags: review?(vlad.mozbugs)
(Reporter)

Updated

6 years ago
Attachment #567066 - Flags: review?(vlad.mozbugs) → review?(alex.lakatos)
(Assignee)

Comment 6

6 years ago
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"
Attachment #567066 - Flags: review?(alex.lakatos) → review-
(Reporter)

Comment 7

6 years ago
Created attachment 568031 [details] [diff] [review]
patch v3

Addressed all issues.
Attachment #567066 - Attachment is obsolete: true
Attachment #568031 - Flags: review?(alex.lakatos)
(Assignee)

Updated

6 years ago
Attachment #568031 - Flags: review?(anthony.s.hughes)
Attachment #568031 - Flags: review?(alex.lakatos)
Attachment #568031 - Flags: review+
Summary: Disabled state of Plugin is remembered after a Restart → Mozmill test for Disabled state of Plugin remembered after a Restart
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.
Attachment #568031 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 9

6 years ago
(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.
(Reporter)

Comment 10

6 years ago
Created attachment 570712 [details] [diff] [review]
patch v4
Attachment #568031 - Attachment is obsolete: true
Attachment #570712 - Flags: review?(hskupin)
(Reporter)

Updated

6 years ago
Attachment #570712 - Flags: review?(hskupin) → review?(gmealer)
(Reporter)

Updated

6 years ago
Attachment #570712 - Flags: review?(gmealer) → review?(anthony.s.hughes)
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.
Attachment #570712 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 12

6 years ago
Created attachment 574556 [details] [diff] [review]
patch v5

All requests have been satisfied.
Attachment #570712 - Attachment is obsolete: true
Attachment #574556 - Flags: review?(anthony.s.hughes)
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
Attachment #574556 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 14

6 years ago
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.
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
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.
(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.
(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.
Whiteboard: [mozmill-aom] → [mozmill-functional][mozmill-aom]
(Reporter)

Comment 19

6 years ago
Anthony, can you please update your version of Flip4Mac to the latest and try to run this test again?
The last time I tested this (comment 17), I had updated to the latest. I still get this error.
(Reporter)

Comment 21

6 years ago
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.
Mac OSX 10.6.8
Mozmill 1.5.7
Flip4Mac 2.4.0.11
This test has been lost in the nirvana. Alex, please take care of it.
Assignee: remus.pop → alex.lakatos
(Assignee)

Comment 24

5 years ago
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.
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.
(Assignee)

Comment 26

5 years ago
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
Attachment #574556 - Attachment is obsolete: true
Attachment #644921 - Flags: review?(hskupin)
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.
Attachment #644921 - Flags: review?(hskupin) → review-
(Assignee)

Comment 28

5 years ago
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?
Attachment #644921 - Attachment is obsolete: true
Attachment #646643 - Flags: review?(dave.hunt)
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
Attachment #646643 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 30

5 years ago
Created attachment 647587 [details] [diff] [review]
patch v8.0

fixed commit message. used getInstalledAddons()
Attachment #646643 - Attachment is obsolete: true
Attachment #647587 - Flags: review?(dave.hunt)
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"
Attachment #647587 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 32

5 years ago
Created attachment 647988 [details] [diff] [review]
patch v9.0

fixed nits
Attachment #647587 - Attachment is obsolete: true
Attachment #647988 - Flags: review?(dave.hunt)
Comment on attachment 647988 [details] [diff] [review]
patch v9.0

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/9eb71d44ba8b (default)
Attachment #647988 - Flags: review?(dave.hunt) → review+
Any reason why this bug is still open? Test landed 5 days ago and seems to stick passing. Lets close this bug out.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox17: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.