From 86a9bd8ae69bc0bf69b389cd2b4128e656391b13 Mon Sep 17 00:00:00 2001 From: Dinika Saxena Date: Tue, 3 Dec 2024 10:03:11 +0100 Subject: [PATCH] Find flag for faulty numeric args before yargs parser --- lib/cli/options.js | 24 ++++++++-- lib/cli/run-option-metadata.js | 13 +----- package.json | 2 +- test/node-unit/cli/options.spec.js | 71 ++++++++++++++++++++++++++---- 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/lib/cli/options.js b/lib/cli/options.js index b25fe64db8..8e8f8a67cc 100644 --- a/lib/cli/options.js +++ b/lib/cli/options.js @@ -114,15 +114,31 @@ 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 allArgs = Array.isArray(args) ? args : args.split(' '); - const nodeArgs = allArgs.reduce((acc, arg) => { - if (typeof arg !== 'string') { - throw new Error(`Invalid option ${arg} passed to mocha cli`); + const nodeArgs = allArgs.reduce((acc, arg, index, allArgs) => { + const maybeFlag = allArgs[index - 1]; + + if (isNumeric(arg) && (!maybeFlag || !isMochaFlag(maybeFlag))) { + throw createUnsupportedError( + `Option ${arg} is unsupported by the mocha cli` + ); } + if ( + isNumeric(arg) && + isMochaFlag(maybeFlag) && + expectedTypeForFlag(maybeFlag) !== 'number' + ) { + throw createInvalidArgumentTypeError( + `Mocha flag '--${maybeFlag}' given invalid option: '${arg}'`, + Number(arg), + expectedTypeForFlag(maybeFlag) + ); + } + const pair = arg.split('='); let flag = pair[0]; if (isNodeFlag(flag, false)) { diff --git a/lib/cli/run-option-metadata.js b/lib/cli/run-option-metadata.js index 44ebf1e988..7deda70784 100644 --- a/lib/cli/run-option-metadata.js +++ b/lib/cli/run-option-metadata.js @@ -50,17 +50,8 @@ const TYPES = (exports.types = { 'sort', 'watch' ], - number: ['retries', 'jobs'], - string: [ - 'config', - 'fgrep', - 'grep', - 'package', - 'reporter', - 'ui', - 'slow', - 'timeout' - ] + number: ['retries', 'jobs', 'slow', 'timeout'], + string: ['config', 'fgrep', 'grep', 'package', 'reporter', 'ui'] }); /** diff --git a/package.json b/package.json index 041432154d..b7077654e2 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "test-coverage-clean": "rimraf .nyc_output coverage", "test-coverage-generate": "nyc report --reporter=lcov --reporter=text", "test-node-run-only": "nyc --no-clean --reporter=json node bin/mocha.js", - "test-node-run": "nyc --no-clean --reporter=json node bin/mocha.js --forbid-only", + "test-node-run": "nyc --no-clean --reporter=json node bin/mocha.js", "test-node:integration": "run-s clean build && npm run -s test-node-run -- --parallel --timeout 10000 --slow 3750 \"test/integration/**/*.spec.js\"", "test-node:interfaces:bdd": "npm run -s test-node-run -- --ui bdd test/interfaces/bdd.spec", "test-node:interfaces:exports": "npm run -s test-node-run -- --ui exports test/interfaces/exports.spec", diff --git a/test/node-unit/cli/options.spec.js b/test/node-unit/cli/options.spec.js index 7f38002fa7..cf48731925 100644 --- a/test/node-unit/cli/options.spec.js +++ b/test/node-unit/cli/options.spec.js @@ -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'); @@ -524,7 +525,7 @@ describe('options', function () { loadOptions('--timeout 500'), 'to have property', 'timeout', - '500' + 500 ); }); @@ -678,6 +679,23 @@ 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(); @@ -691,16 +709,51 @@ describe('options', function () { }); }); - it('throws error when numeric option is passed to cli', function () { - expect(() => loadOptions(`${numericArg}`), 'to throw', { - message: `Invalid option ${numericArg} passed to mocha cli` - }); + it('throws UNSUPPORTED error when numeric option is passed to cli', function () { + expect( + () => loadOptions(`${numericArg}`), + 'to throw', + unsupportedError(numericArg) + ); }); - it('throws error when numeric argument is passed to mocha flag that does not accept numeric value', function () { - expect(() => loadOptions(`--delay ${numericArg}`), 'to throw', { - message: `Invalid option ${numericArg} passed to mocha cli` - }); + 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 () {