child_process: spawn ignores options in case args is undefined

[eduardbme]

spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: #24912

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[mscdex]

Can you add a test for this?

[Trott]

Any reason not to be generous and permit null here? I believe our linter permits == null which evaluates to true for both null and undefined. In fact, there are two other places in this file already that use that.

[eduardbme]

Adjusted that case.

[Trott]

Here's what the test file could look like.

'use strict';
const common = require('../common');

const assert = require('assert');
const { spawn } = require('child_process');

const tmpdir = require('../common/tmpdir');

const command = common.isWindows ? 'cd' : 'pwd';

const subprocess = spawn(command, undefined, { cwd: tmpdir.path });

subprocess.stdout.on('data', common.mustCall((data) => {
  const actual = data.toString().trim().toLowerCase();
  const expected = tmpdir.path.trim().toLowerCase();
  assert.strictEqual(actual, expected);
}));

[eduardbme]

Updated PR.

[bnoordhuis]

New tests should not include the copyright boilerplate.

[eduardbme]

+

[bnoordhuis]

Use subprocess.stdout.setEncoding('utf8') so you get strings instead of buffers and accumulate the strings.

toString-ing a buffer is unsound because the output may be split over multiple buffers (i.e., you can get multiple 'data' events and multi-byte sequences may be split across buffers.)

[eduardbme]

Good catch, overlooked that

[eduardbme]

PR has been updated according to comments.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19358/

[Trott]

LGTM as-is or with the comment I suggested adding.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19396/

[sam-github]

See comment.

[sam-github]

There is already a test for this behaviour, could you expand it instead of adding another test file? See test-child-process-spawn-typeerror.js (and yes, it's name doesn't well reflect is purpose anymore). I assume this change effects, or SHOULD effect, every child_process API that accepts an argv argument, showing that they have identical handling of the argv option, which is what the test I mentioned is set up for.

[Trott]

@sam-github Nit or requirement to land?

[sam-github]

I'd rather it lands with these changes. Particularly with API consistency.

[eduardbme]

That's a little bit controversial.

This file contains lots of unrelated checks, like https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-spawn-typeerror.js#L115. Why do we test execFile function within spawn test file, shouldn't it be placed in test-child-process-exec-file.js or something like that ?

And we have already separate test file test-child-process-spawn-argv0.js for testing argv0 param, so what the problem with args ?

In my opinion, *-spawn-typeerror.js should contain tests just about incorrect types, and just for spawn method.

[eduardbme]

In addition, that fix effects only spawn and spawnSync methods.

[Trott]

I also favor smaller, more-focused test files, especially for child_process tests in parallel.

[sam-github]

I consider that tests that are intended to ensure multiple APIs have behave identically, when they should, are best contained within a single file.

At this time, I can't tell from the tests whether all the child_process APIs do, in fact, have identical arg handling. Also, I can't tell if this PR makes them more (or less) consistent. Its possible that it fixes an inconsistency. Its possible it makes them inconsistent. I don't know. Ideally, the test structure would make that very clear. Every PR doesn't have to be ideal. I'm removing my review.

[eduardbme]

Added same test for spawnSync method.

Not reviewing.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19467/

[Trott]

Looks like there are relevant test failures on Windows:

15:40:38 not ok 64 parallel/test-child-process-spawn-args
15:40:38   ---
15:40:38   duration_ms: 0.251
15:40:38   severity: fail
15:40:38   exitcode: 1
15:40:38   stack: |-
15:40:38     events.js:174
15:40:38           throw er; // Unhandled 'error' event
15:40:38           ^
15:40:38     
15:40:38     Error: spawn cd ENOENT
15:40:38         at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
15:40:38         at onErrorNT (internal/child_process.js:422:16)
15:40:38         at internalTickCallback (internal/process/next_tick.js:72:19)
15:40:38         at process._tickCallback (internal/process/next_tick.js:47:5)
15:40:38         at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
15:40:38         at executeUserCode (internal/bootstrap/node.js:317:15)
15:40:38         at startExecution (internal/bootstrap/node.js:251:3)
15:40:38         at startup (internal/bootstrap/node.js:202:3)
15:40:38         at internal/bootstrap/node.js:682:1
15:40:38     Emitted 'error' event at:
15:40:38         at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
15:40:38         at onErrorNT (internal/child_process.js:422:16)
15:40:38         [... lines matching original stack trace ...]
15:40:38         at internal/bootstrap/node.js:682:1
15:40:38   ...

15:45:14 ok 65 parallel/test-child-process-spawn-argv0
15:45:14   ---
15:45:14   duration_ms: 0.596
15:45:14   ...
15:45:15 not ok 66 parallel/test-child-process-spawnsync-args
15:45:15   ---
15:45:15   duration_ms: 0.201
15:45:15   severity: fail
15:45:15   exitcode: 1
15:45:15   stack: |-
15:45:15     assert.js:86
15:45:15       throw new AssertionError(obj);
15:45:15       ^
15:45:15     
15:45:15     AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
15:45:15     + actual - expected
15:45:15     
15:45:15     + null
15:45:15     - Buffer [Uint8Array] []
15:45:15         at testCases.map (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:33:10)
15:45:15         at Array.map (<anonymous>)
15:45:15         at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:26:27)
15:45:15         at Module._compile (internal/modules/cjs/loader.js:718:30)
15:45:15         at Object.Module._extensions..js (internal/modules/cjs/loader.js:729:10)
15:45:15         at Module.load (internal/modules/cjs/loader.js:617:32)
15:45:15         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
15:45:15         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
15:45:15         at Function.Module.runMain (internal/modules/cjs/loader.js:771:12)
15:45:15         at executeUserCode (internal/bootstrap/node.js:317:15)
15:45:15   ...

[eduardbme]

Fixed tests for Windows based systems

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/19570/

[Trott]

Still seems to be failing in relevant ways on Windows:

14:32:33 not ok 64 parallel/test-child-process-spawn-args
14:32:33   ---
14:32:33   duration_ms: 0.250
14:32:33   severity: fail
14:32:33   exitcode: 1
14:32:33   stack: |-
14:32:33     events.js:174
14:32:33           throw er; // Unhandled 'error' event
14:32:33           ^
14:32:33     
14:32:33     Error: spawn cd ENOENT
14:32:33         at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
14:32:33         at onErrorNT (internal/child_process.js:422:16)
14:32:33         at internalTickCallback (internal/process/next_tick.js:72:19)
14:32:33         at process._tickCallback (internal/process/next_tick.js:47:5)
14:32:33         at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
14:32:33         at executeUserCode (internal/bootstrap/node.js:318:15)
14:32:33         at startExecution (internal/bootstrap/node.js:252:3)
14:32:33         at startup (internal/bootstrap/node.js:203:3)
14:32:33         at internal/bootstrap/node.js:693:1
14:32:33     Emitted 'error' event at:
14:32:33         at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
14:32:33         at onErrorNT (internal/child_process.js:422:16)
14:32:33         [... lines matching original stack trace ...]
14:32:33         at internal/bootstrap/node.js:693:1
14:32:33   ...

Do you have access to a Windows machine? If not, maybe someone from @nodejs/platform-windows can help.

[eduardbme]

@Trott no, I don't but that typo could be the case, let's check one more time
thanks

[richardlau]

New CI: https://ci.nodejs.org/job/node-test-pull-request/19578/

[Trott]

Landed in 2077007

[targos]

The tests added in this PR fail for me locally:

$ ./node test/parallel/test-child-process-spawn-args.js
events.js:174
      throw er; // Unhandled 'error' event
      ^

Error: spawn pwd ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
    at onErrorNT (internal/child_process.js:422:16)
    at internalTickCallback (internal/process/next_tick.js:72:19)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
    at executeUserCode (internal/bootstrap/node.js:318:15)
    at startExecution (internal/bootstrap/node.js:252:3)
    at startup (internal/bootstrap/node.js:203:3)
    at internal/bootstrap/node.js:693:1
Emitted 'error' event at:
    at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
    at onErrorNT (internal/child_process.js:422:16)
    [... lines matching original stack trace ...]
    at internal/bootstrap/node.js:693:1

Am I missing something?

[eduardbme]

@targos

python tools/test.py -J --mode=release test/parallel/test-child-process-spawn-args.js

Details: https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests

[Trott]

@targos The test is missing a tmpdir.refresh() call. It doesn't (usually) affect things when run with the Python script because it assigns a tmpdir that is typically leftover from some other test, but when running from the command line, you may be less lucky.

I'll open a PR to fix.

[targos]

FWIW I initially saw the tests fail with make test -j8