Disable mochitest of test plugin if test plugin is not built

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

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

Tracking

Trunk
mozilla1.9.2a1
All
OpenSolaris
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Posted 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)
Posted 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-
Posted 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.
Posted 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+
Posted 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: 10 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.
You need to log in before you can comment on or make changes to this bug.