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 <dinika@greyllama.cc>

* Use type-check to throw error instead of a broad try/catch

Signed-off-by: Dinika Saxena <dinikasaxenas@gmail.com>

* Rename numerical to numeric

Signed-off-by: Dinika Saxena <dinikasaxenas@gmail.com>

* Find flag for mocha cli after parsing yargs

* Find flag for faulty numeric args before yargs parser

Signed-off-by: Dinika Saxena <dinikasaxenas@gmail.com>

* 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 <dinikasaxenas@gmail.com>

* 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 <dinikasaxenas@gmail.com>

* Revert timeout and slow as string flags so that they can accept human readable values

Signed-off-by: Dinika Saxena <dinikasaxenas@gmail.com>

---------

Signed-off-by: Dinika Saxena <dinika@greyllama.cc>
Signed-off-by: Dinika Saxena <dinikasaxenas@gmail.com>

Author: Dinika

lib/cli/options.js

@@ -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;

lib/cli/run-option-metadata.js

@@ -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)
+  );
+};

lib/utils.js

@@ -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));
+};

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

@@ -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);
+    });
+  });
+});

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');
@@ -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');
+      });
+    });
   });
 });

test/node-unit/utils.spec.js

@@ -45,5 +45,25 @@ describe('utils', function () {
         );
       });
     });
+    describe('isNumeric()', function () {
+      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);
+      });
+    });
   });
 });