Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

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

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
@@ -3,6 +3,7 @@ require('../common');
const assert = require('assert');
const fork = require('child_process').fork;
const net = require('net');
const common = require('../common');
Copy link
Contributor

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
@@ -60,9 +61,11 @@ if (process.argv[2] === 'child') {

const child = fork(process.argv[1], ['child']);

child.on('exit', function() {
child.on('exit', common.mustCall(function(code, signal) {
console.log('CHILD: died');
Copy link
Contributor

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

@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

@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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 @italoacasas
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

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