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

fix: error handling for unexpected numeric arguments passed to cli #5263

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 70 additions & 17 deletions lib/cli/options.js
Dinika marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -10,15 +10,25 @@
const fs = require('fs');
const ansi = require('ansi-colors');
const yargsParser = require('yargs-parser');
const {types, aliases} = require('./run-option-metadata');
const {
types,
aliases,
isMochaFlag,
expectedTypeForFlag
} = require('./run-option-metadata');
const {ONE_AND_DONE_ARGS} = require('./one-and-dones');
const mocharc = require('../mocharc.json');
const {list} = require('./run-helpers');
const {loadConfig, findConfig} = require('./config');
const findUp = require('find-up');
const debug = require('debug')('mocha:cli:options');
const {isNodeFlag} = require('./node-flags');
const {createUnparsableFileError} = require('../errors');
const {
createUnparsableFileError,
createInvalidArgumentTypeError,
createUnsupportedError
} = require('../errors');
const {isNumeric} = require('../utils');

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

/**
* Throws either "UNSUPPORTED" error or "INVALID_ARG_TYPE" error for numeric positional arguments.
* @param {string[]} allArgs - Stringified args passed to mocha cli
* @param {number} numericArg - Numeric positional arg for which error must be thrown
* @param {Object} parsedResult - Result from `yargs-parser`
* @private
* @ignore
*/
const createErrorForNumericPositionalArg = (
numericArg,
allArgs,
parsedResult
) => {
// A flag for `numericArg` exists if:
// 1. A mocha flag immediately preceeded the numericArg in `allArgs` array and
// 2. `numericArg` value could not be assigned to this flag by `yargs-parser` because of incompatible datatype.
const flag = allArgs.find((arg, index) => {
const normalizedArg = arg.replace(/^--?/, '');
return (
isMochaFlag(arg) &&
allArgs[index + 1] === String(numericArg) &&
parsedResult[normalizedArg] !== String(numericArg)
);
});

if (flag) {
throw createInvalidArgumentTypeError(
`Mocha flag '${flag}' given invalid option: '${numericArg}'`,
numericArg,
expectedTypeForFlag(flag)
);
} else {
throw createUnsupportedError(
`Option ${numericArg} is unsupported by the mocha cli`
);
}
};

/**
* Wrapper around `yargs-parser` which applies our settings
* @param {string|string[]} args - Arguments to parse
@@ -104,24 +152,20 @@ const nargOpts = types.array
const parse = (args = [], defaultValues = {}, ...configObjects) => {
// save node-specific args for special handling.
// 1. when these args have a "=" they should be considered to have values
// 2. if they don't, they just boolean flags
// 2. if they don't, they are just boolean flags
// 3. to avoid explicitly defining the set of them, we tell yargs-parser they
// are ALL boolean flags.
// 4. we can then reapply the values after yargs-parser is done.
const nodeArgs = (Array.isArray(args) ? args : args.split(' ')).reduce(
(acc, arg) => {
const pair = arg.split('=');
let flag = pair[0];
if (isNodeFlag(flag, false)) {
flag = flag.replace(/^--?/, '');
return arg.includes('=')
? acc.concat([[flag, pair[1]]])
: acc.concat([[flag, true]]);
}
return acc;
},
[]
);
const allArgs = Array.isArray(args) ? args : args.split(' ');
const nodeArgs = allArgs.reduce((acc, arg) => {
const pair = arg.split('=');
let flag = pair[0];
if (isNodeFlag(flag, false)) {
flag = flag.replace(/^--?/, '');
return acc.concat([[flag, arg.includes('=') ? pair[1] : true]]);
}
return acc;
}, []);

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

const numericPositionalArg = result.argv._.find(arg => isNumeric(arg));
if (numericPositionalArg) {
createErrorForNumericPositionalArg(
numericPositionalArg,
allArgs,
result.argv
);
}

// reapply "=" arg values from above
nodeArgs.forEach(([key, value]) => {
result.argv[key] = value;
21 changes: 21 additions & 0 deletions lib/cli/run-option-metadata.js
Dinika marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -114,3 +114,24 @@ const ALL_MOCHA_FLAGS = Object.keys(TYPES).reduce((acc, key) => {
exports.isMochaFlag = flag => {
return ALL_MOCHA_FLAGS.has(flag.replace(/^--?/, ''));
};

/**
* Returns expected yarg option type for a given mocha flag.
* @param {string} flag - Flag to check (can be with or without leading dashes "--"")
* @returns {string | undefined} - If flag is a valid mocha flag, the expected type of argument for this flag is returned, otherwise undefined is returned.
* @private
*/
exports.expectedTypeForFlag = flag => {
const normalizedName = flag.replace(/^--?/, '');

// If flag is an alias, get it's full name.
const aliases = exports.aliases;
const fullFlagName =
Object.keys(aliases).find(flagName =>
aliases[flagName].includes(normalizedName)
) || normalizedName;

return Object.keys(TYPES).find(flagType =>
TYPES[flagType].includes(fullFlagName)
);
};
7 changes: 7 additions & 0 deletions lib/utils.js
Original file line number Diff line number Diff line change
@@ -689,3 +689,10 @@ exports.breakCircularDeps = inputObj => {

return _breakCircularDeps(inputObj);
};

/**
* Checks if provided input can be parsed as a JavaScript Number.
*/
exports.isNumeric = input => {
return !isNaN(parseFloat(input));
};
27 changes: 27 additions & 0 deletions test/node-unit/cli/mocha-flags.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const {
types,
expectedTypeForFlag
} = require('../../../lib/cli/run-option-metadata');

describe('mocha-flags', function () {
describe('expectedTypeForFlag()', function () {
Object.entries(types).forEach(([dataType, flags]) => {
flags.forEach(flag => {
it(`returns expected ${flag}'s type as ${dataType}`, function () {
expect(expectedTypeForFlag(flag), 'to equal', dataType);
});
});
});

it('returns undefined for node flags', function () {
expect(expectedTypeForFlag('--throw-deprecation'), 'to equal', undefined);
expect(expectedTypeForFlag('throw-deprecation'), 'to equal', undefined);
});

it('returns undefined for unsupported flags', function () {
expect(expectedTypeForFlag('--foo'), 'to equal', undefined);
});
});
});
104 changes: 104 additions & 0 deletions test/node-unit/cli/options.spec.js
Dinika marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
const sinon = require('sinon');
const rewiremock = require('rewiremock/node');
const {ONE_AND_DONE_ARGS} = require('../../../lib/cli/one-and-dones');
const {constants} = require('../../../lib/errors');

const modulePath = require.resolve('../../../lib/cli/options');
const mocharcPath = require.resolve('../../../lib/mocharc.json');
@@ -676,5 +677,108 @@ describe('options', function () {
]);
});
});

describe('"numeric arguments"', function () {
const numericArg = 123;

const unsupportedError = arg => {
return {
message: `Option ${arg} is unsupported by the mocha cli`,
code: constants.UNSUPPORTED
};
};

const invalidArgError = (flag, arg, expectedType = 'string') => {
return {
message: `Mocha flag '${flag}' given invalid option: '${arg}'`,
code: constants.INVALID_ARG_TYPE,
argument: arg,
actual: 'number',
expected: expectedType
};
};

beforeEach(function () {
readFileSync = sinon.stub();
findConfig = sinon.stub();
loadConfig = sinon.stub();
findupSync = sinon.stub();
loadOptions = proxyLoadOptions({
readFileSync,
findConfig,
loadConfig,
findupSync
});
});

it('throws UNSUPPORTED error when numeric option is passed to cli', function () {
expect(
() => loadOptions(`${numericArg}`),
'to throw',
unsupportedError(numericArg)
);
});

it('throws INVALID_ARG_TYPE error when numeric argument is passed to mocha flag that does not accept numeric value', function () {
const flag = '--delay';
expect(
() => loadOptions(`${flag} ${numericArg}`),
'to throw',
invalidArgError(flag, numericArg, 'boolean')
);
});

it('throws INVALID_ARG_TYPE error when incompatible flag does not have preceding "--"', function () {
const flag = 'delay';
expect(
() => loadOptions(`${flag} ${numericArg}`),
'to throw',
invalidArgError(flag, numericArg, 'boolean')
);
});

it('shows correct flag in error when multiple mocha flags have numeric values', function () {
const flag = '--delay';
expect(
() =>
loadOptions(
`--timeout ${numericArg} ${flag} ${numericArg} --retries ${numericArg}`
),
'to throw',
invalidArgError(flag, numericArg, 'boolean')
);
});

it('throws UNSUPPORTED error when numeric arg is passed to unsupported flag', function () {
const invalidFlag = 'foo';
expect(
() => loadOptions(`${invalidFlag} ${numericArg}`),
'to throw',
unsupportedError(numericArg)
);
});

it('does not throw error if numeric value is passed to a compatible mocha flag', function () {
expect(() => loadOptions(`--retries ${numericArg}`), 'not to throw');
});

it('does not throw error if numeric value is passed to a node options', function () {
expect(
() =>
loadOptions(
`--secure-heap-min=${numericArg} --conditions=${numericArg}`
),
'not to throw'
);
});

it('does not throw error if numeric value is passed to string flag', function () {
expect(() => loadOptions(`--grep ${numericArg}`), 'not to throw');
});

it('does not throw error if numeric value is passed to an array flag', function () {
expect(() => loadOptions(`--spec ${numericArg}`), 'not to throw');
});
});
Dinika marked this conversation as resolved.
Show resolved Hide resolved
});
});
20 changes: 20 additions & 0 deletions test/node-unit/utils.spec.js
Original file line number Diff line number Diff line change
@@ -45,5 +45,25 @@ describe('utils', function () {
);
});
});
describe('isNumeric()', function () {
Dinika marked this conversation as resolved.
Show resolved Hide resolved
it('returns true for a number type', function () {
expect(utils.isNumeric(42), 'to equal', true);
});
it('returns true for a string that can be parsed as a number', function () {
expect(utils.isNumeric('42'), 'to equal', true);
});
it('returns false for a string that cannot be parsed as a number', function () {
expect(utils.isNumeric('foo'), 'to equal', false);
});
it('returns false for empty string', function () {
expect(utils.isNumeric(''), 'to equal', false);
});
it('returns false for empty string with many whitespaces', function () {
expect(utils.isNumeric(' '), 'to equal', false);
});
it('returns true for stringified zero', function () {
expect(utils.isNumeric('0'), 'to equal', true);
});
});
});
});
Loading