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

cjihrig · 2016-10-20T15:42:02Z

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

santigimeno · 2016-10-20T15:51:38Z

Maybe testing with a different killSignal than the default too?

LGTM anyway if CI is green.

jasnell · 2016-10-20T17:10:00Z

test/parallel/test-child-process-exec-timeout.js

+
+if (process.argv[2] === 'child') {
+  setTimeout(() => {
+    console.log('child stdout');


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

jasnell

LGTM with a nit

thefourtheye · 2016-10-21T06:52:47Z

test/parallel/test-child-process-exec-timeout.js

+const cmd = `${process.execPath} ${__filename} child`;
+
+// Test the case where a timeout is set, and it expires.
+cp.exec(cmd, { timeout: 1 }, (err, stdout, stderr) => {


Mind adding common.mustCall?

thefourtheye · 2016-10-21T06:53:46Z

test/parallel/test-child-process-exec-timeout.js

+  assert.strictEqual(stderr.trim(), '');
+});
+
+// Test the case where a timeout is set, but not eclipsed.


Nit: Did you mean elapsed?

thefourtheye · 2016-10-21T07:06:48Z

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 · 2016-10-21T07:26:16Z

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 · 2016-10-21T07:28:38Z

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

targos · 2016-10-21T07:50:11Z

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

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 · 2016-10-24T20:34:20Z

#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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

cjihrig · 2016-10-25T16:56:51Z

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

thefourtheye · 2016-10-25T17:03:43Z

Belated LGTM.

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

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 · 2016-11-18T21:40:29Z

@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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

cjihrig · 2016-11-18T21:55:33Z

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

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 · 2016-11-19T00:12:05Z

is it worth backporting without that assertation?

cjihrig · 2016-11-19T03:10:43Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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

Uh oh!

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

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

Uh oh!

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!

Uh oh!