Skip to content
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

child_process: spawn ignores options in case args is undefined #24913

Closed
wants to merge 1 commit into from
Closed

child_process: spawn ignores options in case args is undefined #24913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,9 @@ function normalizeSpawnArguments(file, args, options) {

if (Array.isArray(args)) {
args = args.slice(0);
} else if (args !== undefined &&
(args === null || typeof args !== 'object')) {
} else if (args == null) {
args = [];
} else if (typeof args !== 'object') {
throw new ERR_INVALID_ARG_TYPE('args', 'object', args);
} else {
options = args;
Expand Down
53 changes: 53 additions & 0 deletions test/parallel/test-child-process-spawn-args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';
Trott marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@sam-github Nit or requirement to land?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@eduardbme eduardbme Dec 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// This test confirms that `undefined`, `null`, and `[]`
// can be used as a placeholder for the second argument (`args`) of `spawn()`.
// Previously, there was a bug where using `undefined` for the second argument
// caused the third argument (`options`) to be ignored.
// See https://github.com/nodejs/node/issues/24912.

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

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

const command = common.isWindows ? 'cd' : 'pwd';
const options = { cwd: tmpdir.path };

if (common.isWindows) {
// This test is not the case for Windows based systems
// unless the `shell` options equals to `true`

options.shell = true;
}

const testCases = [
undefined,
null,
[],
];

const expectedResult = tmpdir.path.trim().toLowerCase();

(async () => {
const results = await Promise.all(
testCases.map((testCase) => {
return new Promise((resolve) => {
const subprocess = spawn(command, testCase, options);

let accumulatedData = Buffer.alloc(0);

subprocess.stdout.on('data', common.mustCall((data) => {
accumulatedData = Buffer.concat([accumulatedData, data]);
}));

subprocess.stdout.on('end', () => {
resolve(accumulatedData.toString().trim().toLowerCase());
});
});
})
);

assert.deepStrictEqual([...new Set(results)], [expectedResult]);
})();
8 changes: 2 additions & 6 deletions test/parallel/test-child-process-spawn-typeerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const invalidArgValueError =
common.expectsError({ code: 'ERR_INVALID_ARG_VALUE', type: TypeError }, 14);

const invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 13);
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 11);

assert.throws(function() {
spawn(invalidcmd, 'this is not an array');
Expand All @@ -59,10 +59,6 @@ assert.throws(function() {
spawn(file);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, null);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, true);
}, invalidArgTypeError);
Expand Down Expand Up @@ -103,9 +99,9 @@ spawn(cmd, o);

// Variants of undefined as explicit 'no argument' at a position.
spawn(cmd, u, o);
spawn(cmd, n, o);
spawn(cmd, a, u);

assert.throws(function() { spawn(cmd, n, o); }, invalidArgTypeError);
assert.throws(function() { spawn(cmd, a, n); }, invalidArgTypeError);

assert.throws(function() { spawn(cmd, s); }, invalidArgTypeError);
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-child-process-spawnsync-args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added same test for spawnSync method.


// This test confirms that `undefined`, `null`, and `[]` can be used
// as a placeholder for the second argument (`args`) of `spawnSync()`.
// Previously, there was a bug where using `undefined` for the second argument
// caused the third argument (`options`) to be ignored.
// See https://github.com/nodejs/node/issues/24912.

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

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

const command = common.isWindows ? 'cd' : 'pwd';
const options = { cwd: tmpdir.path };

if (common.isWindows) {
// This test is not the case for Windows based systems
// unless the `shell` options equals to `true`

options.shell = true;
}

const testCases = [
undefined,
null,
[],
];

const expectedResult = tmpdir.path.trim().toLowerCase();

const results = testCases.map((testCase) => {
const { stdout, stderr } = spawnSync(
command,
testCase,
options
);

assert.deepStrictEqual(stderr, Buffer.alloc(0));

return stdout.toString().trim().toLowerCase();
});

assert.deepStrictEqual([...new Set(results)], [expectedResult]);