test: add test for broken child process stdio#9528
Conversation
There was a problem hiding this comment.
I'm seeing a jslint error on this line:
18:40 error Unexpected space before function parentheses space-before-function-paren
|
@danbev sorry. Fixed. |
mscdex
added
the
child_process
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
LGTM although I have a question
There was a problem hiding this comment.
The test works reliably for me (and runs faster) with the setTimeout() replaced with a setImmediate(). Are we sure that the timer is needed here? If so, any chance we can bring it down to 100ms instead of 1000ms?
There was a problem hiding this comment.
A setImmediate() is likely a safe. I just wanted to make sure that the spawned process didn't complete before the 1ms timeout in the third scenario.
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM, I like that the code coverage data is already being used in order to improve coverage :)
|
CI to validate across platforms: https://ci.nodejs.org/job/node-test-pull-request/4819/ |
|
CI again without the |
|
The latest CI run failed on AIX due to the child process exiting too quickly, and hence not being killed. Adding the timeout back, and setting the delay to 100, as requested in #9528 (comment). |
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: nodejs#9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: #9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: nodejs#9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: #9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: #9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created. PR-URL: #9528 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
9 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
This commit adds a test for the scenario where a child process is spawned, but the stdio streams could not be created.
If you look at child process coverage, this adds coverage for the
elseconditions on lines 224, 227, 234, 237, 256, and 275.