Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add informative message for missing executable on Windows #2291

Prev Previous commit
Next Next commit
Another try at co-existing with unit tests!
shadowspawn committed Dec 6, 2024
commit 9dfaf0926939c08f4c88d78b6513713b82c4ad0c
42 changes: 28 additions & 14 deletions lib/command.js
Original file line number Diff line number Diff line change
@@ -1094,6 +1094,24 @@ Expecting one of '${allowedValues.join("', '")}'`);
return this;
}

/**
* Note: masked in some unit tests.
* @param {string} executableFile
* @param {string} executableDir
* @param {string} subcommandName
*/
_throwForMissingExecutable(executableFile, executableDir, subcommandName) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name is _throwForMissingExecutable, but it may not throw, so why not name it something like _validateExecutable?

Or how about doing the checks outside of the method?

if (!fs.existsSync(executableFile)) {
 _throwForMissingExecutable(...);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method name

Oh, yes. I originally had the check outside the routine. Will change name.

Or how about doing the checks outside of the method?

I agree that would be tidier for the code and usage. However, I started that way, and multiple unit tests check whether fs.existsSync has been called and broke. I didn't think of an easy way of fixing the tests, other than moving the fs. fs.existsSync inside the routine. Then easy to completely bypass the new code and run the old tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have several _checkForX routines already so following that style, _checkForMissingExecutable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots!

  • _checkForMissingMandatoryOptions
  • _checkForConflictingLocalOptions
  • _checkForConflictingOptions
  • _checkForBrokenPassThrough

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.
*
@@ -1165,21 +1183,9 @@ Expecting one of '${allowedValues.join("', '")}'`);

launchWithNode = sourceExt.includes(path.extname(executableFile));

function throwMissingExecutable() {
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);
}

let proc;
if (process.platform !== 'win32') {
if (launchWithNode) {
if (!fs.existsSync(executableFile)) throwMissingExecutable();
args.unshift(executableFile);
// add executable arguments to spawn
args = incrementNodeInspectorPort(process.execArgv).concat(args);
@@ -1189,7 +1195,11 @@ Expecting one of '${allowedValues.join("', '")}'`);
proc = childProcess.spawn(executableFile, args, { stdio: 'inherit' });
}
} else {
if (!fs.existsSync(executableFile)) throwMissingExecutable();
this._throwForMissingExecutable(
executableFile,
executableDir,
subcommand._name,
);
args.unshift(executableFile);
// add executable arguments to spawn
args = incrementNodeInspectorPort(process.execArgv).concat(args);
@@ -1228,7 +1238,11 @@ Expecting one of '${allowedValues.join("', '")}'`);
proc.on('error', (err) => {
// @ts-ignore: because err.code is an unknown property
if (err.code === 'ENOENT') {
throwMissingExecutable();
this._throwForMissingExecutable(
executableFile,
executableDir,
subcommand._name,
);
// @ts-ignore: because err.code is an unknown property
} else if (err.code === 'EACCES') {
throw new Error(`'${executableFile}' not executable`);
101 changes: 52 additions & 49 deletions tests/command.executableSubcommand.mock.test.js
Original file line number Diff line number Diff line change
@@ -12,12 +12,14 @@ function makeSystemError(code) {
return err;
}

// These tests are either not relevant to Windows, or don't failure in same way because fail early for missing executable.
const describeOrSkipOnWindows =
process.platform === 'win32' ? describe.skip : describe;
// Suppress false positive warnings due to use of testOrSkipOnWindows
/* eslint-disable jest/no-standalone-expect */

describeOrSkipOnWindows('executable Subcommand failure modes', () => {
test('when subcommand executable missing (ENOENT) then throw custom message', () => {
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();
@@ -34,9 +36,12 @@ describeOrSkipOnWindows('executable Subcommand failure modes', () => {
mockProcess.emit('error', makeSystemError('ENOENT'));
}).toThrow('use the executableFile option to supply a custom name'); // part of custom message
spawnSpy.mockRestore();
});
},
);

test('when subcommand executable not executable (EACCES) then throw custom message', () => {
testOrSkipOnWindows(
'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
@@ -52,50 +57,48 @@ describeOrSkipOnWindows('executable Subcommand failure modes', () => {
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', () => {
// 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.exitOverride((err) => {
throw err;
});
program.command('executable', 'executable description');
program.parse(['executable'], { from: 'user' });
let caughtErr;
try {
mockProcess.emit('error', makeSystemError('OTHER'));
} catch (err) {
caughtErr = err;
}
expect(caughtErr.code).toEqual('commander.executeSubCommandAsync');
expect(caughtErr.nestedError.code).toEqual('OTHER');
spawnSpy.mockRestore();
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;
});

test('when subcommand executable fails with other error then exit', () => {
// 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 exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {});
const program = new commander.Command();
program.command('executable', 'executable description');
program.parse(['executable'], { from: 'user' });
const program = new commander.Command();
program._throwForMissingExecutable = () => {}; // suppress error, call mocked spawn
program.exitOverride((err) => {
throw err;
});
program.command('executable', 'executable description');
program.parse(['executable'], { from: 'user' });
let caughtErr;
try {
mockProcess.emit('error', makeSystemError('OTHER'));
expect(exitSpy).toHaveBeenCalledWith(1);
exitSpy.mockRestore();
spawnSpy.mockRestore();
} catch (err) {
caughtErr = err;
}
expect(caughtErr.code).toEqual('commander.executeSubCommandAsync');
expect(caughtErr.nestedError.code).toEqual('OTHER');
spawnSpy.mockRestore();
});

test('when subcommand executable fails with other error then exit', () => {
// 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 exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {});
const program = new commander.Command();
program._throwForMissingExecutable = () => {}; // suppress error, call mocked spawn
program.command('executable', 'executable description');
program.parse(['executable'], { from: 'user' });
mockProcess.emit('error', makeSystemError('OTHER'));
expect(exitSpy).toHaveBeenCalledWith(1);
exitSpy.mockRestore();
spawnSpy.mockRestore();
});
5 changes: 4 additions & 1 deletion tests/command.executableSubcommand.search.test.js
Original file line number Diff line number Diff line change
@@ -51,13 +51,14 @@ describe('search for subcommand', () => {
});

// fs.existsSync gets called on Windows outside the search, so skip the tests (or come up with a different way of checking).
describeOrSkipOnWindows('whether perform search for local files', () => {
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._throwForMissingExecutable = () => {}; // suppress error, call mocked spawn
program.name('pm');
program.command('sub', 'executable description');
program.parse(['sub'], { from: 'user' });
@@ -67,6 +68,7 @@ describe('search for subcommand', () => {

test('when script arg then search for local files', () => {
const program = new commander.Command();
program._throwForMissingExecutable = () => {}; // suppress error, call mocked spawn
program.name('pm');
program.command('sub', 'executable description');
program.parse(['node', 'script-name', 'sub']);
@@ -76,6 +78,7 @@ describe('search for subcommand', () => {

test('when executableDir then search for local files)', () => {
const program = new commander.Command();
program._throwForMissingExecutable = () => {}; // suppress error, call mocked spawn
program.name('pm');
program.executableDir(__dirname);
program.command('sub', 'executable description');