Jason Duell Assignee
	Description • 15 years ago

We've got a lot of necko xpcshell tests, which can logically be run in either the content or chrome processes (we need to test both paths).

Right now these tests can't be run in content.  Xpcshell runs all scripts in chrome, and the only way to have code run in content is to use sendCommand().

I tried to write simple wrapper scripts that do

  sendCommand("load('test_foo.js');"); 
  sendCommand("run_test()");

But this runs into the problem that the test completes immediately (before the content process even runs, probably.   We need to add a "do_test_pending" here, but if there is already a do_test_pending in the original test script, it hangs (could we fix that?).

I then did a tweak where I changed "run_test()" in the original script to contain only a call to "run_test_really(); do_test_pending();", with all the actual test logic moved to run_test_really().  This allows the test to be run standalone in chrome, as before, but also lets it be run in content without issuing multiple do_test_pending calls:

  sendCommand("load('test_foo.js');"); 
  sendCommand("run_test_really()"); 
  do_test_pending();

This seemed promising, but what we run into next is that the xpcshell environment in the content process is not identical to that in chrome.  We are not running head.js before client code is run, and so functions like do_load_httpd_js() are not available.   Manually loading head.js doesn't fix the problem, as it relies on runxpcshelltests.py to set certain constants, and when they are absent, it barfs.

For the moment I'm going to see if I can get away with loading/starting/stopping httpd in the chrome process, and do the rest of the test in content.  But this requires significant re-working of each test.   Going forward, it would be nice if we could arrange to have a regular, fully functional xpcshell test environment for content process tests.  And ideally a flag to simply tell xpcshell to "run this script in content", rather than forcing all work to be done via sendCommand().

	Jason Duell Assignee
	Comment 1 • 15 years ago

I'm close to a fix that will allow xpcshell tests to be run in the content process unmodified, requiring only a trivial wrapper script for each test.

	Jason Duell Assignee
	Comment 2 • 15 years ago

OK, so this patch adds a new function that lets us easily run an xpcshell test in the content process under e10s (note: this patch is for the e10s branch only--is there any way to mark this in Bugzilla?).

How to add client-side tests (will add these instructions to wiki if patch taken.  The patch has one example test added):

1) create a directory called "unit_ipc" (or whatever) and add it to your Makefile.in's XPCSHELL_TESTS variable only when MOZ_IPC is set. (This is because we will have some platforms that won't work with e10s, so we keep client-side tests in a separate directory.  This is on ted's advice).

2) If you've already got a existing chrome-side test that you also want tested in the child process (many necko tests are like this), simply write a wrapper script ("unit_ipc/test_existing_wrap.js") that does nothing but call "run_test_in_child('../unit/test_existing.js");"  That's it.

3) If you're writing a new test that should only run in the child process, put the test logic into a file that /doesn't/ start with "test_" (say, "unit_ipc/newtest.js"), and then create a file "unit_ipc/test_newtest.js" that calls run_test_in_child("newtest.js");

Client-side tests have the full head.js environment set up and we're getting  stdout/err from the client side.

Tested on Linux/Win32/OSX.  When I test with wrappers for the full set of necko tests (not in patch), I do see some of them hang.  I'm not sure why yet. The failures are not intermittent--the included test always works.  Hopefully that's good enough for starters?

Tests currently leak a PNeckoParent and PTestShellParent each time--will open separate bugs for those.

	Jason Duell Assignee
	Comment 3 • 15 years ago

Note: if we'll have some tests that need to do work in both the chrome and child processes, we may want to rework this patch so that it adds a "setup_child()" function that loads head.js in the child.  Right now commands run with sendCommand don't have any head.js functionality, so they can't even do_throw(). Perhaps we could do a lazy init of the child process's head.js the first time sendCommand is called?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 4 • 15 years ago

Comment on attachment 408546 [details] [diff] [review]
Adds run_test_in_child() xpcshell function: runs script in child process under e10s.

>+print("*************** _HEAD_JS_PATH=" + _HEAD_JS_PATH);

These prints should be removed.

>+function run_test_in_child(test)

This looks really good, the only thing I would suggest is to separate the loading of head.js and the running of the test file. Maybe have a load_test_harness() and a run_test_in_child() that simply calls that? That way you could write tests that don't necessarily use the _execute_test framework but could still have do_throw, etc.

>+function _addQuotes(arg) {

I think you should make this an inner function of run_test_in_child to keep it out of the public functions of head.js.

Oh, and since this is modifying the test harness itself I think we'll need ted to sign off on it too.

	Jason Duell Assignee
	Comment 5 • 15 years ago

OK, I split up the logic into "do_load_child_test_harness()", which loads head.js in the child process, and run_test_in_child(), which does both that and loads/runs a test script.

The patch actually now contains the simple HTTP tests I mentioned, too.

	(not currently active) Ted Mielczarek
	Comment 6 • 15 years ago

Comment on attachment 408716 [details] [diff] [review]
New improved patch. "Last bug fixed"

>diff --git a/netwerk/test/unit_ipc/head_channels_clone.js b/netwerk/test/unit_ipc/head_channels_clone.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit_ipc/head_channels_clone.js
>@@ -0,0 +1,2 @@
>+load("../unit/head_channels.js");

Could you add a comment to this file indicating why it's here? It took me a little bit to realize that you wanted that code loaded in the child process.

>diff --git a/netwerk/test/unit_ipc/test_simple_wrap.js b/netwerk/test/unit_ipc/test_simple_wrap.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit_ipc/test_simple_wrap.js
>@@ -0,0 +1,3 @@
>+function run_test() {
>+  run_test_in_child("../unit/test_simple.js");
>+}

A short comment here as well explaining what you're testing would probably be good.

>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>@@ -398,8 +398,64 @@ function do_get_profile() {

>+/**
>+ * This function loads head.js (this file) in the child process, so that all
>+ * functions defined in this file (do_throw, etc) are available to subsequent
>+ * sendCommand calls.
>+ *
>+ * (Note that you may use sendCommand without calling this function first;  you
>+ * simply won't have any of the functions in this file available.)
>+ */
>+function do_load_child_test_harness()

The comment should mention that it also sets some prerequisite constants for the test harness.

>+  let addQuotes = function (str) { return '"' + str + '"'; }

Why wouldn't you just write this as function addQuotes(str) { ... }?

>+  sendCommand(
>+        "const _HEAD_JS_PATH='" + _HEAD_JS_PATH + "'; "
>+      + "const _HTTPD_JS_PATH='" + _HTTPD_JS_PATH + "'; "
>+      + "const _HEAD_FILES=[" + quoted_head_files.join() + "];"
>+      + "const _TAIL_FILES=[" + quoted_tail_files.join() + "];"

Could you make those .join(",")? I realize that's the default, but for some
reason I can't ever remember that.

>+/**
>+ * Runs an entire xpcshell unit test in a child process (rather than in chrome,
>+ * which is the default).
>+ *
>+ * @param test
>+ *        The name of the script to run.  A relative path may be provided (in
>+ *        Unix format, i.e. use forward slashes), starting from the base
>+ *        directory of whatever script invokes this function.
>+ */

I'd just say that the path format is exactly what load() takes. You should document here that this function will return before the test is finished executing. You also might consider adding an optional callback parameter so someone could write a test that executes a test in the child, and gets some notification upon completion.

>+function run_test_in_child(test) 
>+{
>+  do_load_child_test_harness();
>+
>+  var testPath = do_get_file(test).path.replace(/\\/g, "/");

This is sort of gross, but I guess that's life. You can use a string instead of a regex there, and put the g in the (nonstandard) third argument:
.replace('\\', '/', 'g')

>+  sendCommand("const _TEST_FILE=['" + testPath + "']; _execute_test();", 
>+              do_test_finished);
>+  do_test_pending();

I think you should put the do_test_pending before the sendCommand, it just seems to make more sense. Also, the sendCommand callback will not fire until the script is fully done executing, right? I can't find any useful documentation on what sendCommand() (or the bits underlying it) do.

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>   # <head.js> has to be loaded by xpchell: it can't load itself.
>   xpcsCmd = [xpcshell, '-g', xrePath, '-j', '-s'] + \
>             ['-e', 'const _HTTPD_JS_PATH = "%s";' % httpdJSPath,
>+             '-e', 'const _HEAD_JS_PATH = "%s";' % headJSPath, 
>              '-f', os.path.join(testharnessdir, 'head.js')]

Could you add a comment here noting that if you change any of these constants, you should change do_load_child_test_harness in head.js as well?

Also, I'd like to see a few sanity tests of this. You could put them in ipc/testshell/tests if you'd like. Just something like:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/example/unit/test_sample.js

but run in the child process would be fine.

I'm a little curious as to how this will work with our existing test harness, since it has to grep the output to check for failure. If you did something non-trivial with tests executing in both the parent and the child, do you run the risk of having the output intermingled? As long as it's line-buffered that should be ok.

Anyway, r=me with those things addressed.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 7 • 15 years ago

Comment on attachment 408716 [details] [diff] [review]
New improved patch. "Last bug fixed"

Yeah, what ted said!

	Jason Duell Assignee
	Comment 8 • 15 years ago

Added suggested comments.

> Why wouldn't you just write this as function addQuotes(str) { ... }?

Fixed.

> Could you make those .join(",")? 

Um, there's a reason why they call it syntactic "sugar".  Dumping all the substrings into an array and calling join on it strikes me as gross.  Not taken.

> I think you should put the do_test_pending before the sendCommand

Done.

> Also, I'd like to see a few sanity tests of this. You could put them in
> ipc/testshell/tests if you'd like. 

Actually, no I can't--I would need to create a separate ipc/testshell/tests_ipc (or something) so that they'd only get run if MOZ_IPC.  I think the existing necko test hits enough standard head.js features that we can consider it a sanity test.

> I'm a little curious as to how this will work with our existing test harness,
> since it has to grep the output to check for failure.

Output seems to be line-buffered, so we're OK.  But thanks for making me think of this--I realized my fix to print "child" or "parent" at the front of each line /did/ break error parsing, so I've fixed that in bug 528145.

Asking Ted for another review in case he wants some of the things I didn't take.

	Jason Duell Assignee
	Comment 9 • 15 years ago

This is ready to checkin.  No significant changes since ted and bent r+sr'd it.

	Jason Duell Assignee
	Comment 10 • 15 years ago

Note that this should only be checked into the electrolysis repo for now!  :)

	Ben Newman (:bnewman) (:benjamn)
	Comment 12 • 15 years ago

Since I've been using this patch for a while without any problems, and it has reviews, I've taken the liberty of landing it:

http://hg.mozilla.org/projects/electrolysis/rev/a77022f7680c

Thanks for figuring this stuff out, jduell!