Find flag for faulty numeric args before yargs parser

Signed-off-by: Dinika Saxena <[email protected]>

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)) {
@@ -156,30 +172,6 @@ const parse = (args = [], defaultValues = {}, ...configObjects) => {
     result.argv[key] = value;
   });
 
-  const numericPositionalArgs = result.argv._.filter(arg => isNumeric(arg));
-  numericPositionalArgs.forEach(numericArg => {
-    const flag = allArgs
-      .map(arg => arg.replace(/^--?/, ''))
-      .find((arg, index) => {
-        return (
-          isMochaFlag(arg) &&
-          args[index + 1] === String(numericArg) &&
-          String(result.argv[arg]) !== String(numericArg)
-        );
-      });
-    if (flag) {
-      throw createInvalidArgumentTypeError(
-        `Flag ${flag} has invalid arg ${numericArg}`,
-        numericArg,
-        expectedTypeForFlag(flag)
-      );
-    } else {
-      throw createUnsupportedError(
-        `Invalid option ${numericArg} passed to mocha cli`
-      );
-    }
-  });
-
   return result.argv;
 };
 
@@ -227,7 +219,8 @@ const loadPkgRc = (args = {}) => {
           filepath
         );
       } else {
-        debug('failed to read default package.json at %s; ignoring', filepath);
+        debug('failed to read default package.json at %s; ignoring',
+              filepath);
         return result;
       }
     }

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']
 });
 
 /**

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 () {
+    it('returns expected type for all mocha flags', function () {
+      Object.entries(types).forEach(([dataType, flags]) => {
+        flags.forEach(flag => {
+          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');
@@ -204,7 +205,9 @@ describe('options', function () {
             const filepath = '/some/package.json';
             readFileSync = sinon.stub();
             // package.json
-            readFileSync.onFirstCall().returns('{definitely-invalid');
+            readFileSync
+              .onFirstCall()
+              .returns('{definitely-invalid');
             findConfig = sinon.stub().returns('/some/.mocharc.json');
             loadConfig = sinon.stub().returns({});
             findupSync = sinon.stub().returns(filepath);
@@ -222,7 +225,7 @@ describe('options', function () {
                 loadOptions();
               },
               'to throw',
-              /SyntaxError/
+                /SyntaxError/,
             );
           });
         });
@@ -292,7 +295,7 @@ describe('options', function () {
 
             result = loadOptions('--no-diff --no-config');
           });
-
+          isMocha
           it('should return parsed args, default config and package.json', function () {
             expect(
               result,
@@ -524,7 +527,7 @@ describe('options', function () {
           loadOptions('--timeout 500'),
           'to have property',
           'timeout',
-          '500'
+          500
         );
       });
 
@@ -678,6 +681,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 +711,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 () {

test/node-unit/utils.spec.js

@@ -45,5 +45,16 @@ 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);
+      });
+    });
   });
 });