GeezerVison

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

GeezerVison is showing github.com — test: complete coverage of lib/child_process.js by cjihrig · Pull Request #12367 · nodejs/node

Conversation

cjihrig commented Apr 12, 2017

Copy link

Copy Markdown

Contributor

This commit adds a test which brings coverage of lib/child_process.js to 100%. It adds coverage for the call to uv.errname() in execFile()'s exithandler() function.

I haven't been able to hit this line of code completely naturally, but as shown in the test, it can be done with public APIs. So, there is a question of whether that line of code needs to exist. To execute that line:

exithandler() must be called for the first time.
A callback must be provided to execFile().
ex must not be set and code must be less than zero.

Out of the three call sites of exithandler(), the child process 'close' handler is the only one that does not set ex. The 'close' event is emitted once all of child processes streams have been closed. I find it unlikely that all of the streams will be closed before an error is emitted. That would make the code in question dead code. However, I'd like to avoid possibly breaking anyone, and it's technically possible to recreate the case, as shown in the test.

cc: @Trott

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

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

Apr 12, 2017

Trott approved these changes

Apr 12, 2017

View reviewed changes

Trott left a comment

Copy link

Copy Markdown

Member

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 is CI is green.

Would also be OK with removing the check in a subsequent semver-major but don't much care either way. Seems the risk is small and so is the benefit, so /shrug

Sorry, something went wrong.

Uh oh!

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

All reactions

mscdex added the child_process Issues and PRs related to the child_process subsystem. label

Apr 12, 2017

lucamaraschi suggested changes

Apr 12, 2017

View reviewed changes

lucamaraschi left a comment •

edited

Loading

Uh oh!

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

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 u please move the require('../common') to line 2 as indicated in the writing testing guide?

Sorry, something went wrong.

Uh oh!

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

All reactions

mhdawson commented Apr 13, 2017

Copy link

Copy Markdown

Member

I'm thinking that unlikely does not make it dead code, so better safe than sorry.

All reactions

Sorry, something went wrong.

Uh oh!

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

mhdawson approved these changes

Apr 13, 2017

View reviewed changes

mhdawson left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

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

All reactions

jasnell approved these changes

Apr 13, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare

April 14, 2017 04:12

hiroppy approved these changes

Apr 14, 2017

View reviewed changes

addaleax commented Apr 14, 2017

Copy link

Copy Markdown

Member

CI: https://ci.nodejs.org/job/node-test-commit/9116/

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 14, 2017

Copy link

Copy Markdown

Member

Test fails on Windows. Maybe some characters in the path are being treated as special characters in the regular expression created through the constructor. Looking at it, I think the regular expression isn't needed. Just create a string and do an assert.strictEqual(). As a bonus, this will improve the error message when the assertion is triggered.

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott reviewed

Apr 14, 2017

View reviewed changes

Comment thread

test/parallel/test-child-process-execfile.js Outdated

Trott Apr 14, 2017

Copy link

Copy Markdown

Member

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.

This could just be a string rather than a regexp...

Sorry, something went wrong.

Uh oh!

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

All reactions

Trott reviewed

Apr 14, 2017

View reviewed changes

Comment thread

test/parallel/test-child-process-execfile.js Outdated

Trott Apr 14, 2017

Copy link

Copy Markdown

Member

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.

...and then this could be assert.strictEqual().

Sorry, something went wrong.

Uh oh!

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

All reactions

thefourtheye approved these changes

Apr 15, 2017

View reviewed changes

Trott requested changes

Apr 16, 2017

View reviewed changes

Trott left a comment

Copy link

Copy Markdown

Member

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.

Marking as "request changes" until the Windows situation is fixed, just to make sure no one lands it without noting the problem.

Sorry, something went wrong.

Uh oh!

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

All reactions

cjihrig force-pushed the coverage branch from fa7d278 to d46d1bc Compare

April 18, 2017 18:17

cjihrig commented Apr 18, 2017

Copy link

Copy Markdown

Contributor Author

New CI with the regular expression changed to a string. Passes locally on Windows now.

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

All reactions

Sorry, something went wrong.

Uh oh!

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


          test: complete coverage of lib/child_process.js

d2a3651

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: nodejs#12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

cjihrig force-pushed the coverage branch from d46d1bc to d2a3651 Compare

April 18, 2017 19:15

cjihrig merged commit d2a3651 into nodejs:master

Apr 18, 2017

cjihrig deleted the coverage branch

April 18, 2017 19:17

evanlucas pushed a commit that referenced this pull request

Apr 25, 2017


          test: complete coverage of lib/child_process.js

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: #12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

evanlucas mentioned this pull request

May 1, 2017

v7.10.0 proposal #12775

Merged

evanlucas pushed a commit that referenced this pull request

May 1, 2017


          test: complete coverage of lib/child_process.js

4adb82d

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: #12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

evanlucas pushed a commit that referenced this pull request

May 2, 2017


          test: complete coverage of lib/child_process.js

8e3d54a

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: #12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

gibfahn added the land-on-v6.x label

May 16, 2017

gibfahn pushed a commit that referenced this pull request

May 16, 2017


          test: complete coverage of lib/child_process.js

2bf9bd9

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: #12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

MylesBorins pushed a commit that referenced this pull request

May 18, 2017


          test: complete coverage of lib/child_process.js

a4b092c

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: #12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

MylesBorins mentioned this pull request

May 23, 2017

v6.11.0 proposal #13059

Merged

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

Jul 19, 2017


          test: complete coverage of lib/child_process.js

749b1c1

This commit adds a test which brings coverage of
lib/child_process.js to 100%. It adds coverage for the call to
uv.errname() in execFile()'s exithandler() function.

PR-URL: nodejs/node#12367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>

coderabbitai Bot mentioned this pull request

Sep 18, 2026

feat(harness): qualify deterministic MCP launch commands sapiom/sapiom-js#1065

Merged

Reviewers

Trott

Trott requested changes

jasnell

jasnell approved these changes

+4 more reviewers

lucamaraschi

lucamaraschi requested changes

thefourtheye

thefourtheye approved these changes

hiroppy

hiroppy approved these changes

mhdawson

mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

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.

11 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 01402520 — 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

cjihrig commented Apr 12, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

lucamaraschi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 13, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 14, 2017

Uh oh!

Trott commented Apr 14, 2017

Uh oh!

Trott Apr 14, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Apr 14, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Apr 18, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

lucamaraschi left a comment •

edited

Loading

Uh oh!

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