test: fail when child died in fork-net - #11684
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
Deleted child.kill(), the new CI is all green now: https://ci.nodejs.org/job/node-test-commit/8265/ @AndreasMadsen (pinging according to git blame) @nodejs/testing Is the child.kill() here necessary? From what I understand, the child should not be hanging around when the test completes. If the test has to call child.kill() manually, that probably means there is a bug in the child_process module.. |
Sorry, something went wrong.
cjihrig
left a comment
There was a problem hiding this comment.
LGTM. This will need another CI run though.
Sorry, something went wrong.
There was a problem hiding this comment.
common is already required a few lines up. It just needs to be assigned to a variable.
Sorry, something went wrong.
There was a problem hiding this comment.
Can you remove this console.log() since it's now included in the assertion message.
Sorry, something went wrong.
|
@joyeecheung blame will also tell you that this was 5 years ago, so I don't really remember, sorry :/ |
Sorry, something went wrong.
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call.
|
@AndreasMadsen I asked just in case...anyway thanks for the reply :) Addressed @cjihrig 's comments. CI: https://ci.nodejs.org/job/node-test-pull-request/6838/ |
Sorry, something went wrong.
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs#11684 Ref: nodejs#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs#11684 Ref: nodejs#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: #11684 Ref: #11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This makes it fail so at least there is more information about why this test fails. Also removes the unnecessary child.kill() call. PR-URL: nodejs/node#11684 Ref: nodejs/node#11667 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Previously when the child dies with errors in this test, the parent will just hang and timeout, the errors in the child would be swallowed. This PR makes it fail so at least there is more information about why this test fails.
On a side note, looks like this test is the only test that is touching the options instanceof TCP section in net.Server.prototyp.listen, and it is touching an undocumented API (calling net.Server.prototyp.listen on the TCP handle directly instead of putting it in an object's handle or _handle) indirectly through child_process. This probably means the documented API for handles (like {handle: new TCP()}) doesn't get any direct test coverage.
Refs: #11667
Checklist
Affected core subsystem(s)
test, child_process, net