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: complete coverage of lib/child_process.js #12367

Merged
merged 1 commit into from
Apr 18, 2017
Merged

test: complete coverage of lib/child_process.js #12367

merged 1 commit into from
Apr 18, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 12, 2017

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:

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 12, 2017
Trott
Trott approved these changes Apr 12, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a 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

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Apr 12, 2017
lucamaraschi
lucamaraschi suggested changes Apr 12, 2017
View reviewed changes
Copy link
Contributor

@lucamaraschi lucamaraschi left a comment •
edited
Loading

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?

@mhdawson
Copy link
Member

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

mhdawson
mhdawson approved these changes Apr 13, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Apr 14, 2017

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.

Trott
Trott reviewed Apr 14, 2017
View reviewed changes
test/parallel/test-child-process-execfile.js Outdated

{
// Verify that negative exit codes can be translated to UV error names.
const errRegExp = new RegExp(`^Error: Command failed: ${process.execPath}$`);
Copy link
Member

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

Trott
Trott reviewed Apr 14, 2017
View reviewed changes
test/parallel/test-child-process-execfile.js Outdated
const errRegExp = new RegExp(`^Error: Command failed: ${process.execPath}$`);
const code = -1;
const callback = common.mustCall((err, stdout, stderr) => {
assert(errRegExp.test(err.toString().trim()));
Copy link
Member

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

Trott
Trott requested changes Apr 16, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a 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.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 18, 2017

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/

@cjihrig
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@cjihrig cjihrig merged commit d2a3651 into nodejs:master Apr 18, 2017
@cjihrig cjihrig deleted the coverage branch April 18, 2017 19:17
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@cjihrig @evanlucas
test: complete coverage of lib/child_process.js
8070353 
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@cjihrig @evanlucas
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@cjihrig @evanlucas
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
gibfahn pushed a commit that referenced this pull request May 16, 2017
@cjihrig @gibfahn
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@cjihrig @MylesBorins
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@cjihrig @andrew749
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 <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott requested changes

@lucamaraschi lucamaraschi lucamaraschi requested changes

@mhdawson mhdawson mhdawson approved these changes

@hiroppy hiroppy hiroppy approved these changes

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