From a973446b91f6b026d4964b9db430cfc58be646f7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 5 Aug 2023 09:29:53 +0200 Subject: [PATCH] test: use `spawn` and `spawnPromisified` instead of `exec` PR-URL: https://github.com/nodejs/node/pull/48991 Reviewed-By: Chemi Atlow Reviewed-By: Debadree Chatterjee --- test/parallel/test-preload.js | 158 ++++++++---------- .../test-timers-immediate-promisified.js | 6 +- .../test-timers-interval-promisified.js | 6 +- .../test-timers-timeout-promisified.js | 6 +- test/parallel/test-url-parse-invalid-input.js | 14 +- test/sequential/test-cli-syntax-require.js | 21 ++- test/sequential/test-fs-stat-sync-overflow.js | 18 +- 7 files changed, 101 insertions(+), 128 deletions(-) diff --git a/test/parallel/test-preload.js b/test/parallel/test-preload.js index 3e18e39e0b9d2e..24fe181da20423 100644 --- a/test/parallel/test-preload.js +++ b/test/parallel/test-preload.js @@ -11,12 +11,8 @@ const childProcess = require('child_process'); const nodeBinary = process.argv[0]; -const preloadOption = (preloads) => { - let option = ''; - preloads.forEach(function(preload, index) { - option += `-r "${preload}" `; - }); - return option; +const preloadOption = (...preloads) => { + return preloads.flatMap((preload) => ['-r', preload]); }; const fixtureA = fixtures.path('printA.js'); @@ -34,75 +30,52 @@ assert(!module.isPreloading); // Test that module.isPreloading is set in preloaded module // Test preloading a single module works -childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureIsPreloading])} "${fixtureB}"`, - function(err, stdout, stderr) { - assert.ifError(err); - assert.strictEqual(stdout, 'B\n'); - }); +common.spawnPromisified(nodeBinary, [...preloadOption(fixtureIsPreloading), fixtureB]).then(common.mustCall( + ({ stdout }) => { + assert.strictEqual(stdout.trim(), 'B'); + } +)); // Test preloading a single module works -childProcess.exec(`"${nodeBinary}" ${preloadOption([fixtureA])} "${fixtureB}"`, - function(err, stdout, stderr) { - assert.ifError(err); - assert.strictEqual(stdout, 'A\nB\n'); - }); +common.spawnPromisified(nodeBinary, [...preloadOption(fixtureA), fixtureB]).then(common.mustCall( + ({ stdout }) => { + assert.strictEqual(stdout, 'A\nB\n'); + })); // Test preloading multiple modules works -childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureA, fixtureB])} "${fixtureC}"`, - function(err, stdout, stderr) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, [...preloadOption(fixtureA, fixtureB), fixtureC]).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'A\nB\nC\n'); } -); +)); // Test that preloading a throwing module aborts -childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureA, fixtureThrows])} "${fixtureB}"`, - function(err, stdout, stderr) { - if (err) { - assert.strictEqual(stdout, 'A\n'); - } else { - throw new Error('Preload should have failed'); - } +common.spawnPromisified(nodeBinary, [...preloadOption(fixtureA, fixtureThrows), fixtureB]).then(common.mustCall( + ({ code, stdout }) => { + assert.strictEqual(stdout, 'A\n'); + assert.strictEqual(code, 1); } -); +)); // Test that preload can be used with --eval -childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureA])}-e "console.log('hello');"`, - function(err, stdout, stderr) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, [...preloadOption(fixtureA), '-e', 'console.log("hello");']).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'A\nhello\n'); } -); +)); // Test that preload can be used with --frozen-intrinsics -childProcess.exec( - `"${nodeBinary}" --frozen-intrinsics ${ - preloadOption([fixtureE]) - } ${ - fixtureF - }`, - function(err, stdout) { - assert.ifError(err); - assert.strictEqual(stdout, 'smoosh\n'); - } -); -childProcess.exec( - `"${ - nodeBinary - }" --frozen-intrinsics ${ - preloadOption([fixtureE]) - } ${ - fixtureG - } ${fixtureF}`, - function(err, stdout) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, ['--frozen-intrinsics', ...preloadOption(fixtureE), fixtureF]).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'smoosh\n'); } -); +)); +common.spawnPromisified(nodeBinary, ['--frozen-intrinsics', ...preloadOption(fixtureE), fixtureG, fixtureF]) + .then(common.mustCall( + ({ stdout }) => { + assert.strictEqual(stdout, 'smoosh\n'); + } + )); // Test that preload can be used with stdin const stdinProc = childProcess.spawn( @@ -143,61 +116,62 @@ replProc.on('close', function(code) { // Test that preload placement at other points in the cmdline // also test that duplicated preload only gets loaded once -childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureA])}-e "console.log('hello');" ${ - preloadOption([fixtureA, fixtureB])}`, - function(err, stdout, stderr) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, [ + ...preloadOption(fixtureA), + '-e', 'console.log("hello");', + ...preloadOption(fixtureA, fixtureB), +]).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'A\nB\nhello\n'); } -); +)); // Test that preload works with -i -const interactive = childProcess.exec( - `"${nodeBinary}" ${preloadOption([fixtureD])}-i`, - common.mustSucceed((stdout, stderr) => { - assert.ok(stdout.endsWith("> 'test'\n> ")); - }) -); +const interactive = childProcess.spawn(nodeBinary, [...preloadOption(fixtureD), '-i']); + +{ + const stdout = []; + interactive.stdout.on('data', (chunk) => { + stdout.push(chunk); + }); + interactive.on('close', common.mustCall(() => { + assert.match(Buffer.concat(stdout).toString('utf8'), /> 'test'\r?\n> $/); + })); +} interactive.stdin.write('a\n'); interactive.stdin.write('process.exit()\n'); -childProcess.exec( - `"${nodeBinary}" --require "${fixtures.path('cluster-preload.js')}" "${ - fixtures.path('cluster-preload-test.js')}"`, - function(err, stdout, stderr) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, + ['--require', fixtures.path('cluster-preload.js'), fixtures.path('cluster-preload-test.js')]) + .then(common.mustCall(({ stdout }) => { assert.match(stdout, /worker terminated with code 43/); - } -); + })); // Test that preloading with a relative path works -childProcess.exec( - `"${nodeBinary}" ${preloadOption(['./printA.js'])} "${fixtureB}"`, - { cwd: fixtures.fixturesDir }, - common.mustSucceed((stdout, stderr) => { +common.spawnPromisified(nodeBinary, + [...preloadOption('./printA.js'), fixtureB], + { cwd: fixtures.fixturesDir }).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'A\nB\n'); }) ); if (common.isWindows) { // https://github.com/nodejs/node/issues/21918 - childProcess.exec( - `"${nodeBinary}" ${preloadOption(['.\\printA.js'])} "${fixtureB}"`, - { cwd: fixtures.fixturesDir }, - common.mustSucceed((stdout, stderr) => { + common.spawnPromisified(nodeBinary, + [...preloadOption('.\\printA.js'), fixtureB], + { cwd: fixtures.fixturesDir }).then(common.mustCall( + ({ stdout }) => { assert.strictEqual(stdout, 'A\nB\n'); }) ); } // https://github.com/nodejs/node/issues/1691 -childProcess.exec( - `"${nodeBinary}" --require ` + - `"${fixtures.path('cluster-preload.js')}" cluster-preload-test.js`, - { cwd: fixtures.fixturesDir }, - function(err, stdout, stderr) { - assert.ifError(err); +common.spawnPromisified(nodeBinary, + ['--require', fixtures.path('cluster-preload.js'), 'cluster-preload-test.js'], + { cwd: fixtures.fixturesDir }).then(common.mustCall( + ({ stdout }) => { assert.match(stdout, /worker terminated with code 43/); } -); +)); diff --git a/test/parallel/test-timers-immediate-promisified.js b/test/parallel/test-timers-immediate-promisified.js index b504ce1778e8d9..586fd501273ae3 100644 --- a/test/parallel/test-timers-immediate-promisified.js +++ b/test/parallel/test-timers-immediate-promisified.js @@ -4,7 +4,6 @@ const common = require('../common'); const assert = require('assert'); const timers = require('timers'); const { promisify } = require('util'); -const child_process = require('child_process'); const { getEventListeners } = require('events'); const { NodeEventTarget } = require('internal/event_target'); @@ -12,7 +11,6 @@ const { NodeEventTarget } = require('internal/event_target'); const timerPromises = require('timers/promises'); const setPromiseImmediate = promisify(timers.setImmediate); -const exec = promisify(child_process.exec); assert.strictEqual(setPromiseImmediate, timerPromises.setImmediate); @@ -91,9 +89,9 @@ process.on('multipleResolves', common.mustNotCall()); } { - exec(`${process.execPath} -pe "const assert = require('assert');` + + common.spawnPromisified(process.execPath, ['-pe', "const assert = require('assert');" + 'require(\'timers/promises\').setImmediate(null, { ref: false }).' + - 'then(assert.fail)"').then(common.mustCall(({ stderr }) => { + 'then(assert.fail)']).then(common.mustCall(({ stderr }) => { assert.strictEqual(stderr, ''); })); } diff --git a/test/parallel/test-timers-interval-promisified.js b/test/parallel/test-timers-interval-promisified.js index 46b977ff80a8ef..e9434625e38026 100644 --- a/test/parallel/test-timers-interval-promisified.js +++ b/test/parallel/test-timers-interval-promisified.js @@ -4,7 +4,6 @@ const common = require('../common'); const assert = require('assert'); const timers = require('timers'); const { promisify } = require('util'); -const child_process = require('child_process'); const { getEventListeners } = require('events'); const { NodeEventTarget } = require('internal/event_target'); @@ -12,7 +11,6 @@ const { NodeEventTarget } = require('internal/event_target'); const timerPromises = require('timers/promises'); const setPromiseTimeout = promisify(timers.setTimeout); -const exec = promisify(child_process.exec); const { setInterval } = timerPromises; @@ -154,11 +152,11 @@ process.on('multipleResolves', common.mustNotCall()); } { - exec(`${process.execPath} -pe "const assert = require('assert');` + + common.spawnPromisified(process.execPath, ['-pe', "const assert = require('assert');" + 'const interval = require(\'timers/promises\')' + '.setInterval(1000, null, { ref: false });' + 'interval[Symbol.asyncIterator]().next()' + - '.then(assert.fail)"').then(common.mustCall(({ stderr }) => { + '.then(assert.fail)']).then(common.mustCall(({ stderr }) => { assert.strictEqual(stderr, ''); })); } diff --git a/test/parallel/test-timers-timeout-promisified.js b/test/parallel/test-timers-timeout-promisified.js index 95298c6a42977d..a63923b7feea86 100644 --- a/test/parallel/test-timers-timeout-promisified.js +++ b/test/parallel/test-timers-timeout-promisified.js @@ -4,7 +4,6 @@ const common = require('../common'); const assert = require('assert'); const timers = require('timers'); const { promisify } = require('util'); -const child_process = require('child_process'); const { getEventListeners } = require('events'); const { NodeEventTarget } = require('internal/event_target'); @@ -12,7 +11,6 @@ const { NodeEventTarget } = require('internal/event_target'); const timerPromises = require('timers/promises'); const setPromiseTimeout = promisify(timers.setTimeout); -const exec = promisify(child_process.exec); assert.strictEqual(setPromiseTimeout, timerPromises.setTimeout); @@ -91,9 +89,9 @@ process.on('multipleResolves', common.mustNotCall()); } { - exec(`${process.execPath} -pe "const assert = require('assert');` + + common.spawnPromisified(process.execPath, ['-pe', 'const assert = require(\'assert\');' + 'require(\'timers/promises\').setTimeout(1000, null, { ref: false }).' + - 'then(assert.fail)"').then(common.mustCall(({ stderr }) => { + 'then(assert.fail)']).then(common.mustCall(({ stderr }) => { assert.strictEqual(stderr, ''); })); } diff --git a/test/parallel/test-url-parse-invalid-input.js b/test/parallel/test-url-parse-invalid-input.js index 023f74ee86a70e..6f655843e51ffd 100644 --- a/test/parallel/test-url-parse-invalid-input.js +++ b/test/parallel/test-url-parse-invalid-input.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const childProcess = require('child_process'); const url = require('url'); // https://github.com/joyent/node/issues/568 @@ -82,13 +81,12 @@ if (common.hasIntl) { 'git+ssh://git@github.com:npm/npm', ]; badURLs.forEach((badURL) => { - childProcess.exec(`${process.execPath} -e "url.parse('${badURL}')"`, - common.mustCall((err, stdout, stderr) => { - assert.strictEqual(err, null); - assert.strictEqual(stdout, ''); - assert.match(stderr, /\[DEP0170\] DeprecationWarning:/); - }) - ); + common.spawnPromisified(process.execPath, ['-e', `url.parse(${JSON.stringify(badURL)})`]) + .then(common.mustCall(({ code, stdout, stderr }) => { + assert.strictEqual(code, 0); + assert.strictEqual(stdout, ''); + assert.match(stderr, /\[DEP0170\] DeprecationWarning:/); + })); }); // Warning should only happen once per process. diff --git a/test/sequential/test-cli-syntax-require.js b/test/sequential/test-cli-syntax-require.js index e97eb5953aac0b..055458ca8542b1 100644 --- a/test/sequential/test-cli-syntax-require.js +++ b/test/sequential/test-cli-syntax-require.js @@ -2,7 +2,7 @@ const common = require('../common'); const assert = require('assert'); -const { exec } = require('child_process'); +const { spawn } = require('child_process'); const fixtures = require('../common/fixtures'); const node = process.execPath; @@ -17,20 +17,25 @@ const syntaxErrorRE = /^SyntaxError: \b/m; const preloadFile = fixtures.path('no-wrapper.js'); const file = fixtures.path('syntax', 'illegal_if_not_wrapped.js'); const args = [requireFlag, preloadFile, checkFlag, file]; - const cmd = [node, ...args].join(' '); - exec(cmd, common.mustCall((err, stdout, stderr) => { - assert.strictEqual(err instanceof Error, true); - assert.strictEqual(err.code, 1, - `code ${err.code} !== 1 for error:\n\n${err}`); + const cp = spawn(node, args); - // No stdout should be produced - assert.strictEqual(stdout, ''); + // No stdout should be produced + cp.stdout.on('data', common.mustNotCall('stdout data event')); + const stderrBuffer = []; + cp.stderr.on('data', (chunk) => stderrBuffer.push(chunk)); + + cp.on('exit', common.mustCall((code, signal) => { + assert.strictEqual(code, 1); + assert.strictEqual(signal, null); + + const stderr = Buffer.concat(stderrBuffer).toString('utf-8'); // stderr should have a syntax error message assert.match(stderr, syntaxErrorRE); // stderr should include the filename assert(stderr.startsWith(file), `${stderr} starts with ${file}`); })); + }); }); diff --git a/test/sequential/test-fs-stat-sync-overflow.js b/test/sequential/test-fs-stat-sync-overflow.js index 8855c374bf7d2b..0150ce0c2d43ba 100644 --- a/test/sequential/test-fs-stat-sync-overflow.js +++ b/test/sequential/test-fs-stat-sync-overflow.js @@ -20,8 +20,8 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -require('../common'); -const { fixturesDir } = require('../common/fixtures'); +const common = require('../common'); +const fixtures = require('../common/fixtures'); // Check that the calls to Integer::New() and Date::New() succeed and bail out // if they don't. @@ -31,11 +31,13 @@ const { fixturesDir } = require('../common/fixtures'); // https://github.com/nodejs/node-v0.x-archive/issues/4015 const assert = require('assert'); -const { exec } = require('child_process'); +const { spawn } = require('child_process'); -const cmd = - `"${process.execPath}" "${fixturesDir}/test-fs-stat-sync-overflow.js"`; +const cp = spawn(process.execPath, [fixtures.path('test-fs-stat-sync-overflow.js')]); -exec(cmd, function(err, stdout, stderr) { - assert.match(stderr, /RangeError: Maximum call stack size exceeded/); -}); +const stderr = []; +cp.stderr.on('data', (chunk) => stderr.push(chunk)); + +cp.on('exit', common.mustCall(() => { + assert.match(Buffer.concat(stderr).toString('utf8'), /RangeError: Maximum call stack size exceeded/); +}));