Skip to content

Commit 210d658

Browse files
DinikaDinika
authored
fix: error handling for unexpected numeric arguments passed to cli (#5263)
* Fix 5028 // Numerical arguments to cli throw uncaught error This PR throws a custom error which is (hopefully) easier to understand when: i. a numerical argument is passed to mocha cli ii. numerical value is used as a value for one of the mocha flags that is not compatible with numerical values. Signed-off-by: Dinika Saxena <[email protected]> * Use type-check to throw error instead of a broad try/catch Signed-off-by: Dinika Saxena <[email protected]> * Rename numerical to numeric Signed-off-by: Dinika Saxena <[email protected]> * Find flag for mocha cli after parsing yargs * Find flag for faulty numeric args before yargs parser Signed-off-by: Dinika Saxena <[email protected]> * Add js-doc private keyword to fix netlify doc deployment * [Style] // Reduce code duplication * Add an "it" block for each flag for easier debug-ability Signed-off-by: Dinika Saxena <[email protected]> * Remove ? from flag since it cannot be undefined * Add test cases for empty string checks for isNumeric * Do not add extra leading -- in error message for flag * Throw error for numeric positional arg after yargs-parser has parsed args Signed-off-by: Dinika Saxena <[email protected]> * Revert timeout and slow as string flags so that they can accept human readable values Signed-off-by: Dinika Saxena <[email protected]> --------- Signed-off-by: Dinika Saxena <[email protected]> Signed-off-by: Dinika Saxena <[email protected]>
1 parent 6f10d12 commit 210d658

File tree

6 files changed

+249
-17
lines changed

6 files changed

+249
-17
lines changed

‎lib/cli/options.js‎

Lines changed: 70 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,25 @@
1010
const fs = require('fs');
1111
const ansi = require('ansi-colors');
1212
const yargsParser = require('yargs-parser');
13-
const {types, aliases} = require('./run-option-metadata');
13+
const {
14+
types,
15+
aliases,
16+
isMochaFlag,
17+
expectedTypeForFlag
18+
} = require('./run-option-metadata');
1419
const {ONE_AND_DONE_ARGS} = require('./one-and-dones');
1520
const mocharc = require('../mocharc.json');
1621
const {list} = require('./run-helpers');
1722
const {loadConfig, findConfig} = require('./config');
1823
const findUp = require('find-up');
1924
const debug = require('debug')('mocha:cli:options');
2025
const {isNodeFlag} = require('./node-flags');
21-
const {createUnparsableFileError} = require('../errors');
26+
const {
27+
createUnparsableFileError,
28+
createInvalidArgumentTypeError,
29+
createUnsupportedError
30+
} = require('../errors');
31+
const {isNumeric} = require('../utils');
2232

2333
/**
2434
* The `yargs-parser` namespace
@@ -93,6 +103,44 @@ const nargOpts = types.array
93103
.concat(types.string, types.number)
94104
.reduce((acc, arg) => Object.assign(acc, {[arg]: 1}), {});
95105

106+
/**
107+
* Throws either "UNSUPPORTED" error or "INVALID_ARG_TYPE" error for numeric positional arguments.
108+
* @param {string[]} allArgs - Stringified args passed to mocha cli
109+
* @param {number} numericArg - Numeric positional arg for which error must be thrown
110+
* @param {Object} parsedResult - Result from `yargs-parser`
111+
* @private
112+
* @ignore
113+
*/
114+
const createErrorForNumericPositionalArg = (
115+
numericArg,
116+
allArgs,
117+
parsedResult
118+
) => {
119+
// A flag for `numericArg` exists if:
120+
// 1. A mocha flag immediately preceeded the numericArg in `allArgs` array and
121+
// 2. `numericArg` value could not be assigned to this flag by `yargs-parser` because of incompatible datatype.
122+
const flag = allArgs.find((arg, index) => {
123+
const normalizedArg = arg.replace(/^--?/, '');
124+
return (
125+
isMochaFlag(arg) &&
126+
allArgs[index + 1] === String(numericArg) &&
127+
parsedResult[normalizedArg] !== String(numericArg)
128+
);
129+
});
130+
131+
if (flag) {
132+
throw createInvalidArgumentTypeError(
133+
`Mocha flag '${flag}' given invalid option: '${numericArg}'`,
134+
numericArg,
135+
expectedTypeForFlag(flag)
136+
);
137+
} else {
138+
throw createUnsupportedError(
139+
`Option ${numericArg} is unsupported by the mocha cli`
140+
);
141+
}
142+
};
143+
96144
/**
97145
* Wrapper around `yargs-parser` which applies our settings
98146
* @param {string|string[]} args - Arguments to parse
@@ -104,24 +152,20 @@ const nargOpts = types.array
104152
const parse = (args = [], defaultValues = {}, ...configObjects) => {
105153
// save node-specific args for special handling.
106154
// 1. when these args have a "=" they should be considered to have values
107-
// 2. if they don't, they just boolean flags
155+
// 2. if they don't, they are just boolean flags
108156
// 3. to avoid explicitly defining the set of them, we tell yargs-parser they
109157
// are ALL boolean flags.
110158
// 4. we can then reapply the values after yargs-parser is done.
111-
const nodeArgs = (Array.isArray(args) ? args : args.split(' ')).reduce(
112-
(acc, arg) => {
113-
const pair = arg.split('=');
114-
let flag = pair[0];
115-
if (isNodeFlag(flag, false)) {
116-
flag = flag.replace(/^--?/, '');
117-
return arg.includes('=')
118-
? acc.concat([[flag, pair[1]]])
119-
: acc.concat([[flag, true]]);
120-
}
121-
return acc;
122-
},
123-
[]
124-
);
159+
const allArgs = Array.isArray(args) ? args : args.split(' ');
160+
const nodeArgs = allArgs.reduce((acc, arg) => {
161+
const pair = arg.split('=');
162+
let flag = pair[0];
163+
if (isNodeFlag(flag, false)) {
164+
flag = flag.replace(/^--?/, '');
165+
return acc.concat([[flag, arg.includes('=') ? pair[1] : true]]);
166+
}
167+
return acc;
168+
}, []);
125169

126170
const result = yargsParser.detailed(args, {
127171
configuration,
@@ -140,6 +184,15 @@ const parse = (args = [], defaultValues = {}, ...configObjects) => {
140184
process.exit(1);
141185
}
142186

187+
const numericPositionalArg = result.argv._.find(arg => isNumeric(arg));
188+
if (numericPositionalArg) {
189+
createErrorForNumericPositionalArg(
190+
numericPositionalArg,
191+
allArgs,
192+
result.argv
193+
);
194+
}
195+
143196
// reapply "=" arg values from above
144197
nodeArgs.forEach(([key, value]) => {
145198
result.argv[key] = value;

‎lib/cli/run-option-metadata.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,3 +114,24 @@ const ALL_MOCHA_FLAGS = Object.keys(TYPES).reduce((acc, key) => {
114114
exports.isMochaFlag = flag => {
115115
return ALL_MOCHA_FLAGS.has(flag.replace(/^--?/, ''));
116116
};
117+
118+
/**
119+
* Returns expected yarg option type for a given mocha flag.
120+
* @param {string} flag - Flag to check (can be with or without leading dashes "--"")
121+
* @returns {string | undefined} - If flag is a valid mocha flag, the expected type of argument for this flag is returned, otherwise undefined is returned.
122+
* @private
123+
*/
124+
exports.expectedTypeForFlag = flag => {
125+
const normalizedName = flag.replace(/^--?/, '');
126+
127+
// If flag is an alias, get it's full name.
128+
const aliases = exports.aliases;
129+
const fullFlagName =
130+
Object.keys(aliases).find(flagName =>
131+
aliases[flagName].includes(normalizedName)
132+
) || normalizedName;
133+
134+
return Object.keys(TYPES).find(flagType =>
135+
TYPES[flagType].includes(fullFlagName)
136+
);
137+
};

‎lib/utils.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -689,3 +689,10 @@ exports.breakCircularDeps = inputObj => {
689689

690690
return _breakCircularDeps(inputObj);
691691
};
692+
693+
/**
694+
* Checks if provided input can be parsed as a JavaScript Number.
695+
*/
696+
exports.isNumeric = input => {
697+
return !isNaN(parseFloat(input));
698+
};

‎test/node-unit/cli/mocha-flags.spec.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
'use strict';
2+
3+
const {
4+
types,
5+
expectedTypeForFlag
6+
} = require('../../../lib/cli/run-option-metadata');
7+
8+
describe('mocha-flags', function () {
9+
describe('expectedTypeForFlag()', function () {
10+
Object.entries(types).forEach(([dataType, flags]) => {
11+
flags.forEach(flag => {
12+
it(`returns expected ${flag}'s type as ${dataType}`, function () {
13+
expect(expectedTypeForFlag(flag), 'to equal', dataType);
14+
});
15+
});
16+
});
17+
18+
it('returns undefined for node flags', function () {
19+
expect(expectedTypeForFlag('--throw-deprecation'), 'to equal', undefined);
20+
expect(expectedTypeForFlag('throw-deprecation'), 'to equal', undefined);
21+
});
22+
23+
it('returns undefined for unsupported flags', function () {
24+
expect(expectedTypeForFlag('--foo'), 'to equal', undefined);
25+
});
26+
});
27+
});

‎test/node-unit/cli/options.spec.js‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
const sinon = require('sinon');
44
const rewiremock = require('rewiremock/node');
55
const {ONE_AND_DONE_ARGS} = require('../../../lib/cli/one-and-dones');
6+
const {constants} = require('../../../lib/errors');
67

78
const modulePath = require.resolve('../../../lib/cli/options');
89
const mocharcPath = require.resolve('../../../lib/mocharc.json');
@@ -676,5 +677,108 @@ describe('options', function () {
676677
]);
677678
});
678679
});
680+
681+
describe('"numeric arguments"', function () {
682+
const numericArg = 123;
683+
684+
const unsupportedError = arg => {
685+
return {
686+
message: `Option ${arg} is unsupported by the mocha cli`,
687+
code: constants.UNSUPPORTED
688+
};
689+
};
690+
691+
const invalidArgError = (flag, arg, expectedType = 'string') => {
692+
return {
693+
message: `Mocha flag '${flag}' given invalid option: '${arg}'`,
694+
code: constants.INVALID_ARG_TYPE,
695+
argument: arg,
696+
actual: 'number',
697+
expected: expectedType
698+
};
699+
};
700+
701+
beforeEach(function () {
702+
readFileSync = sinon.stub();
703+
findConfig = sinon.stub();
704+
loadConfig = sinon.stub();
705+
findupSync = sinon.stub();
706+
loadOptions = proxyLoadOptions({
707+
readFileSync,
708+
findConfig,
709+
loadConfig,
710+
findupSync
711+
});
712+
});
713+
714+
it('throws UNSUPPORTED error when numeric option is passed to cli', function () {
715+
expect(
716+
() => loadOptions(`${numericArg}`),
717+
'to throw',
718+
unsupportedError(numericArg)
719+
);
720+
});
721+
722+
it('throws INVALID_ARG_TYPE error when numeric argument is passed to mocha flag that does not accept numeric value', function () {
723+
const flag = '--delay';
724+
expect(
725+
() => loadOptions(`${flag} ${numericArg}`),
726+
'to throw',
727+
invalidArgError(flag, numericArg, 'boolean')
728+
);
729+
});
730+
731+
it('throws INVALID_ARG_TYPE error when incompatible flag does not have preceding "--"', function () {
732+
const flag = 'delay';
733+
expect(
734+
() => loadOptions(`${flag} ${numericArg}`),
735+
'to throw',
736+
invalidArgError(flag, numericArg, 'boolean')
737+
);
738+
});
739+
740+
it('shows correct flag in error when multiple mocha flags have numeric values', function () {
741+
const flag = '--delay';
742+
expect(
743+
() =>
744+
loadOptions(
745+
`--timeout ${numericArg} ${flag} ${numericArg} --retries ${numericArg}`
746+
),
747+
'to throw',
748+
invalidArgError(flag, numericArg, 'boolean')
749+
);
750+
});
751+
752+
it('throws UNSUPPORTED error when numeric arg is passed to unsupported flag', function () {
753+
const invalidFlag = 'foo';
754+
expect(
755+
() => loadOptions(`${invalidFlag} ${numericArg}`),
756+
'to throw',
757+
unsupportedError(numericArg)
758+
);
759+
});
760+
761+
it('does not throw error if numeric value is passed to a compatible mocha flag', function () {
762+
expect(() => loadOptions(`--retries ${numericArg}`), 'not to throw');
763+
});
764+
765+
it('does not throw error if numeric value is passed to a node options', function () {
766+
expect(
767+
() =>
768+
loadOptions(
769+
`--secure-heap-min=${numericArg} --conditions=${numericArg}`
770+
),
771+
'not to throw'
772+
);
773+
});
774+
775+
it('does not throw error if numeric value is passed to string flag', function () {
776+
expect(() => loadOptions(`--grep ${numericArg}`), 'not to throw');
777+
});
778+
779+
it('does not throw error if numeric value is passed to an array flag', function () {
780+
expect(() => loadOptions(`--spec ${numericArg}`), 'not to throw');
781+
});
782+
});
679783
});
680784
});

‎test/node-unit/utils.spec.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,5 +45,25 @@ describe('utils', function () {
4545
);
4646
});
4747
});
48+
describe('isNumeric()', function () {
49+
it('returns true for a number type', function () {
50+
expect(utils.isNumeric(42), 'to equal', true);
51+
});
52+
it('returns true for a string that can be parsed as a number', function () {
53+
expect(utils.isNumeric('42'), 'to equal', true);
54+
});
55+
it('returns false for a string that cannot be parsed as a number', function () {
56+
expect(utils.isNumeric('foo'), 'to equal', false);
57+
});
58+
it('returns false for empty string', function () {
59+
expect(utils.isNumeric(''), 'to equal', false);
60+
});
61+
it('returns false for empty string with many whitespaces', function () {
62+
expect(utils.isNumeric(' '), 'to equal', false);
63+
});
64+
it('returns true for stringified zero', function () {
65+
expect(utils.isNumeric('0'), 'to equal', true);
66+
});
67+
});
4868
});
4969
});

0 commit comments

Comments
 (0)