Skip to content

test: add child_process.exec() timeout coverage#9208

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
Merged
cjihrig merged 1 commit into from
Oct 25, 2016
Merged

test: add child_process.exec() timeout coverage #9208

cjihrig merged 1 commit into from
Oct 25, 2016

Conversation

@cjihrig
Copy link
Contributor

@cjihrigcjihrig commented Oct 20, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

This commit adds coverage for the timeout option used by child_processexec() and execFile().

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Oct 20, 2016
@santigimeno
Copy link
Member

santigimeno commented Oct 20, 2016

Maybe testing with a different killSignal than the default too?

LGTM anyway if CI is green.

@santigimenosantigimeno added the child_process Issues and PRs related to the child_process subsystem. label Oct 20, 2016
jasnell
jasnell reviewed Oct 20, 2016
View reviewed changes
test/parallel/test-child-process-exec-timeout.js Outdated
Copy link
Member

@jasnelljasnellOct 20, 2016

Choose a reason for hiding this comment

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

Would you mind adding a comment mentioning that the console.log() calls are intentionally part of the test.

jasnell
jasnell approved these changes Oct 20, 2016
View reviewed changes
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

thefourtheye
thefourtheye reviewed Oct 21, 2016
View reviewed changes
test/parallel/test-child-process-exec-timeout.js Outdated
Copy link
Contributor

@thefourtheyethefourtheyeOct 21, 2016

Choose a reason for hiding this comment

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

Mind adding common.mustCall?

test/parallel/test-child-process-exec-timeout.js Outdated
Copy link
Contributor

@thefourtheyethefourtheyeOct 21, 2016

Choose a reason for hiding this comment

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

Nit: Did you mean elapsed?

@thefourtheye
Copy link
Contributor

thefourtheye commented Oct 21, 2016

Not sure why, but when I ran the test locally without the common require, I get this error

cp.exec(cmd,{timeout: 2 ** 30 }, (err, stdout, stderr) =>{^ SyntaxError: Unexpected token *

Also, this trips the linter,

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \ benchmark lib test tools node/test/parallel/test-child-process-exec-timeout.js 27:28 error Parsing error: Unexpected token * ✖ 1 problem (1 error, 0 warnings)

@thefourtheye
Copy link
Contributor

thefourtheye commented Oct 21, 2016

Oh okay. Exponentiation Operator is only in ES7, so you might have to use Math.pow atleast for the sake of the linter. But I wonder how V8 allows this already.

@thefourtheye
Copy link
Contributor

thefourtheye commented Oct 21, 2016

Hmmm, apparently V8 allowed this harmony feature to be turned on by default. Reference: https://bugs.chromium.org/p/v8/issues/detail?id=3915#c18

@targostargos mentioned this pull request Oct 21, 2016
Merged
2 tasks
@targos
Copy link
Member

targos commented Oct 21, 2016

PR to allow the exponentiation operator in the linter: #9218

targos added a commit to targos/node that referenced this pull request Oct 24, 2016
@targos
tools: enable ES2016 syntax support in ESLint
6031d8e
This allows us to use the exponentiation operator. PR-URL: nodejs#9218 Ref: nodejs#9208 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 24, 2016
@targos@jasnell
tools: enable ES2016 syntax support in ESLint
827660e
This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@targos
Copy link
Member

targos commented Oct 24, 2016

#9218 landed

@cjihrig
test: add child_process.exec() timeout coverage
1cf55f8
This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: nodejs#9208 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@cjihrig
Copy link
ContributorAuthor

cjihrig commented Oct 25, 2016

Green CI: https://ci.nodejs.org/job/node-test-pull-request/4666/. Landing.

@cjihrigcjihrig merged commit 1cf55f8 into nodejs:masterOct 25, 2016
@cjihrigcjihrig deleted the timeout branch October 25, 2016 16:57
@thefourtheye
Copy link
Contributor

thefourtheye commented Oct 25, 2016

Belated LGTM.

evanlucas pushed a commit that referenced this pull request Nov 2, 2016
@cjihrig@evanlucas
test: add child_process.exec() timeout coverage
deef2f6
This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@evanlucasevanlucas mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@targos
tools: enable ES2016 syntax support in ESLint
5f0a0fa
This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 18, 2016

@cjihrig this landed cleanly on v6.x with a small modification. v4.x is failing this test though

output:

=== release test-child-process-exec-timeout === Path: parallel/test-child-process-exec-timeout assert.js:85 throw new assert.AssertionError({^ AssertionError: '/bin/sh -c /Users/thealphanerd/code/node/v4.x/out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-child-proc === '/Users/thealphanerd/code/node/v4.x/out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-child-process-exec-ti at /Users/thealphanerd/code/node/v4.x/test/parallel/test-child-process-exec-timeout.js:22:10 at /Users/thealphanerd/code/node/v4.x/test/common.js:402:15 at ChildProcess.exithandler (child_process.js:220:5) at emitTwo (events.js:87:13) at ChildProcess.emit (events.js:172:7) at maybeClose (internal/child_process.js:854:16) at Process.ChildProcess._handle.onexit (internal/child_process.js:222:5) Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-child-process-exec-timeout.js

any idea what is up?

MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
@cjihrig
test: add child_process.exec() timeout coverage
9913b31
This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 18, 2016

I believe it's the difference between this on v6:
https://github.com/nodejs/node/blob/v6.x/lib/child_process.js#L94

And this on v4:
https://github.com/nodejs/node/blob/v4.x/lib/child_process.js#L100-L101

That assertion can probably be dropped. It's not super important.

MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
@targos@MylesBorins
tools: enable ES2016 syntax support in ESLint
a120199
This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
@cjihrig@MylesBorins
test: add child_process.exec() timeout coverage
3368a98
This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 19, 2016

is it worth backporting without that assertation?

@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 19, 2016

If you deem the test worthy of backporting, then err.cmd assertions aren't important.

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
@cjihrig
test: add child_process.exec() timeout coverage
b83b517
This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 22, 2016
Merged
@gibfahngibfahn mentioned this pull request Jan 28, 2017
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@targostargostargos approved these changes

+1 more reviewer

@thefourtheyethefourtheyethefourtheye left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_processIssues and PRs related to the child_process subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cjihrig@santigimeno@thefourtheye@targos@MylesBorins@jasnell@nodejs-github-bot