Closed Bug 478845 Opened 14 years ago Closed 13 years ago

Disable mochitest of test plugin if test plugin is not built

Categories

(Core Graveyard :: Plug-ins, defect)

All
OpenSolaris
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #362689 - Flags: review?(joshmoz)
I meant this is a follow-up of bug 474503.
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!
Ted, can we just use get_test_plugin() ?
Attachment #362689 - Flags: review?(roc)
Attached patch patch v2 (obsolete) — Splinter Review
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).
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?
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?
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-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #363286 - Attachment is obsolete: true
Attachment #364052 - Flags: review?(jwalden+bmo)
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-
Jeff, I've trouble to make ("Components" in this) work there.
I think "this" is class PluginUtils here.
Attached patch patch v4 (obsolete) — Splinter Review
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+
Attached patch patch v5Splinter Review
Attachment #368207 - Attachment is obsolete: true
Attachment #373809 - Flags: review?(jwalden+bmo)
Attachment #373809 - Flags: review?(jwalden+bmo) → review+
Pushed:

http://hg.mozilla.org/mozilla-central/rev/f46bebbe48e6
Status: ASSIGNED → RESOLVED
Closed: 13 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?
(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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.