diff --git a/lib/child_process.js b/lib/child_process.js index 711764dba7e06e..222df8aed7e8fd 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -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; diff --git a/test/parallel/test-child-process-spawn-args.js b/test/parallel/test-child-process-spawn-args.js new file mode 100644 index 00000000000000..35a1ea40208adf --- /dev/null +++ b/test/parallel/test-child-process-spawn-args.js @@ -0,0 +1,53 @@ +'use strict'; + +// 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]); +})(); diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 71bfdfdfe8f24f..738a6f36cfe810 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -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'); @@ -59,10 +59,6 @@ assert.throws(function() { spawn(file); }, invalidArgTypeError); -assert.throws(function() { - spawn(cmd, null); -}, invalidArgTypeError); - assert.throws(function() { spawn(cmd, true); }, invalidArgTypeError); @@ -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); diff --git a/test/parallel/test-child-process-spawnsync-args.js b/test/parallel/test-child-process-spawnsync-args.js new file mode 100644 index 00000000000000..d8dbf9c6bd56d0 --- /dev/null +++ b/test/parallel/test-child-process-spawnsync-args.js @@ -0,0 +1,45 @@ +'use strict'; + +// 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]);