fix --inspect and its ilk; closes #3681 (#3699)

This seems to make `--inspect` work properly with or without a parameter (`--inspect=0.0.0.0:12345`).  The `=` is required, as it's required by `node`.

In v6 of Node, `--debug --port=12345` should also work as expected.

Due to ignorance, skip Windows for now

.mocharc.yml

@@ -4,4 +4,4 @@ global:
   - okGlobalA,okGlobalB
   - okGlobalC
   - callback*
-timeout: 200
+timeout: 300

bin/mocha

@@ -12,10 +12,15 @@
 const {deprecate, warn} = require('../lib/utils');
 const {spawn} = require('child_process');
 const {loadOptions} = require('../lib/cli/options');
-const {isNodeFlag, impliesNoTimeouts} = require('../lib/cli/node-flags');
+const {
+  unparseNodeFlags,
+  isNodeFlag,
+  impliesNoTimeouts
+} = require('../lib/cli/node-flags');
 const unparse = require('yargs-unparser');
-const debug = require('debug')('mocha:cli');
+const debug = require('debug')('mocha:cli:mocha');
 const {aliases} = require('../lib/cli/run-option-metadata');
+const nodeEnv = require('node-environment-flags');
 
 const mochaPath = require.resolve('./_mocha');
 const mochaArgs = {};
@@ -32,7 +37,7 @@ debug('loaded opts', opts);
 const disableTimeouts = value => {
   if (impliesNoTimeouts(value)) {
     debug(`option "${value}" disabled timeouts`);
-    mochaArgs.timeout = false;
+    mochaArgs.timeout = 0;
     delete mochaArgs.timeouts;
     delete mochaArgs.t;
   }
@@ -45,13 +50,12 @@ const disableTimeouts = value => {
  * @ignore
  */
 const trimV8Option = value =>
-  value !== 'v8-options' && /^v8-/.test(value) ? value.slice(2) : value;
+  value !== 'v8-options' && /^v8-/.test(value) ? value.slice(3) : value;
 
 // sort options into "node" and "mocha" buckets
 Object.keys(opts).forEach(opt => {
-  opt = trimV8Option(opt);
   if (isNodeFlag(opt)) {
-    nodeArgs[opt] = opts[opt];
+    nodeArgs[trimV8Option(opt)] = opts[opt];
     disableTimeouts(opt);
   } else {
     mochaArgs[opt] = opts[opt];
@@ -90,20 +94,19 @@ if (/^(debug|inspect)$/.test(mochaArgs._[0])) {
 }
 
 // allow --debug to invoke --inspect on Node.js v8 or newer.
-// these show up in childOpts because they are not recognized as valid node flags in this version of node.
 ['debug', 'debug-brk']
-  .filter(opt => opt in mochaArgs)
+  .filter(opt => opt in nodeArgs && !nodeEnv.has(opt))
   .forEach(opt => {
     const newOpt = opt === 'debug' ? 'inspect' : 'inspect-brk';
     warn(
       `"--${opt}" is not available in Node.js ${
         process.version
       }; use "--${newOpt}" instead.`
     );
-    nodeArgs[newOpt] = mochaArgs[opt];
+    nodeArgs[newOpt] = nodeArgs[opt];
     mochaArgs.timeout = false;
     debug(`--${opt} -> ${newOpt}`);
-    delete mochaArgs[opt];
+    delete nodeArgs[opt];
   });
 
 // historical
@@ -115,8 +118,10 @@ if (nodeArgs.gc) {
   delete nodeArgs.gc;
 }
 
+debug('final node args', nodeArgs);
+
 const args = [].concat(
-  unparse(nodeArgs),
+  unparseNodeFlags(nodeArgs),
   mochaPath,
   unparse(mochaArgs, {alias: aliases})
 );

lib/cli/node-flags.js

@@ -7,6 +7,7 @@
  */
 
 const nodeFlags = require('node-environment-flags');
+const unparse = require('yargs-unparser');
 
 /**
  * These flags are considered "debug" flags.
@@ -34,6 +35,7 @@ const debugFlags = new Set(['debug', 'debug-brk', 'inspect', 'inspect-brk']);
 exports.isNodeFlag = flag =>
   !/^(?:require|r)$/.test(flag) &&
   (nodeFlags.has(flag) ||
+    debugFlags.has(flag) ||
     /(?:preserve-symlinks(?:-main)?|harmony(?:[_-]|$)|(?:trace[_-].+$)|gc(?:[_-]global)?$|es[_-]staging$|use[_-]strict$|v8[_-](?!options).+?$)/.test(
       flag
     ));
@@ -46,3 +48,22 @@ exports.isNodeFlag = flag =>
  * @private
  */
 exports.impliesNoTimeouts = flag => debugFlags.has(flag);
+
+/**
+ * All non-strictly-boolean arguments to node--those with values--must specify those values using `=`, e.g., `--inspect=0.0.0.0`.
+ * Unparse these arguments using `yargs-unparser` (which would result in `--inspect 0.0.0.0`), then supply `=` where we have values.
+ * There's probably an easier or more robust way to do this; fixes welcome
+ * @param {Object} opts - Arguments object
+ * @returns {string[]} Unparsed arguments using `=` to specify values
+ * @private
+ */
+exports.unparseNodeFlags = opts => {
+  var args = unparse(opts);
+  return args.length
+    ? args
+        .join(' ')
+        .split(/\b/)
+        .map(arg => (arg === ' ' ? '=' : arg))
+        .join('')
+    : [];
+};

lib/cli/options.js

@@ -18,6 +18,7 @@ const findup = require('findup-sync');
 const {deprecate} = require('../utils');
 const debug = require('debug')('mocha:cli:options');
 const {createMissingArgumentError} = require('../errors');
+const {isNodeFlag} = require('./node-flags');
 
 /**
  * The `yargs-parser` namespace
@@ -75,22 +76,46 @@ const nargOpts = types.array
  * @ignore
  */
 const parse = (args = [], ...configObjects) => {
-  const result = yargsParser.detailed(
-    args,
-    Object.assign(
-      {
-        configuration,
-        configObjects,
-        coerce: coerceOpts,
-        narg: nargOpts,
-        alias: aliases
-      },
-      types
-    )
+  // 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
+  // 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('=');
+      const flag = pair[0].replace(/^--?/, '');
+      if (isNodeFlag(flag)) {
+        return arg.includes('=')
+          ? acc.concat([[flag, pair[1]]])
+          : acc.concat([[flag, true]]);
+      }
+      return acc;
+    },
+    []
   );
+
+  const result = yargsParser.detailed(args, {
+    configuration,
+    configObjects,
+    coerce: coerceOpts,
+    narg: nargOpts,
+    alias: aliases,
+    string: types.string,
+    array: types.array,
+    number: types.number,
+    boolean: types.boolean.concat(nodeArgs.map(pair => pair[0]))
+  });
   if (result.error) {
     throw createMissingArgumentError(result.error.message);
   }
+
+  // reapply "=" arg values from above
+  nodeArgs.forEach(([key, value]) => {
+    result.argv[key] = value;
+  });
+
   return result.argv;
 };
 

package.json

@@ -498,7 +498,7 @@
     "minimatch": "3.0.4",
     "mkdirp": "0.5.1",
     "ms": "2.1.1",
-    "node-environment-flags": "1.0.2",
+    "node-environment-flags": "1.0.4",
     "object.assign": "4.1.0",
     "strip-json-comments": "2.0.1",
     "supports-color": "6.0.0",

test/assertions.js

@@ -9,7 +9,7 @@ exports.mixinMochaAssertions = function(expect) {
         return (
           Object.prototype.toString.call(v) === '[object Object]' &&
           typeof v.output === 'string' &&
-          typeof v.code === 'number' &&
+          'code' in v && // may be null
           Array.isArray(v.args)
         );
       }
@@ -59,9 +59,9 @@ exports.mixinMochaAssertions = function(expect) {
       }
     )
     .addAssertion(
-      '<RawResult|RawRunResult|JSONRunResult> [not] to have [completed with] [exit] code <number>',
+      '<RawRunResult|JSONRunResult> [not] to have completed with [exit] code <number>',
       function(expect, result, code) {
-        expect(result, '[not] to have property', 'code', code);
+        expect(result.code, '[not] to be', code);
       }
     )
     .addAssertion(
@@ -295,5 +295,11 @@ exports.mixinMochaAssertions = function(expect) {
       function(expect, result, output) {
         expect(result.output, '[not] to satisfy', output);
       }
+    )
+    .addAssertion(
+      '<RawRunResult|JSONRunResult> to have [exit] code <number>',
+      function(expect, result, code) {
+        expect(result.code, 'to be', code);
+      }
     );
 };

test/integration/helpers.js

@@ -81,10 +81,10 @@ module.exports = {
     var path;
 
     path = resolveFixturePath(fixturePath);
-    args = args || [];
+    args = (args || []).concat('--reporter', 'json', path);
 
     return invokeMocha(
-      args.concat(['--reporter', 'json', path]),
+      args,
       function(err, res) {
         if (err) return fn(err);
 
@@ -95,8 +95,8 @@ module.exports = {
           fn(
             new Error(
               format(
-                'Failed to parse JSON reporter output.\nArgs: %O\nResult:\n\n%O',
-                args,
+                'Failed to parse JSON reporter output. Error:\n%O\nResponse:\n%O',
+                err,
                 res
               )
             )

test/integration/options/debug.spec.js

@@ -14,19 +14,117 @@ describe('--debug', function() {
 
     it('should invoke --inspect', function(done) {
       invokeMocha(
-        ['--debug', '--file', DEFAULT_FIXTURE],
+        ['--debug', DEFAULT_FIXTURE],
         function(err, res) {
           if (err) {
             return done(err);
           }
-          expect(res, 'to have passed').and(
+          expect(res, 'to contain output', /Debugger listening/i);
+          done();
+        },
+        'pipe'
+      );
+    });
+
+    it('should invoke --inspect-brk', function(done) {
+      var proc = invokeMocha(
+        ['--debug-brk', DEFAULT_FIXTURE],
+        function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res, 'to contain output', /Debugger listening/i);
+          done();
+        },
+        'pipe'
+      );
+
+      // debugger must be manually killed
+      setTimeout(function() {
+        process.kill(proc.pid, 'SIGINT');
+      }, 2000);
+    });
+
+    it('should respect custom host/port', function(done) {
+      invokeMocha(
+        ['--debug=127.0.0.1:9229', DEFAULT_FIXTURE],
+        function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(
+            res,
+            'to contain output',
+            /Debugger listening on .*127.0.0.1:9229/i
+          );
+          done();
+        },
+        'pipe'
+      );
+    });
+
+    it('should warn about incorrect usage for version', function(done) {
+      invokeMocha(
+        ['--debug=127.0.0.1:9229', DEFAULT_FIXTURE],
+        function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res, 'to contain output', /"--debug" is not available/i);
+          done();
+        },
+        'pipe'
+      );
+    });
+  });
+
+  describe('Node.js v6', function() {
+    // note that v6.3.0 and newer supports --inspect but still supports --debug.
+    before(function() {
+      if (process.version.substring(0, 2) !== 'v6') {
+        this.skip();
+      }
+    });
+
+    it('should start debugger', function(done) {
+      var proc = invokeMocha(
+        ['--debug', DEFAULT_FIXTURE],
+        function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res, 'to contain output', /Debugger listening/i);
+          done();
+        },
+        'pipe'
+      );
+
+      // debugger must be manually killed
+      setTimeout(function() {
+        process.kill(proc.pid, 'SIGINT');
+      }, 2000);
+    });
+
+    it('should respect custom host/port', function(done) {
+      var proc = invokeMocha(
+        ['--debug=127.0.0.1:9229', DEFAULT_FIXTURE],
+        function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(
+            res,
             'to contain output',
-            /Debugger listening/i
+            /Debugger listening on .*127.0.0.1:9229/i
           );
           done();
         },
-        {stdio: 'pipe'}
+        'pipe'
       );
+
+      setTimeout(function() {
+        process.kill(proc.pid, 'SIGINT');
+      }, 2000);
     });
   });
 });