test: refacor spawn[Sync]Pwd

* extract the gist into common.pwdCommand
* Merge test-child-process-buffering.js into test-child-process-stdio.js

PR-URL: https://github.com/nodejs/node/pull/22522
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

7 files changed, 108 insertions, 148 deletions

diff --git a/test/common/README.md b/test/common/README.md
index 36f3afb5c9..28d9bd04f0 100644
--- a/test/common/README.md
+++ b/test/common/README.md
@@ -317,6 +317,17 @@ A port number for tests to use if one is needed.
 
 Logs '1..0 # Skipped: ' + `msg`
 
+### pwdCommand
+* [<array>] First two argument for the `spawn`/`exec` functions.
+
+Platform normalized `pwd` command options. Usage example:
+```js
+const common = require('../common');
+const { spawn } = require('child_process');
+
+spawn(...common.pwdCommand, { stdio: ['pipe'] });
+```
+
 ### rootDir
 * [<string>]
 
@@ -350,18 +361,6 @@ was disabled at compile time.
 Skip the rest of the tests in the current file when the Node.js executable
 was compiled with a pointer size smaller than 64 bits.
 
-### spawnPwd(options)
-* `options` [<Object>]
-* return [<Object>]
-
-Platform normalizes the `pwd` command.
-
-### spawnSyncPwd(options)
-* `options` [<Object>]
-* return [<Object>]
-
-Synchronous version of `spawnPwd`.
-
 ## ArrayStream Module
 
 The `ArrayStream` module provides a simple `Stream` that pushes elements from
diff --git a/test/common/index.js b/test/common/index.js
index 18f102c5c3..6f37cb734a 100644
--- a/test/common/index.js
+++ b/test/common/index.js
@@ -26,7 +26,7 @@ const path = require('path');
 const fs = require('fs');
 const assert = require('assert');
 const os = require('os');
-const { exec, execSync, spawn, spawnSync } = require('child_process');
+const { exec, execSync, spawnSync } = require('child_process');
 const util = require('util');
 const { fixturesDir } = require('./fixtures');
 const tmpdir = require('./tmpdir');
@@ -268,22 +268,10 @@ exports.ddCommand = function(filename, kilobytes) {
 };
 
 
-exports.spawnPwd = function(options) {
-  if (exports.isWindows) {
-    return spawn('cmd.exe', ['/d', '/c', 'cd'], options);
-  } else {
-    return spawn('pwd', [], options);
-  }
-};
-
+exports.pwdCommand = exports.isWindows ?
+  ['cmd.exe', ['/d', '/c', 'cd']] :
+  ['pwd', []];
 
-exports.spawnSyncPwd = function(options) {
-  if (exports.isWindows) {
-    return spawnSync('cmd.exe', ['/d', '/c', 'cd'], options);
-  } else {
-    return spawnSync('pwd', [], options);
-  }
-};
 
 exports.platformTimeout = function(ms) {
   if (process.features.debug)
diff --git a/test/parallel/test-child-process-buffering.js b/test/parallel/test-child-process-buffering.js
deleted file mode 100644
index d37d255e18..0000000000
--- a/test/parallel/test-child-process-buffering.js
+++ /dev/null
@@ -1,51 +0,0 @@
-// Copyright Joyent, Inc. and other Node contributors.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a
-// copy of this software and associated documentation files (the
-// "Software"), to deal in the Software without restriction, including
-// without limitation the rights to use, copy, modify, merge, publish,
-// distribute, sublicense, and/or sell copies of the Software, and to permit
-// persons to whom the Software is furnished to do so, subject to the
-// following conditions:
-//
-// The above copyright notice and this permission notice shall be included
-// in all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
-// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
-// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
-// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
-// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
-// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
-// USE OR OTHER DEALINGS IN THE SOFTWARE.
-
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-
-function pwd(callback) {
-  let output = '';
-  const child = common.spawnPwd();
-
-  child.stdout.setEncoding('utf8');
-  child.stdout.on('data', function(s) {
-    console.log(`stdout: ${JSON.stringify(s)}`);
-    output += s;
-  });
-
-  child.on('exit', common.mustCall(function(c) {
-    console.log(`exit: ${c}`);
-    assert.strictEqual(0, c);
-  }));
-
-  child.on('close', common.mustCall(function() {
-    callback(output);
-  }));
-}
-
-
-pwd(function(result) {
-  console.dir(result);
-  assert.strictEqual(true, result.length > 1);
-  assert.strictEqual('\n', result[result.length - 1]);
-});
diff --git a/test/parallel/test-child-process-custom-fds.js b/test/parallel/test-child-process-custom-fds.js
index c314656476..1c89c207d3 100644
--- a/test/parallel/test-child-process-custom-fds.js
+++ b/test/parallel/test-child-process-custom-fds.js
@@ -2,19 +2,22 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
+const { spawnSync } = require('child_process');
 const internalCp = require('internal/child_process');
-const oldSpawnSync = internalCp.spawnSync;
 
 if (!common.isMainThread)
   common.skip('stdio is not associated with file descriptors in Workers');
 
+// This test uses the deprecated `customFds` option. We expect a deprecation
+// warning, but only once (per node process).
+const msg = 'child_process: options.customFds option is deprecated. ' +
+            'Use options.stdio instead.';
+common.expectWarning('DeprecationWarning', msg, 'DEP0006');
+
 // Verify that customFds is used if stdio is not provided.
 {
-  const msg = 'child_process: options.customFds option is deprecated. ' +
-              'Use options.stdio instead.';
-  common.expectWarning('DeprecationWarning', msg, 'DEP0006');
-
   const customFds = [-1, process.stdout.fd, process.stderr.fd];
+  const oldSpawnSync = internalCp.spawnSync;
   internalCp.spawnSync = common.mustCall(function(opts) {
     assert.deepStrictEqual(opts.options.customFds, customFds);
     assert.deepStrictEqual(opts.options.stdio, [
@@ -23,13 +26,14 @@ if (!common.isMainThread)
       { type: 'fd', fd: process.stderr.fd }
     ]);
   });
-  common.spawnSyncPwd({ customFds });
+  spawnSync(...common.pwdCommand, { customFds });
   internalCp.spawnSync = oldSpawnSync;
 }
 
 // Verify that customFds is ignored when stdio is present.
 {
   const customFds = [0, 1, 2];
+  const oldSpawnSync = internalCp.spawnSync;
   internalCp.spawnSync = common.mustCall(function(opts) {
     assert.deepStrictEqual(opts.options.customFds, customFds);
     assert.deepStrictEqual(opts.options.stdio, [
@@ -38,6 +42,6 @@ if (!common.isMainThread)
       { type: 'pipe', readable: false, writable: true }
     ]);
   });
-  common.spawnSyncPwd({ customFds, stdio: 'pipe' });
+  spawnSync(...common.pwdCommand, { customFds, stdio: 'pipe' });
   internalCp.spawnSync = oldSpawnSync;
 }
diff --git a/test/parallel/test-child-process-cwd.js b/test/parallel/test-child-process-cwd.js
index e1dcf85b09..fdca618a8b 100644
--- a/test/parallel/test-child-process-cwd.js
+++ b/test/parallel/test-child-process-cwd.js
@@ -22,47 +22,37 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-
-let returns = 0;
+const { spawn } = require('child_process');
 
 /*
   Spawns 'pwd' with given options, then test
-  - whether the exit code equals forCode,
-  - optionally whether the stdout result matches forData
-    (after removing trailing whitespace)
+  - whether the exit code equals expectCode,
+  - optionally whether the trimmed stdout result matches expectData
 */
-function testCwd(options, forCode, forData) {
-  let data = '';
-
-  const child = common.spawnPwd(options);
+function testCwd(options, expectCode = 0, expectData) {
+  const child = spawn(...common.pwdCommand, options);
 
   child.stdout.setEncoding('utf8');
 
+  // No need to assert callback since `data` is asserted.
+  let data = '';
   child.stdout.on('data', function(chunk) {
     data += chunk;
   });
 
+  // Can't assert callback, as stayed in to API:
+  // _The 'exit' event may or may not fire after an error has occurred._
   child.on('exit', function(code, signal) {
-    assert.strictEqual(forCode, code);
-  });
-
-  child.on('close', function() {
-    forData && assert.strictEqual(forData, data.replace(/[\s\r\n]+$/, ''));
-    returns--;
+    assert.strictEqual(expectCode, code);
   });
 
-  returns++;
+  child.on('close', common.mustCall(function() {
+    expectData && assert.strictEqual(data.trim(), expectData);
+  }));
 
   return child;
 }
 
-// Assume these exist, and 'pwd' gives us the right directory back
-testCwd({ cwd: common.rootDir }, 0, common.rootDir);
-if (common.isWindows) {
-  testCwd({ cwd: process.env.windir }, 0, process.env.windir);
-} else {
-  testCwd({ cwd: '/dev' }, 0, '/dev');
-}
 
 // Assume does-not-exist doesn't exist, expect exitCode=-1 and errno=ENOENT
 {
@@ -72,15 +62,12 @@ if (common.isWindows) {
     }));
 }
 
-// Spawn() shouldn't try to chdir() so this should just work
-testCwd(undefined, 0);
-testCwd({}, 0);
-testCwd({ cwd: '' }, 0);
-testCwd({ cwd: undefined }, 0);
-testCwd({ cwd: null }, 0);
+// Assume these exist, and 'pwd' gives us the right directory back
+testCwd({ cwd: common.rootDir }, 0, common.rootDir);
+const shouldExistDir = common.isWindows ? process.env.windir : '/dev';
+testCwd({ cwd: shouldExistDir }, 0, shouldExistDir);
 
-// Check whether all tests actually returned
-assert.notStrictEqual(returns, 0);
-process.on('exit', function() {
-  assert.strictEqual(returns, 0);
-});
+// Spawn() shouldn't try to chdir() to invalid arg, so this should just work
+testCwd({ cwd: '' });
+testCwd({ cwd: undefined });
+testCwd({ cwd: null });
diff --git a/test/parallel/test-child-process-spawnsync.js b/test/parallel/test-child-process-spawnsync.js
index 6ff7b41eda..99ce75da12 100644
--- a/test/parallel/test-child-process-spawnsync.js
+++ b/test/parallel/test-child-process-spawnsync.js
@@ -22,10 +22,9 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
+const { spawnSync } = require('child_process');
 
-const spawnSync = require('child_process').spawnSync;
-
-// Echo does different things on Windows and Unix, but in both cases, it does
+// `sleep` does different things on Windows and Unix, but in both cases, it does
 // more-or-less nothing if there are no parameters
 const ret = spawnSync('sleep', ['0']);
 assert.strictEqual(ret.status, 0);
@@ -42,21 +41,23 @@ assert.deepStrictEqual(ret_err.spawnargs, ['bar']);
 {
   // Test the cwd option
   const cwd = common.rootDir;
-  const response = common.spawnSyncPwd({ cwd });
+  const response = spawnSync(...common.pwdCommand, { cwd });
 
   assert.strictEqual(response.stdout.toString().trim(), cwd);
 }
 
+
 {
-  // Test the encoding option
-  const noEncoding = common.spawnSyncPwd();
-  const bufferEncoding = common.spawnSyncPwd({ encoding: 'buffer' });
-  const utf8Encoding = common.spawnSyncPwd({ encoding: 'utf8' });
+  // Assert Buffer is the default encoding
+  const retDefault = spawnSync(...common.pwdCommand);
+  const retBuffer = spawnSync(...common.pwdCommand, { encoding: 'buffer' });
+  assert.deepStrictEqual(retDefault.output, retBuffer.output);
 
-  assert.deepStrictEqual(noEncoding.output, bufferEncoding.output);
-  assert.deepStrictEqual([
+  const retUTF8 = spawnSync(...common.pwdCommand, { encoding: 'utf8' });
+  const stringifiedDefault = [
     null,
-    noEncoding.stdout.toString(),
-    noEncoding.stderr.toString()
-  ], utf8Encoding.output);
+    retDefault.stdout.toString(),
+    retDefault.stderr.toString()
+  ];
+  assert.deepStrictEqual(retUTF8.output, stringifiedDefault);
 }
diff --git a/test/parallel/test-child-process-stdio.js b/test/parallel/test-child-process-stdio.js
index 8624a13e1b..5ca3875f56 100644
--- a/test/parallel/test-child-process-stdio.js
+++ b/test/parallel/test-child-process-stdio.js
@@ -22,24 +22,56 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const spawnSync = require('child_process').spawnSync;
+const { spawn } = require('child_process');
 
-let options = { stdio: ['pipe'] };
-let child = common.spawnPwd(options);
+// Test stdio piping.
+{
+  const child = spawn(...common.pwdCommand, { stdio: ['pipe'] });
+  assert.notStrictEqual(child.stdout, null);
+  assert.notStrictEqual(child.stderr, null);
+}
 
-assert.notStrictEqual(child.stdout, null);
-assert.notStrictEqual(child.stderr, null);
+// Test stdio ignoring.
+{
+  const child = spawn(...common.pwdCommand, { stdio: 'ignore' });
+  assert.strictEqual(child.stdout, null);
+  assert.strictEqual(child.stderr, null);
+}
 
-options = { stdio: 'ignore' };
-child = common.spawnPwd(options);
+// Asset options invariance.
+{
+  const options = { stdio: 'ignore' };
+  spawn(...common.pwdCommand, options);
+  assert.deepStrictEqual(options, { stdio: 'ignore' });
+}
 
-assert.strictEqual(child.stdout, null);
-assert.strictEqual(child.stderr, null);
+// Test stdout buffering.
+{
+  let output = '';
+  const child = spawn(...common.pwdCommand);
 
-options = { stdio: 'ignore' };
-child = spawnSync('cat', [], options);
-assert.deepStrictEqual(options, { stdio: 'ignore' });
+  child.stdout.setEncoding('utf8');
+  child.stdout.on('data', function(s) {
+    output += s;
+  });
 
-common.expectsError(() => {
-  common.spawnPwd({ stdio: ['pipe', 'pipe', 'pipe', 'ipc', 'ipc'] });
-}, { code: 'ERR_IPC_ONE_PIPE', type: Error });
+  child.on('exit', common.mustCall(function(code) {
+    assert.strictEqual(code, 0);
+  }));
+
+  child.on('close', common.mustCall(function() {
+    assert.strictEqual(true, output.length > 1);
+    assert.strictEqual('\n', output[output.length - 1]);
+  }));
+}
+
+// Assert only one IPC pipe allowed.
+common.expectsError(
+  () => {
+    spawn(
+      ...common.pwdCommand,
+      { stdio: ['pipe', 'pipe', 'pipe', 'ipc', 'ipc'] }
+    );
+  },
+  { code: 'ERR_IPC_ONE_PIPE', type: Error }
+);