test: fix flaky child-process-exec-kill-throws

[Trott]

This is a fix for test-child-process-exec-kill-throws which is currently
flaky on Windows.

A bug in the test was causing the child process to fail for reasons
other than those intended by the test. Instead of failing for exceeding
the maxBuffer setting, the test was failing because it was trying to
load internal/child_process without being passed the
expose-internals flag. Move that module to where only the parent
process (which gets the flag) loads it.

Additionally, improve an assertion message to help debug problems like
this.

Fixes: #12053

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

test child_process

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/7082/

[Trott]

Stress test on master which should show failures: https://ci.nodejs.org/job/node-stress-single-test/1147/nodes=win-vs2015/console

[Trott]

Stress test on this PR (should show no failures): https://ci.nodejs.org/job/node-stress-single-test/1148/nodes=win-vs2015/console

[Trott]

If the CI and stress tests come up as expected, I would propose that we land this sooner than the 48 hours we normally wait. (I won't argue, though, if someone feels it should wait.)

[Trott]

@nodejs/testing

[Trott]

Hmmm, CI is not looking good. Looks like it might be failing because something (maybe this test somehow) is leaving around leftover child processes? Will have to look more closely...

[Trott]

Confirmed that now that the initial bug in the test is fixed, it now does not reliably terminate the child process. :-|

[Trott]

(On the upside, the stress tests are looking good.)

[Trott]

Adding a listener for the exit event on the child process. Hoping that fixes everything by preventing the parent process from exiting before it has actually sent the signal. (Perhaps it bails early due to the monkey-patched error?) I might be reaching here, but let's try CI and see:

CI: https://ci.nodejs.org/job/node-test-pull-request/7083/

[Trott]

sigh Nope, that didn't fix the "child process not actually terminating" issue...

[santigimeno]

I think the problem here is that killing the child process will only kill the shell process that executes the nodejs child process leaving the child process orphan and alive. TBH I don't know how kill could work in this scenario. Any ideas?

[bzoz]

If @santigimeno is right, then try using spawn instead of exec. The spawn function does not create a shell by default.

[gibfahn]

@bzoz Given that the test is called child-process-exec-kill-throws, I'd assume changing to spawn() would defeat the purpose of the test. See the original PR: #11038.

cc/ @cjihrig

[gibfahn]

@Trott this seems like it could be related to #11052, also a child-process-exec test that has issues on windows.

[Trott]

@Trott this seems like it could be related to #11052, also a child-process-exec test that has issues on windows.

Possibly, but this isn't Windows-specific. The bug in the test that this fixes was Windows specific. The new issue it exposes now that bug is fixed.. that's on all sorts of platforms. Check the CI results.

[Trott]

(In fact, the current not-cleaning-up-after-itself issue doesn't affect the Windows runs on the last CI, so it may not affect Windows at all!)

[Trott]

Unfortunately, I don't think changing to spawn() will work because this test is designed to add coverage for two lines that get run with exec() but now spawn().

I don't think we need a process that runs forever. We just need a process that errors out. So I'm removing the setInterval() and I think this should do it.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/7127/

[Trott]

This looks much better! And it still exercises the relevant code.

Some Linux hosts in CI are struggling right now, but that's not related to this. I might run another CI again once that is cleared up, just to be cautious.

[Trott]

(Also: I'm super-pleased that the Makefile changes from a few weeks ago caught a test that sometimes leaves stray processes around. Hooray!)

[Trott]

Oh, right, need to do a Windows stress test too:

https://ci.nodejs.org/job/node-stress-single-test/1149/nodes=win-vs2015/console

[Trott]

CI is clean except for one unrelated build failure.

Stress test is still running, but looks great so far (1200 successes and 0 failures so far).

[Trott]

Landed in a10e657