Skip to content

test: fail when child died in fork-net#11684

Closed
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:assert-in-fork-net
Closed

test: fail when child died in fork-net#11684
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:assert-in-fork-net

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 4, 2017 •
edited
Loading

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, child_process, net

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 4, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 4, 2017

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

@joyeecheung joyeecheung changed the title test: fail when child died in fork-net [WIP] test: fail when child died in fork-net Mar 4, 2017
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Mar 4, 2017
@joyeecheung joyeecheung changed the title [WIP] test: fail when child died in fork-net test: fail when child died in fork-net Mar 4, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 4, 2017

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..

cjihrig
cjihrig approved these changes Mar 13, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This will need another CI run though.

test/parallel/test-child-process-fork-net.js Outdated
Copy link
Contributor

@cjihrig cjihrig Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common is already required a few lines up. It just needs to be assigned to a variable.

test/parallel/test-child-process-fork-net.js Outdated
Copy link
Contributor

@cjihrig cjihrig Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this console.log() since it's now included in the assertion message.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Mar 13, 2017

@joyeecheung blame will also tell you that this was 5 years ago, so I don't really remember, sorry :/

@joyeecheung
test: fail when child dies in fork-net
8eb9e51 
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.
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 14, 2017

@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/

jasnell pushed a commit that referenced this pull request Mar 14, 2017
@joyeecheung @jasnell
test: fail when child dies in fork-net
c9bc3e5 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 14, 2017

Landed in c9bc3e5

@jasnell jasnell closed this Mar 14, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
@joyeecheung
test: fail when child dies in fork-net
5f6025b 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 20, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@joyeecheung @jungx098
test: fail when child dies in fork-net
1ba472d 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@joyeecheung @MylesBorins
test: fail when child dies in fork-net
0d4d67a 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@joyeecheung @MylesBorins
test: fail when child dies in fork-net
4d916da 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@joyeecheung @andrew749
test: fail when child dies in fork-net
d070f81 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@joyeecheung @AndreasMadsen @jasnell @cjihrig @mscdex @MylesBorins @nodejs-github-bot