test: add child_process.exec() timeout coverage by cjihrig · Pull Request #9208 · nodejs/node · GitHub

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_process exec() and execFile().

Maybe testing with a different killSignal than the default too?

LGTM anyway if CI is green.

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

LGTM with a nit

Mind adding common.mustCall?

Nit: Did you mean elapsed?

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)

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.

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

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

This allows us to use the exponentiation operator. PR-URL: nodejs#9218 Ref: nodejs#9208 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

#9218 landed

This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: nodejs#9208 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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

Belated LGTM.

This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

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

This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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.

This allows us to use the exponentiation operator. PR-URL: #9218 Ref: #9208 (comment) Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

is it worth backporting without that assertation?

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

This commit adds coverage for the timeout option used by child_process exec() and execFile(). PR-URL: #9208 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 20, 2016

santigimeno added the child_process Issues and PRs related to the child_process subsystem. label Oct 20, 2016

jasnell reviewed Oct 20, 2016

View reviewed changes

jasnell approved these changes Oct 20, 2016

View reviewed changes

targos approved these changes Oct 20, 2016

View reviewed changes

thefourtheye reviewed Oct 21, 2016

View reviewed changes

targos mentioned this pull request Oct 21, 2016

tools: enable ES2016 support in ESLint #9218

Merged

2 tasks

cjihrig force-pushed the timeout branch from 5543ae3 to 1cf55f8 Compare October 25, 2016 16:27

cjihrig merged commit 1cf55f8 into nodejs:master Oct 25, 2016

cjihrig deleted the timeout branch October 25, 2016 16:57

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added land-on-v6.x labels Nov 18, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v6.9.2 proposal #9735

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016

gibfahn mentioned this pull request Jan 28, 2017

parallel/test-child-process-exec-timeout not cleaning up child process on win7 #11052

Closed

test: add child_process.exec() timeout coverage by cjihrig · Pull Request #9208 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: add child_process.exec() timeout coverage#9208cjihrig merged 1 commit intonodejs:masternodejs/node:masterfrom cjihrig:timeoutcjihrig/node:timeoutCopy head branch name to clipboard

Conversation

cjihrig commented Oct 20, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

santigimeno commented Oct 20, 2016

Uh oh!

jasnell Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

thefourtheye Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Oct 21, 2016

Uh oh!

thefourtheye commented Oct 21, 2016

Uh oh!

thefourtheye commented Oct 21, 2016

Uh oh!

targos commented Oct 21, 2016

Uh oh!

targos commented Oct 24, 2016

Uh oh!

cjihrig commented Oct 25, 2016

Uh oh!

thefourtheye commented Oct 25, 2016

Uh oh!

MylesBorins commented Nov 18, 2016

Uh oh!

cjihrig commented Nov 18, 2016

Uh oh!

MylesBorins commented Nov 19, 2016

Uh oh!

cjihrig commented Nov 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test: add child_process.exec() timeout coverage#9208
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:timeout