test: add child_process.exec() timeout coverage

[cjihrig]

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]

Maybe testing with a different killSignal than the default too?

LGTM anyway if CI is green.

[jasnell]

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

[jasnell]

LGTM with a nit

[thefourtheye]

Mind adding common.mustCall?

[thefourtheye]

Nit: Did you mean elapsed?

[thefourtheye]

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]

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]

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]

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

[targos]

#9218 landed

[cjihrig]

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

[thefourtheye]

Belated LGTM.

[MylesBorins]

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

[cjihrig]

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]

is it worth backporting without that assertation?

[cjihrig]

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