Closed
Bug 478845
Opened 14 years ago
Closed 14 years ago
Disable mochitest of test plugin if test plugin is not built
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)
References
Details
Attachments
(1 file, 4 obsolete files)
7.84 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
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.
Comment 4•14 years ago
|
||
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!
Attachment #362689 -
Flags: review?(roc)
Attachment #362689 -
Attachment is obsolete: true
Attachment #363286 -
Flags: review?(ted.mielczarek)
Comment 8•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 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.
Comment 11•14 years ago
|
||
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•14 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 13•14 years ago
|
||
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•14 years ago
|
||
Attachment #363286 -
Attachment is obsolete: true
Attachment #364052 -
Flags: review?(jwalden+bmo)
Comment 15•14 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•14 years ago
|
||
Jeff, I've trouble to make ("Components" in this) work there. I think "this" is class PluginUtils here.
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #364052 -
Attachment is obsolete: true
Attachment #368207 -
Flags: review?(jwalden+bmo)
Comment 18•14 years ago
|
||
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•14 years ago
|
||
Attachment #368207 -
Attachment is obsolete: true
Attachment #373809 -
Flags: review?(jwalden+bmo)
Updated•14 years ago
|
Attachment #373809 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 20•14 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/f46bebbe48e6
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment 21•14 years ago
|
||
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•14 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.
Updated•9 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•