Add informative message for missing executable on Windows (#2291)

lib/command.js

@@ -1103,6 +1103,26 @@ Expecting one of '${allowedValues.join("', '")}'`);
     return this;
   }
 
+  /**
+   * Throw if expected executable is missing. Add lots of help for author.
+   *
+   * @param {string} executableFile
+   * @param {string} executableDir
+   * @param {string} subcommandName
+   */
+  _checkForMissingExecutable(executableFile, executableDir, subcommandName) {
+    if (fs.existsSync(executableFile)) return;
+
+    const executableDirMessage = executableDir
+      ? `searched for local subcommand relative to directory '${executableDir}'`
+      : 'no directory for search for local subcommand, use .executableDir() to supply a custom directory';
+    const executableMissing = `'${executableFile}' does not exist
+ - if '${subcommandName}' is not meant to be an executable command, remove description parameter from '.command()' and use '.description()' instead
+ - if the default executable name is not suitable, use the executableFile option to supply a custom name or path
+ - ${executableDirMessage}`;
+    throw new Error(executableMissing);
+  }
+
   /**
    * Execute a sub-command executable.
    *
@@ -1186,6 +1206,11 @@ Expecting one of '${allowedValues.join("', '")}'`);
         proc = childProcess.spawn(executableFile, args, { stdio: 'inherit' });
       }
     } else {
+      this._checkForMissingExecutable(
+        executableFile,
+        executableDir,
+        subcommand._name,
+      );
       args.unshift(executableFile);
       // add executable arguments to spawn
       args = incrementNodeInspectorPort(process.execArgv).concat(args);
@@ -1224,14 +1249,11 @@ Expecting one of '${allowedValues.join("', '")}'`);
     proc.on('error', (err) => {
       // @ts-ignore: because err.code is an unknown property
       if (err.code === 'ENOENT') {
-        const executableDirMessage = executableDir
-          ? `searched for local subcommand relative to directory '${executableDir}'`
-          : 'no directory for search for local subcommand, use .executableDir() to supply a custom directory';
-        const executableMissing = `'${executableFile}' does not exist
- - if '${subcommand._name}' is not meant to be an executable command, remove description parameter from '.command()' and use '.description()' instead
- - if the default executable name is not suitable, use the executableFile option to supply a custom name or path
- - ${executableDirMessage}`;
-        throw new Error(executableMissing);
+        this._checkForMissingExecutable(
+          executableFile,
+          executableDir,
+          subcommand._name,
+        );
         // @ts-ignore: because err.code is an unknown property
       } else if (err.code === 'EACCES') {
         throw new Error(`'${executableFile}' not executable`);

tests/command.executableSubcommand.mock.test.js

@@ -12,47 +12,62 @@ function makeSystemError(code) {
   return err;
 }
 
-test('when subcommand executable missing (ENOENT) then throw custom message', () => {
-  // If the command is not found, we show a custom error with an explanation and offer
-  // some advice for possible fixes.
-  const mockProcess = new EventEmitter();
-  const spawnSpy = jest.spyOn(childProcess, 'spawn').mockImplementation(() => {
-    return mockProcess;
-  });
-  const program = new commander.Command();
-  program.exitOverride();
-  program.command('executable', 'executable description');
-  program.parse(['executable'], { from: 'user' });
-  expect(() => {
-    mockProcess.emit('error', makeSystemError('ENOENT'));
-  }).toThrow('use the executableFile option to supply a custom name'); // part of custom message
-  spawnSpy.mockRestore();
-});
+// Suppress false positive warnings due to use of testOrSkipOnWindows
+/* eslint-disable jest/no-standalone-expect */
 
-test('when subcommand executable not executable (EACCES) then throw custom message', () => {
-  // Side note: this error does not actually happen on Windows! But we can still simulate the behaviour on other platforms.
-  const mockProcess = new EventEmitter();
-  const spawnSpy = jest.spyOn(childProcess, 'spawn').mockImplementation(() => {
-    return mockProcess;
-  });
-  const program = new commander.Command();
-  program.exitOverride();
-  program.command('executable', 'executable description');
-  program.parse(['executable'], { from: 'user' });
-  expect(() => {
-    mockProcess.emit('error', makeSystemError('EACCES'));
-  }).toThrow('not executable'); // part of custom message
-  spawnSpy.mockRestore();
-});
+const testOrSkipOnWindows = process.platform === 'win32' ? test.skip : test;
+
+testOrSkipOnWindows(
+  'when subcommand executable missing (ENOENT) then throw custom message',
+  () => {
+    // If the command is not found, we show a custom error with an explanation and offer
+    // some advice for possible fixes.
+    const mockProcess = new EventEmitter();
+    const spawnSpy = jest
+      .spyOn(childProcess, 'spawn')
+      .mockImplementation(() => {
+        return mockProcess;
+      });
+    const program = new commander.Command();
+    program.exitOverride();
+    program.command('executable', 'executable description');
+    program.parse(['executable'], { from: 'user' });
+    expect(() => {
+      mockProcess.emit('error', makeSystemError('ENOENT'));
+    }).toThrow('use the executableFile option to supply a custom name'); // part of custom message
+    spawnSpy.mockRestore();
+  },
+);
+
+testOrSkipOnWindows(
+  'when subcommand executable not executable (EACCES) then throw custom message',
+  () => {
+    const mockProcess = new EventEmitter();
+    const spawnSpy = jest
+      .spyOn(childProcess, 'spawn')
+      .mockImplementation(() => {
+        return mockProcess;
+      });
+    const program = new commander.Command();
+    program.exitOverride();
+    program.command('executable', 'executable description');
+    program.parse(['executable'], { from: 'user' });
+    expect(() => {
+      mockProcess.emit('error', makeSystemError('EACCES'));
+    }).toThrow('not executable'); // part of custom message
+    spawnSpy.mockRestore();
+  },
+);
 
-test('when subcommand executable fails with other error  and exitOverride then return in custom wrapper', () => {
+test('when subcommand executable fails with other error and exitOverride then return in custom wrapper', () => {
   // The existing behaviour is to just silently fail for unexpected errors, as it is happening
   // asynchronously in spawned process and client can not catch errors.
   const mockProcess = new EventEmitter();
   const spawnSpy = jest.spyOn(childProcess, 'spawn').mockImplementation(() => {
     return mockProcess;
   });
   const program = new commander.Command();
+  program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
   program.exitOverride((err) => {
     throw err;
   });
@@ -78,6 +93,7 @@ test('when subcommand executable fails with other error then exit', () => {
   });
   const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {});
   const program = new commander.Command();
+  program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
   program.command('executable', 'executable description');
   program.parse(['executable'], { from: 'user' });
   mockProcess.emit('error', makeSystemError('OTHER'));

tests/command.executableSubcommand.search.test.js

@@ -50,13 +50,15 @@ describe('search for subcommand', () => {
     spawnSpy.mockRestore();
   });
 
+  // fs.existsSync gets called on Windows outside the search, so skip the tests (or come up with a different way of checking).
   describe('whether perform search for local files', () => {
     beforeEach(() => {
       existsSpy.mockImplementation(() => false);
     });
 
     test('when no script arg or executableDir then no search for local file', () => {
       const program = new commander.Command();
+      program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
       program.name('pm');
       program.command('sub', 'executable description');
       program.parse(['sub'], { from: 'user' });
@@ -66,6 +68,7 @@ describe('search for subcommand', () => {
 
     test('when script arg then search for local files', () => {
       const program = new commander.Command();
+      program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
       program.name('pm');
       program.command('sub', 'executable description');
       program.parse(['node', 'script-name', 'sub']);
@@ -75,6 +78,7 @@ describe('search for subcommand', () => {
 
     test('when executableDir then search for local files)', () => {
       const program = new commander.Command();
+      program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
       program.name('pm');
       program.executableDir(__dirname);
       program.command('sub', 'executable description');
@@ -301,6 +305,7 @@ describe('search for subcommand', () => {
     test('when script arg then search for local script-sub.js, .ts, .tsx, .mpjs, .cjs', () => {
       existsSpy.mockImplementation((path) => false);
       const program = new commander.Command();
+      program._checkForMissingExecutable = () => {}; // suppress error, call mocked spawn
       program.command('sub', 'executable description');
       const scriptPath = path.resolve(gLocalDirectory, 'script');
       program.parse(['node', scriptPath, 'sub']);