Closed Bug 519574 Opened 10 years ago Closed 10 years ago

Run plugin mochitests with OOP plugins on and off


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: benjamin, Assigned: jgriffin)




(1 file, 1 obsolete file)

We need to run the plugin mochitests with IPC on and off. Currently I think the pref is read the first time we load a plugin module, so dynamic changes to the pref aren't enough. We may just want a new test suite mochitest-ipcplugins or somesuch.
Assignee: nobody → jgriffin
Attached patch patch (obsolete) — Splinter Review
The attached patch allows you to run reftest with additional preferences set.  E.g.,

python --extra-pref=ipc.dom.plugins.enabled=true

bsmedberg, ted, do you think this is sufficient?
That's a good start. How will we have the tinderboxes call that configuration, though? Have a separate mochitest-ipcplugins target which calls that pref flag?
I think we should add a new target to testsuite-targets that runs mochitest with this extra pref + --test-path=modules/plugins or whatever the right path is there, to just run the plugin tests.
Attached patch patch v2Splinter Review
This patch adds a mochitest-oop make target that works as ted suggests:  it runs mochitest plain with --extra-pref=dom.ipc.plugins.enabled=true --test-path=modules/plugin/test
Attachment #405943 - Attachment is obsolete: true
Attachment #406132 - Flags: review?(ted.mielczarek)
Comment on attachment 406132 [details] [diff] [review]
patch v2

Looks good, just one change I'd like to see, and one nit.

diff -r 120721a177f0 build/
+  for v in extraPrefs:
+    ix = v.find("=")
+    if ix <= 0:
+      print "Error: syntax error in --extra-pref=" + v
+      sys.exit(1)
+    part = 'user_pref("%(prefname)s", %(prefval)s);\n' % {"prefname": v[:ix], "prefval": v[ix + 1:]}

That string slicing is kind of ugly. Just use v.split("=", 1) here, and you can still sanity check that len(result) == 2 (since that only guarantees that it will split at most once, it can still return a single item if there are no equals signs). Then you can just use result[0] and result[1]. Also, since you're just formatting two values into the string, I think using a dict and keynames in the format string is a little overkill. 'user_pref("%s", %s);' % (result[0], result[1]) should be plenty readable.

diff -r 120721a177f0 testing/mochitest/
+    self.add_option("--extra-pref",

Should we call this --setpref for consistency with --setenv? (Not that this file is a bastion of consistency, but still.)

r=me with those changes.
Attachment #406132 - Flags: review?(ted.mielczarek) → review+
bc, looks like you could easily port this to the reftest harness and use it to clean up that ugly bit I complained about in my review comment in bug 469718.
repeated occurrences of --extra-pref for each pref to be set? At the moment user.js has redundant preferences and could be reduced, but it would still require a number of prefs. Not sure how that rates on the ugly meter, but it would free up EXTRA_TEST_ARGS if someone wanted to use it for something else. If you want, I can do this after we get the tests running on the unittest machines.
Thanks Ted.  I'll make the suggested changes.  Ted and bsmedberg:  which repo should I put this in?  I was thinking of adding --setpref to both e10s and trunk, and the mochitest-oop makefile target only to e10s.  Does that sound right?
Yes, that sounds good.
Yeah, it makes sense to land as much in mozilla-central as possible to avoid merge pain for the future. Sounds like you've got the right plan.
Will it be appropriate for this to be landed on 1.9.2 and 1.9.1 at a later date?
The pref part could be if it were needed.
Bob: we take any test harness changes on any branch with blanket approval. I don't lock-step backport them, but usually just backport them as needed to unblock other work.
I looked at the original comment in this bug and realized bsmedberg's suggestion of calling the make target mochitest-ipcplugins is much better than mochitest-oop, so I've made that change:
(In reply to comment #0)
> We need to run the plugin mochitests with IPC on and off. Currently I think the
> pref is read the first time we load a plugin module, so dynamic changes to the
> pref aren't enough.

Is there a compelling reason not to make the pref live?
The plugin host is finicky, and you'd get the behavior of whatever the pref was the first time you loaded a plugin. After that, any new instances of the plugin would use the in- or out-of-process behavior that you started out with. So I'm a little worried about trying to make the behavior sane with a live pref.
I see mochitest-ipcplugins tests are not being run at  Ted, how we can get that test set running there?
I bet that has something to do with packaged unittests.
Yeah, you'll need to file a bug in Release Engineering asking that the electrolysis builders run this testsuite. The buildbot configs specify the exact commands to run, so adding new commands requires changes to the config.
Depends on: 523712
Depends on: 523894
Duplicate of this bug: 476886
This is FIXED, right?
Yes, although something along the way has broken the regular mochitests, but I don't think it's this bug... they work locally, just not on tbox.
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 525454
Duplicate of this bug: 442303
You need to log in before you can comment on or make changes to this bug.