Disable mochitest of test plugin if test plugin is not built

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Plug-ins
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Ginn Chen, Assigned: Ginn Chen)

Tracking

Trunk
mozilla1.9.2a1
All
OpenSolaris
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
This is a follow-up of bug 471759.

Test plugin is not built for port platforms.
We should also disable the mochitest of test plugin.

I found test plugin works fine on Solaris.
So I enabled it on Solaris.
(Assignee)

Comment 1

10 years ago
Created attachment 362689 [details] [diff] [review]
patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #362689 - Flags: review?(joshmoz)
(Assignee)

Comment 2

10 years ago
I meant this is a follow-up of bug 474503.

Updated

10 years ago
Attachment #362689 - Flags: review?(joshmoz) → review?(roc)
Ted, can you come up with a scalable solution here? We don't want to sprinkle this architcture-test macro all over the place because we'll forget to update it if we add a supported test plugin platform.
My thought was to add a function to the test harness like haveTestPlugin(), so you could do something like:

if (!haveTestPlugin()) {
  todo(false, "Need a test plugin on this platform");
}
else {
  // run some tests...
}

haveTestPlugin() could basically reuse the code I wrote for reftest:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#241
OK, Ginn, can you add that to SimpleTest.js and the tests? Thanks!
(Assignee)

Comment 6

10 years ago
Ted, can we just use get_test_plugin() ?
(Assignee)

Updated

10 years ago
Attachment #362689 - Flags: review?(roc)
(Assignee)

Comment 7

10 years ago
Created attachment 363286 [details] [diff] [review]
patch v2
Attachment #362689 - Attachment is obsolete: true
Attachment #363286 - Flags: review?(ted.mielczarek)
I don't see why, I think my version is just as good (and doesn't require UniversalXPConnect privileges).
(Assignee)

Comment 9

10 years ago
I just thought it would be easier to reuse the code in test_bug391728.html, if there's no functional difference.

Should I copy "haveTestPlugin" to test_bug391728.html or somewhere else?

I think I don't need "enabledPlugin != null" for test_bug391728.html, right?
(Assignee)

Comment 10

10 years ago
Ted,

I found the difference of "haveTestPlugin" and "get_test_plugin".

If the test plugin is disabled, "haveTestPlugin" will not see the plugin, "get_test_plugin" still returns the plugin.
Since we're testing plugin.disabled later in this file, I think "get_test_plugin" more appropriate here.
Ok, that sounds reasonable. We should put this function in a common Mochitest file. Waldo: where are we keeping utility functions like this in Mochitest?

Comment 12

10 years ago
EventUtils and WindowSnapshot both make the case for somewhere in testing/mochitest/tests/SimpleTest/, in a standalone file as this wouldn't need to hook into the test-execution sequence.
Comment on attachment 363286 [details] [diff] [review]
patch v2

r- because I'd like to see this function move to the mochitest harness. Please r? jwalden if you write a patch to do that.
Attachment #363286 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 14

10 years ago
Created attachment 364052 [details] [diff] [review]
patch v3
Attachment #363286 - Attachment is obsolete: true
Attachment #364052 - Flags: review?(jwalden+bmo)

Comment 15

10 years ago
Comment on attachment 364052 [details] [diff] [review]
patch v3

>diff --git a/testing/mochitest/tests/SimpleTest/plugin.js b/testing/mochitest/tests/SimpleTest/plugin.js

>+function get_test_plugin() {
>+  var ph = Components.classes["@mozilla.org/plugin/host;1"]
>+                     .getService(Components.interfaces.nsIPluginHost);
>+  var tags = ph.getPluginTags({});
>+  
>+  // Find the test plugin
>+  for (var i = 0; i < tags.length; i++) {
>+    if (tags[i].name == "Test Plug-in")
>+      return tags[i];
>+  }
>+}

An alternative strawman API:

var PluginUtils =
  {
    withTestPlugin: function(callback)
    {
      if (!("Components" in this))
      {
        todo(false, "not a Mozilla-based browser");
        return;
      }

      var ph = Components.classes["@mozilla.org/plugin/host;1"]
                         .getService(Components.interfaces.nsIPluginHost);
      var tags = ph.getPluginTags({});

      // Find the test plugin
      for (var i = 0; i < tags.length; i++)
      {
        if (tags[i].name == "Test Plug-in")
        {
          callback(tags[i]);
          return;
        }
      }
    };
  };

This would make it harder to blindly assume the test plugin exists.  Thoughts?  Also, on less substantial concerns, please use camelCaps naming for APIs instead of under_scoring, and make the file something like PluginUtils.js or similar (every word in the identifier capitalized), but in any case I think this isn't quite what we want.
Attachment #364052 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 16

10 years ago
Jeff, I've trouble to make ("Components" in this) work there.
I think "this" is class PluginUtils here.
(Assignee)

Comment 17

10 years ago
Created attachment 368207 [details] [diff] [review]
patch v4
Attachment #364052 - Attachment is obsolete: true
Attachment #368207 - Flags: review?(jwalden+bmo)
Comment on attachment 368207 [details] [diff] [review]
patch v4

Sorry for the delay on this...

>diff --git a/content/base/test/test_bug391728.html b/content/base/test/test_bug391728.html

>+  if (!PluginUtils.withTestPlugin(start_test))
>+    SimpleTest.finish();

Nice call on making the return value indicate whether the test plugin was present or not, should have thought of doing that myself.


>   // We must delay to wait for the plugin sources to be loaded :(
>-  setTimeout(gNextTest, 500);
>+  setTimeout("PluginUtils.withTestPlugin(gNextTest)", 500);

Make this |function() { PluginUtils.withTestPlugin(gNextTest); }| please.  The string form of setTimeout is more easily susceptible to abuse than the lambda form, in general, and I'd rather our code be idiomatic.


>diff --git a/testing/mochitest/tests/SimpleTest/PluginUtils.js b/testing/mochitest/tests/SimpleTest/PluginUtils.js

>+var PluginUtils =
>+{
>+  withTestPlugin : function(callback)
>+  {
>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+    var ph = Components.classes["@mozilla.org/plugin/host;1"]
>+                       .getService(Components.interfaces.nsIPluginHost);

Oh, rookie mistake on my part forgetting about this-binding like that.  Guard with |if (typeof Components === "undefined") return false;| instead.
Attachment #368207 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 19

9 years ago
Created attachment 373809 [details] [diff] [review]
patch v5
Attachment #368207 - Attachment is obsolete: true
Attachment #373809 - Flags: review?(jwalden+bmo)

Updated

9 years ago
Attachment #373809 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 20

9 years ago
Pushed:

http://hg.mozilla.org/mozilla-central/rev/f46bebbe48e6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 368207 [details] [diff] [review]
patch v4

>diff --git a/modules/plugin/Makefile.in b/modules/plugin/Makefile.in
> ifdef ENABLE_TESTS
>-ifneq (,$(filter WINNT Darwin Linux,$(OS_ARCH)))
>+ifneq (,$(filter WINNT Darwin Linux SunOS,$(OS_ARCH)))
> DIRS += test

If this patch makes the test produce a reasonable TODO on all platforms, why only run it on these?
(Assignee)

Comment 22

9 years ago
(In reply to comment #21)
> (From update of attachment 368207 [details] [diff] [review])
> >diff --git a/modules/plugin/Makefile.in b/modules/plugin/Makefile.in
> > ifdef ENABLE_TESTS
> >-ifneq (,$(filter WINNT Darwin Linux,$(OS_ARCH)))
> >+ifneq (,$(filter WINNT Darwin Linux SunOS,$(OS_ARCH)))
> > DIRS += test
> 
> If this patch makes the test produce a reasonable TODO on all platforms, why
> only run it on these?

Because the test plugin may not built successfully on some platforms, it would block the build process.
You need to log in before you can comment on or make changes to this bug.