1100695 - testChild() in selftest.py doesn't wait for child process to start up before shutting down the parent process

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Andrew McCreight [:mccr8]]

In bug 1091766, I'm changing the behavior of some IPC code to keep running when we hit an error, rather than just calling exit(0) immediately. One issue it has exposed is that the child process in testChild() crashes in startup, because it can't send a message to the parent.  It looks like it doesn't start running until after ContentParent has done ActorDestroy, so when the child process tries to send a message to the ContentParent, it fails, which causes us to abort in SendPNeckoConstructor.

Changing the body of add_test in CHILD_HARNESS_SIMPLE to
  do_load_child_test_harness();
  do_test_pending("some child");
  sendCommand("Assert.ok(true);", do_test_finished);
  run_next_test();

...seems to fix the crash, but I don't understand what this test is supposed to test, so maybe this is making it so we don't test anything.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

This is testing the reference error I introduced and fixed in bug 1093834 -- the version above still tests that. Thanks for tracking that down, I had no idea that introduced a crash.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: investigate failing test.

When I change the test to be as in comment 0, I get a new failure:
0:20.03 TEST-UNEXPECTED-FAIL | No test output (missing mozunit.main() call?): ./testing/xpcshell/selftest.py

This is generated by mach itself:
  http://mxr.mozilla.org/mozilla-central/source/python/mach_commands.py#109

This is weird because the log is running some code in the child process that does a TEST- thing, but somehow mach isn't seeing it?

If you uncomment the line that calls _testLogger.error in this patch, then you get a different error:
0:19.86 TEST-UNEXPECTED-FAIL | /var/folders/j7/00m_17816s5_1b4pl8crf6p00000gn/T/tmpZUBzbi/test_child_assertions.js | xpcshell return code: 0

It is expected that it fails, because I added a failure, but the weird thing is that now we don't get the original error about no test output.  I added some logging, and in this case, I think mach does see the trivial passing test that the sendCommand is supposed to do:
 TEST-PASS | /var/folders/j7/00m_17816s5_1b4pl8crf6p00000gn/T/tmpZUBzbi/test_child_assertions.js | None - true == true

So it seems like maybe the child process messages are being eating somewhere in the logging infrastructure in a way that mach doesn't see?

Comment 3

[Andrew McCreight [:mccr8]]

Would you mind looking into this, Chris?  It seems to involve a bunch of layers of our test harness framework that I'm entirely unfamiliar with.  Failing that, maybe we could disable this for now?  I don't know how much danger there is in this regressing.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

I'll look into this later this afternoon. I think that's all explained by the fact that process output is suppressed in runxpcshelltests.py if there's no failure, and maybe mach is doing further analysis of output that's slightly spurious in this case. 

I usually run these with make check, which is what our test infrastructure does. What command are you using to invoke these tests? Just |./mach python testing/xpcshell/selftest.py| ?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Chris Manchester [:chmanchester] from comment #4)
> I'll look into this later this afternoon.
Thanks!

> I think that's all explained by
> the fact that process output is suppressed in runxpcshelltests.py if there's
> no failure, and maybe mach is doing further analysis of output that's
> slightly spurious in this case.
That sounds very plausible.  I was trying to figure out how to just dump something in the output to make mach happy, but I couldn't come up with anything that worked.

> I usually run these with make check, which is what our test infrastructure
> does. What command are you using to invoke these tests? Just |./mach python
> testing/xpcshell/selftest.py| ?

Ah, so maybe it could land like this without turning the tree orange.  I can do a try push.  Though this extra mach check seems weird.

I'm running it with
  ./mach python-test testing/xpcshell/selftest.py

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Looks like mach's python-test is doing some extra log parsing (http://hg.mozilla.org/mozilla-central/file/4e8f29e386bd/python/mach_commands.py#l95) that causes the failure output. 'print "TEST-"' in selftest.py placates that. Output from the child process otherwise seems to work ok.

Comment 7

[Andrew McCreight [:mccr8]]

Thanks!  That ends up printing something extra to the console, but I think that isn't a big deal, given how few people probably even run this test locally.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Wait for child to finish in test_child_assert XPCShell unit test.

the unit test part of this try run looks fine:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82f7ce409c07

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8526961 [details] [diff] [review]
Wait for child to finish in test_child_assert XPCShell unit test.

Review of attachment 8526961 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/selftest.py
@@ +483,5 @@
>          self.assertEquals(0, self.x.todoCount)
>          self.assertInLog(TEST_PASS_STRING)
>          self.assertNotInLog(TEST_FAIL_STRING)
>  
> +        print TEST_PASS_STRING + " This is a fake pass message to stop a bogus mach failure"

I didn't mean to imply this should actually be added, although I'm sure it doesn't hurt anything. My conclusion was that these tests shouldn't be run with the mach python-test target (mach python or make check work fine).

Comment 10

[Andrew McCreight [:mccr8]]

Oh, oops.  I'll play around with that.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Chris Manchester [:chmanchester] from comment #9)
> I didn't mean to imply this should actually be added, although I'm sure it
> doesn't hurt anything. My conclusion was that these tests shouldn't be run
> with the mach python-test target (mach python or make check work fine).

Oh, I think I re-misunderstood you.  Basically, you are saying that I should land the patch I have here, except without the print statement, and then just live with the fact that running this test via mach python-test will produce a failure?  I can do that, and then just add a comment to the effect that this test will fail when run that way.  And I guess file a bug about it against mach.

Comment 12

[Andrew McCreight [:mccr8]]

I filed bug 1103226 on the mach issue.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Chris Manchester [:chmanchester] from comment #9)
> > I didn't mean to imply this should actually be added, although I'm sure it
> > doesn't hurt anything. My conclusion was that these tests shouldn't be run
> > with the mach python-test target (mach python or make check work fine).
> 
> Oh, I think I re-misunderstood you.  Basically, you are saying that I should
> land the patch I have here, except without the print statement, and then
> just live with the fact that running this test via mach python-test will
> produce a failure?  I can do that, and then just add a comment to the effect
> that this test will fail when run that way.  And I guess file a bug about it
> against mach.

Exactly. Sorry for the confusion. I've never run across the python-test target before, but it looks like it expects output from a test runner that selftest.py doesn't use. I see the error with or without your patch when running from python-test, it seems unrelated to this bug.

Comment 14

[Andrew McCreight [:mccr8]]

Thanks for your help.

I removed the print and added a reference to the bug I filed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f86a92a28ec9

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f86a92a28ec9