478845 - Disable mochitest of test plugin if test plugin is not built

Status: RESOLVED FIXED

Description

[Ginn Chen]

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.

Comment 1

[Ginn Chen]

Created attachment 362689 [details] [diff] [review]
patch

Comment 2

[Ginn Chen]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Ted Mielczarek [:ted] [:ted.mielczarek]]

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, Ginn, can you add that to SimpleTest.js and the tests? Thanks!

Comment 6

[Ginn Chen]

Ted, can we just use get_test_plugin() ?

Comment 7

[Ginn Chen]

Created attachment 363286 [details] [diff] [review]
patch v2

Comment 8

[Ted Mielczarek [:ted] [:ted.mielczarek]]

I don't see why, I think my version is just as good (and doesn't require UniversalXPConnect privileges).

Comment 9

[Ginn Chen]

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?

Comment 10

[Ginn Chen]

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.

Comment 11

[Ted Mielczarek [:ted] [:ted.mielczarek]]

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

[Jeff Walden [:Waldo]]

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 13

[Ted Mielczarek [:ted] [:ted.mielczarek]]

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.

Comment 14

[Ginn Chen]

Created attachment 364052 [details] [diff] [review]
patch v3

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Ginn Chen]

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

Comment 17

[Ginn Chen]

Created attachment 368207 [details] [diff] [review]
patch v4

Comment 18

[Jeff Walden [:Waldo]]

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.

Comment 19

[Ginn Chen]

Created attachment 373809 [details] [diff] [review]
patch v5

Comment 20

[Ginn Chen]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/f46bebbe48e6

Comment 21

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

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?

Comment 22

[Ginn Chen]

(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.