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

RESOLVED FIXED in mozilla36

Status

Testing
XPCShell Harness
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → continuation
(Assignee)

Comment 2

4 years ago
Created attachment 8525524 [details] [diff] [review]
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?
(Assignee)

Comment 3

4 years ago
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.
Flags: needinfo?(cmanchester)
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| ?
(Assignee)

Comment 5

4 years ago
(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
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.
Flags: needinfo?(cmanchester)
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
Created attachment 8526961 [details] [diff] [review]
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
Attachment #8526961 - Flags: review?(cmanchester)
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).
Attachment #8526961 - Flags: review?(cmanchester) → review+
(Assignee)

Comment 10

4 years ago
Oh, oops.  I'll play around with that.
(Assignee)

Comment 11

4 years ago
(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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(cmanchester)
(Assignee)

Comment 12

4 years ago
I filed bug 1103226 on the mach issue.
(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.
Flags: needinfo?(cmanchester)
(Assignee)

Comment 14

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/f86a92a28ec9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.