GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test: fail when child died in fork-net by joyeecheung · Pull Request #11684 · nodejs/node

Conversation

joyeecheung commented Mar 4, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label

Mar 4, 2017

joyeecheung commented Mar 4, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 added the child_process Issues and PRs related to the child_process subsystem. label

Mar 4, 2017

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 commented Mar 4, 2017

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig approved these changes

Mar 13, 2017

View reviewed changes

cjihrig left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-child-process-fork-net.js Outdated

cjihrig Mar 13, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-child-process-fork-net.js Outdated

cjihrig Mar 13, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AndreasMadsen commented Mar 13, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Mar 13, 2017

View reviewed changes


          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 force-pushed the assert-in-fork-net branch from caac394 to 8eb9e51 Compare

March 14, 2017 16:27

joyeecheung commented Mar 14, 2017

Copy link

Copy Markdown

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell pushed a commit that referenced this pull request

Mar 14, 2017


          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 commented Mar 14, 2017

Copy link

Copy Markdown

Member

Landed in c9bc3e5

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell closed this

Mar 14, 2017

italoacasas pushed a commit to italoacasas/node that referenced this pull request

Mar 20, 2017


          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 mentioned this pull request

Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

jungx098 pushed a commit to jungx098/node that referenced this pull request

Mar 21, 2017


          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 added the land-on-v6.x label

Apr 18, 2017

MylesBorins pushed a commit that referenced this pull request

Apr 18, 2017


          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


          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 mentioned this pull request

Apr 19, 2017

v6.10.3 proposal #12498

Merged

andrew749 pushed a commit to michielbaird/node that referenced this pull request

Jul 19, 2017


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

Reviewers

jasnell

jasnell approved these changes

cjihrig

cjihrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

child_process

Issues and PRs related to the child_process subsystem.

test

Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

joyeecheung commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Mar 4, 2017

Uh oh!

joyeecheung commented Mar 4, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 13, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 13, 2017

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Mar 13, 2017

Uh oh!

joyeecheung commented Mar 14, 2017

Uh oh!

jasnell commented Mar 14, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joyeecheung commented Mar 4, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.