From a8ef5cf3e1975380974ab5c4f92c26fb2c5e3209 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 8 Dec 2024 14:47:27 +1300 Subject: [PATCH] Add informative message for missing executable on Windows (#2291) --- lib/command.js | 38 +++++++-- .../command.executableSubcommand.mock.test.js | 80 +++++++++++-------- ...ommand.executableSubcommand.search.test.js | 5 ++ 3 files changed, 83 insertions(+), 40 deletions(-) diff --git a/lib/command.js b/lib/command.js index ac36d8d0d..e27d2e53f 100644 --- a/lib/command.js +++ b/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`); diff --git a/tests/command.executableSubcommand.mock.test.js b/tests/command.executableSubcommand.mock.test.js index 460cbb1b8..4c80d58ef 100644 --- a/tests/command.executableSubcommand.mock.test.js +++ b/tests/command.executableSubcommand.mock.test.js @@ -12,40 +12,54 @@ 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(); @@ -53,6 +67,7 @@ test('when subcommand executable fails with other error and exitOverride then r 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')); diff --git a/tests/command.executableSubcommand.search.test.js b/tests/command.executableSubcommand.search.test.js index a2ebdaf4a..96a1302ff 100644 --- a/tests/command.executableSubcommand.search.test.js +++ b/tests/command.executableSubcommand.search.test.js @@ -50,6 +50,7 @@ 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); @@ -57,6 +58,7 @@ describe('search for subcommand', () => { 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']);