Closed Bug 739935 Opened 8 years ago Closed 8 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(firefox12 verified, firefox13 verified, firefox14 verified, firefox15 verified, firefox-esr10 unaffected, status1.9.2 unaffected)

VERIFIED FIXED
Tracking Status
firefox12 --- verified
firefox13 --- verified
firefox14 --- verified
firefox15 --- verified
firefox-esr10 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: remus.pop, Assigned: remus.pop)

References

()

Details

(Whiteboard: [lib][mozmill-test-failure])

Attachments

(2 files, 4 obsolete files)

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.
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.
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.
But plugin.name should not be undefined then.
I think that was a problem when the test was written due to lack of observation or the property name has been made obsolete.
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
Attachment #613819 - Flags: review?(hskupin)
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)
Attachment #613819 - Flags: review?(hskupin) → review+
(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.
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.
Attachment #614290 - Flags: review?(hskupin)
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?
Attachment #614290 - Flags: review?(hskupin) → review-
(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.
Since fix for failure message has landed, message is now "Shockwave Flash has been disabled - 'true' should equal 'false'"
Summary: Failure in testPluginDisableAffectsAboutPlugins | undefined has been disabled - 'true' should equal 'false' → Failure in testPluginDisableAffectsAboutPlugins | Shockwave Flash has been disabled - 'true' should equal 'false'
Any progress on this yet Remus?
Whiteboard: [lib]
(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.
I used pluginID because persisted.plugin changes after enetring the test function so I needed the ID when enabling it.
Attachment #614290 - Attachment is obsolete: true
Attachment #616565 - Flags: review?(hskupin)
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
Whiteboard: [lib] → [lib][mozmill-test-failure]
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.
Attachment #616565 - Flags: review?(hskupin) → review-
Status: ASSIGNED → NEW
Depends on: 747308
Remus, can we please get an update on this failure? I kinda would like to see this fixed. Thanks.
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.
Duplicate of this bug: 751112
Any update here? The turnaround for updates is quite slow at the moment. I would really like to see this failure fixed.
I have some work in progress and I'll finish it today.
I believe this bug status should be changed to "assigned"
Status: NEW → ASSIGNED
I left pluginID so we can safely activate back the plugin in teardownModule.
Attachment #616565 - Attachment is obsolete: true
Attachment #620313 - Flags: review?(hskupin)
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.
Attachment #620313 - Flags: review?(hskupin) → review-
Adressed all requests.
Attachment #620313 - Attachment is obsolete: true
Attachment #621008 - Flags: review?(hskupin)
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.
Attachment #621008 - Flags: review?(hskupin) → review-
Sorry about the TIMEOUT. I had a hard time testing.
All requests were addressed.
Attachment #621008 - Attachment is obsolete: true
Attachment #621021 - Flags: review?(hskupin)
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.
Attachment #621021 - Flags: review?(hskupin) → review+
Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4c6f4ef15963
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
No failures seen past May 9th, verified fixed.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.