739935 - Failure in testPluginDisableAffectsAboutPlugins | Shockwave Flash has been disabled - 'true' should equal 'false'

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

This is failing since 26th of March, on Nightly and on linux only.

I cannot reproduce the test locally, but I can see that the undefined appears in my output too, though  it should be the plugins' name.

Comment 1

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

So that would mean 'persisted.plugin' has not been set in setupModule. But update tests for the same machine show that plugins are installed and even enabled:

http://mozmill-release.blargon7.com/#/update/report/d7273b70010f02a4bbdbf44f1738a019

I kinda feel that we should update our code to use the Add-ons Manager to retrieve available plugins to test.

Comment 2

[Remus Pop (:RemusPop)]

I was thinking that when we check for disabled plugin, Firefox does not have enough time to disable it.

  addonsManager.disableAddon({addon: plugin});

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

So maybe a waitFor instead or before of this assert would do some good.

Comment 3

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

But plugin.name should not be undefined then.

Comment 4

[Remus Pop (:RemusPop)]

I think that was a problem when the test was written due to lack of observation or the property name has been made obsolete.

Comment 5

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

Attachment: Fixed failure messages and made Mozmill 2.0 compatible. v1.0

This patch fixes the failure messages so they won't include 'undefined'. It also makes the test compatible with Mozmill 2.0 by using plugin.getNode() instead of plugin.node

Comment 6

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

Comment on attachment 613819 [details] [diff] [review]
Fixed failure messages and made Mozmill 2.0 compatible. v1.0

Looks good. I will land it on all branches but leave this bug open. Reason is that we still have to fix the remaining issue with the undefined name.

Remus, would you mind updating the plugin retrieval code in the setupModule method to use the AddonManager API instead? We would only have to filter for plugins and also take the first entry. If none is present we wanna skip this test.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/7d8e8e378cd2 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/a5bc1b9ff09c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/48ead6667912 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/866ffed43256 (release)

Comment 7

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Looks good. I will land it on all branches but leave this bug open. Reason
> is that we still have to fix the remaining issue with the undefined name.
> 
> Remus, would you mind updating the plugin retrieval code in the setupModule
> method to use the AddonManager API instead? We would only have to filter for
> plugins and also take the first entry. If none is present we wanna skip this
> test.

I'm on it.

Comment 8

[Remus Pop (:RemusPop)]

Attachment: Plugin retrieval update v1 (all branches)

Defined a new function which gets enabled plugins from the Addons Manager.
It seems pluginNames from pluginExistsInAboutPlugins() missed being declared so that's also fixed.

Comment 9

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

Comment on attachment 614290 [details] [diff] [review]
Plugin retrieval update v1 (all branches)

>+  var plugins = getEnabledPlugins();

You want to use the addons.getInstalledAddons() function here. There is no need to define another method for this test. Create the appropriate callback which filters for enabled plugins.

>+  if (plugins.length < 1) {
>     testDisableEnablePlugin.__force_skip__ = "No enabled plugins detected";
>     teardownModule.__force_skip__ = "No enabled plugins detected";

Please compare to === 0

> function testDisableEnablePlugin() {
>-  addonsManager.open();
>-
>-  // Select the Plugins category
>-  addonsManager.setCategory({
>-    category: addonsManager.getCategoryById({id: "plugin"})
>-  });

This is part of the test and has to be left in here.

>+  tabBrowser.selectedIndex = 1;

Please help me, why is that necessary?

Comment 10

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> >+  tabBrowser.selectedIndex = 1;
> 
> Please help me, why is that necessary?

Must've slipped somehow. I see now that it works without that. Oops, I see there are more leftovers.

Comment 11

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

Since fix for failure message has landed, message is now "Shockwave Flash has been disabled - 'true' should equal 'false'"

Comment 12

[u279076]

Any progress on this yet Remus?

Comment 13

[Remus Pop (:RemusPop)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> You want to use the addons.getInstalledAddons() function here. There is no
> need to define another method for this test. Create the appropriate callback
> which filters for enabled plugins.

I will use that to get all addons but a filter will output "void 0" objects for no matching addon. So I've filtered the results in a forEach loop.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: Plugin retrieval update v2 (all branches)

I used pluginID because persisted.plugin changes after enetring the test function so I needed the ID when enabling it.

Comment 15

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

Comment on attachment 616565 [details] [diff] [review]
Plugin retrieval update v2 (all branches)

>+  var allAddons = addons.getInstalledAddons();
>+  var plugins = new Array();
>+  allAddons.forEach(function (addon) {
>+    if (addon.type === "plugin" && addon.isActive === true)
>+      plugins.push(addon);
>+  });

We have to fix this in the addons module. So we only add addons to the list if the callback filter returns true. Exactly what you have here.

>-    persisted.plugin = controller.window.navigator.plugins[0];
>+    persisted.plugin = plugins[0];
>+    persisted.pluginID = persisted.plugin.id;

I don't see a reason why pluginID has to be stored separately.

>   expect.ok(!pluginExistsInAboutPlugins(),
>-            persisted.plugin.getNode().mAddon.name + " does not appear in about:plugins");
>+            persisted.plugin.getNode().mAddon.name +
>+            " does not appear in about:plugins");

Can you please store the value of 'persisted.plugin.getNode().mAddon.name' in a variable, pass it over to pluginExistsInAboutPlugins() and use it in the expect message? That would make the code way cleaner and less magic.

Comment 16

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

Remus, can we please get an update on this failure? I kinda would like to see this fixed. Thanks.

Comment 17

[Remus Pop (:RemusPop)]

We'll need to use pluginID in case something happens with persisted.plugin and we cannot get the id from that. I say that enabling the plugin will be bullet proof.

Comment 19

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

Any update here? The turnaround for updates is quite slow at the moment. I would really like to see this failure fixed.

Comment 20

[Remus Pop (:RemusPop)]

I have some work in progress and I'll finish it today.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

I believe this bug status should be changed to "assigned"

Comment 22

[Remus Pop (:RemusPop)]

Attachment: Plugin retrieval update v3 (all branches)

I left pluginID so we can safely activate back the plugin in teardownModule.

Comment 23

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

Comment on attachment 620313 [details] [diff] [review]
Plugin retrieval update v3 (all branches)

>Bug 739935 - Updated retrieval of plugins r=hskupin

Please make the comment more descriptive what you are fixing here.

>+  var activePlugins = addons.getInstalledAddons(function(addon) {

nit: blank between 'function' and '(' and use 'aAddon' for the parameter.

>+    if (addon.isActive && addon.type == "plugin")

nit: please use '===' for the comparison.

>+  if (activePlugins.length === 0) {

Can we flip both conditions so that the skip code comes last?

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

I still don't like that. Why do we have to use persisted and not a global variable instead? Also we should never overwrite persisted.plugin or however it is called.

> function teardownModule() {
>   delete persisted.plugin;
>+  delete persisted.pluginID;

You missed to close the add-ons manager. That can break follow-up tests, especially with Mozmill 2.

> function testDisableEnablePlugin() {
>   addonsManager.open();
>-
>+  

Please take care of those unnecessary white-space changes.

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

IMO the message makes the comment unnecessary.

Comment 24

[Remus Pop (:RemusPop)]

Attachment: Plugin retrieval update v4 (all branches)

Adressed all requests.

Comment 25

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

Comment on attachment 621008 [details] [diff] [review]
Plugin retrieval update v4 (all branches)

>-        }, "Selected category has been loaded.", TIMEOUT, undefined, this);
>+        }, "Selected category has been loaded.", 10000, undefined, this);

Why that change? Please get rid of it.

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

>+  assert.equal(selectedPlugin.getNode().getAttribute("active"), "false",
>+               pluginName + " has been disabled");

Probably missed that before but therefore we have .isAddonEnabled().

>+  assert.ok(selectedPlugin.getNode().getAttribute("active"),
>+            pluginName + " has been enabled");

Same here.

Comment 26

[Remus Pop (:RemusPop)]

Attachment: Plugin retrieval update v5 (all branches)

Sorry about the TIMEOUT. I had a hard time testing.
All requests were addressed.

Comment 27

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

Comment on attachment 621021 [details] [diff] [review]
Plugin retrieval update v5 (all branches)

Looks good, Remus. I will check it in right away. Lets see the results for the daily testrun on horus.

Comment 28

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

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4c6f4ef15963

Comment 29

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

Missed to land the other patches. Done now:
http://hg.mozilla.org/qa/mozmill-tests/rev/be4ff83e60b8 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f3ef24918ee8 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/27f3f4048d6b (release)

Comment 30

[u279076]

No failures seen past May 9th, verified fixed.