peer review changes

- rename "not supported" error to "unsupported" error
- rename "undefined error" error to "invalid exception" error
- remove "invalid argument" factory; use "unsupported" factory
- doc fixes
- move `utils.getError` to `Runnable.getError`, since it's only used there
- remove `utils.undefinedError`; use `createInvalidExceptionError` instead.

docs/index.md

@@ -250,7 +250,7 @@ describe('User', function() {
 });
 ```
 
-To make things even easier, the `done()` callback also accepts an `Error` instance (i.e. `new Error()`), so we may use this directly:
+Alternatively, just use the `done()` callback directly (which will handle an error argument, if it exists):
 
 ```js
 describe('User', function() {
@@ -288,6 +288,7 @@ In Mocha v3.0.0 and newer, returning a `Promise` _and_ calling `done()` will res
 ```js
 const assert = require('assert');
 
+// antipattern
 it('should complete this test', function(done) {
   return new Promise(function(resolve) {
     assert.ok(true);
@@ -495,7 +496,7 @@ describe('Array', function() {
 });
 ```
 
-Previous to v3.0.0, `.only()` used string matching to decide which tests to execute. As of v3.0.0, this is no longer the case. In v3.0.0 or newer, `.only()` can be used multiple times to define a subset of tests to run:
+Previous to v3.0.0, `.only()` used string matching to decide which tests to execute; this is no longer the case. In v3.0.0 or newer, `.only()` can be used multiple times to define a subset of tests to run:
 
 ```js
 describe('Array', function() {
@@ -931,7 +932,7 @@ _Prior to_ version v4.0.0, _by default_, Mocha would force its own process to ex
 
 "Hanging" most often manifests itself if a server is still listening on a port, or a socket is still open, etc. It can also be something like a runaway `setInterval()`, or even an errant `Promise` that never fulfilled.
 
-The _default behavior_ in v4.0.0 is `--no-exit`, where previously it was `--exit`.
+The _default behavior_ in v4.0.0 (and newer) is `--no-exit`, where previously it was `--exit`.
 
 **The easiest way to "fix" the issue is to simply pass `--exit` to the Mocha process.** It _can_ be time-consuming to debug--because it's not always obvious where the problem is--but it _is_ recommended to do so.
 
@@ -1733,11 +1734,11 @@ When Mocha itself throws exception, the associated `Error` will have a `code` pr
 | ERR_MOCHA_INVALID_ARG            | argument is invalid                                          |
 | ERR_MOCHA_INVALID_ARG_TYPE       | argument of the wrong type was passed to Mocha's API         |
 | ERR_MOCHA_INVALID_ARG_VALUE      | invalid or unsupported value was passed for a given argument |
+| ERR_MOCHA_INVALID_EXCEPTION      | a falsy or otherwise underspecified exception was thrown     |
 | ERR_MOCHA_INVALID_INTERFACE      | interface specified in options not found                     |
 | ERR_MOCHA_INVALID_REPORTER       | reporter specified in options not found                      |
 | ERR_MOCHA_NO_FILES_MATCH_PATTERN | file/s of test could not be found                            |
-| ERR_MOCHA_NOT_SUPPORTED          | type of output specified was not supported                   |
-| ERR_MOCHA_UNDEFINED_ERROR        | an error was thrown but no details were specified            |
+| ERR_MOCHA_UNSUPPORTED            | requested behavior, option, or parameter is unsupported      |
 
 ## Editor Plugins
 

lib/cli/run.js

@@ -10,7 +10,7 @@
 const Mocha = require('../mocha');
 const ansi = require('ansi-colors');
 const {
-  createInvalidArgumentError,
+  createUnsupportedError,
   createInvalidArgumentValueError,
   createMissingArgumentError
 } = require('../errors');
@@ -269,10 +269,9 @@ exports.builder = yargs =>
       }
 
       if (argv.compilers) {
-        throw createInvalidArgumentError(
+        throw createUnsupportedError(
           `--compilers is DEPRECATED and no longer supported.
-          See ${ansi.cyan('https://git.io/vdcSr')} for migration information.`,
-          'compilers'
+          See ${ansi.cyan('https://git.io/vdcSr')} for migration information.`
         );
       }
 

lib/errors.js

@@ -7,7 +7,7 @@
  */
 
 /**
- * Creates an error object used when no files to be tested could be found using specified pattern.
+ * Creates an error object to be thrown when no files to be tested could be found using specified pattern.
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -22,7 +22,7 @@ function createNoFilesMatchPatternError(message, pattern) {
 }
 
 /**
- * Creates an error object used when the reporter specified in the options was not found.
+ * Creates an error object to be thrown when the reporter specified in the options was not found.
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -37,7 +37,7 @@ function createInvalidReporterError(message, reporter) {
 }
 
 /**
- * Creates an error object used when the interface specified in the options was not found.
+ * Creates an error object to be thrown when the interface specified in the options was not found.
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -52,20 +52,20 @@ function createInvalidInterfaceError(message, ui) {
 }
 
 /**
- * Creates an error object used when the type of output specified was not supported.
+ * Creates an error object to be thrown when a behavior, option, or parameter is unsupported.
  *
  * @public
  * @param {string} message - Error message to be displayed.
  * @returns {Error} instance detailing the error condition
  */
-function createNotSupportedError(message) {
+function createUnsupportedError(message) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_NOT_SUPPORTED';
+  err.code = 'ERR_MOCHA_UNSUPPORTED';
   return err;
 }
 
 /**
- * Creates an error object used when an argument is missing.
+ * Creates an error object to be thrown when an argument is missing.
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -78,7 +78,7 @@ function createMissingArgumentError(message, argument, expected) {
 }
 
 /**
- * Creates an error object used when an argument did not use the supported type
+ * Creates an error object to be thrown when an argument did not use the supported type
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -96,7 +96,7 @@ function createInvalidArgumentTypeError(message, argument, expected) {
 }
 
 /**
- * Creates an error object used when an argument did not use the supported value
+ * Creates an error object to be thrown when an argument did not use the supported value
  *
  * @public
  * @param {string} message - Error message to be displayed.
@@ -115,43 +115,27 @@ function createInvalidArgumentValueError(message, argument, value, reason) {
 }
 
 /**
- * Creates an error object used when an argument itself is invalid
- *
- * @public
- * @param {string} message - Error message to be displayed.
- * @param {string} argument - Argument name.
- * @param {string} [reason] - Why argument is invalid.
- * @returns {Error} instance detailing the error condition
- */
-function createInvalidArgumentError(message, argument, reason) {
-  var err = new Error(message);
-  err.code = 'ERR_MOCHA_INVALID_ARG';
-  err.argument = argument;
-  err.reason = typeof reason !== 'undefined' ? reason : 'is invalid';
-  return err;
-}
-
-/**
- * Creates an error object used when an error was thrown but no details were specified.
+ * Creates an error object to be thrown when an exception was caught, but the `Error` is falsy or undefined.
  *
  * @public
  * @param {string} message - Error message to be displayed.
  * @returns {Error} instance detailing the error condition
  */
-function createUndefinedError(message) {
+function createInvalidExceptionError(message, value) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_UNDEFINED_ERROR';
+  err.code = 'ERR_MOCHA_INVALID_EXCEPTION';
+  err.valueType = typeof value;
+  err.value = value;
   return err;
 }
 
 module.exports = {
-  createInvalidArgumentError: createInvalidArgumentError,
   createInvalidArgumentTypeError: createInvalidArgumentTypeError,
   createInvalidArgumentValueError: createInvalidArgumentValueError,
+  createInvalidExceptionError: createInvalidExceptionError,
   createInvalidInterfaceError: createInvalidInterfaceError,
   createInvalidReporterError: createInvalidReporterError,
   createMissingArgumentError: createMissingArgumentError,
   createNoFilesMatchPatternError: createNoFilesMatchPatternError,
-  createNotSupportedError: createNotSupportedError,
-  createUndefinedError: createUndefinedError
+  createUnsupportedError: createUnsupportedError
 };

lib/reporters/xunit.js

@@ -12,6 +12,7 @@ var fs = require('fs');
 var mkdirp = require('mkdirp');
 var path = require('path');
 var errors = require('../errors');
+var createUnsupportedError = errors.createUnsupportedError;
 var constants = require('../runner').constants;
 var EVENT_TEST_PASS = constants.EVENT_TEST_PASS;
 var EVENT_TEST_FAIL = constants.EVENT_TEST_FAIL;
@@ -20,7 +21,6 @@ var EVENT_TEST_PENDING = constants.EVENT_TEST_PENDING;
 var STATE_FAILED = require('../runnable').constants.STATE_FAILED;
 var inherits = utils.inherits;
 var escape = utils.escape;
-var createNotSupportedError = errors.createNotSupportedError;
 
 /**
  * Save timer references to avoid Sinon interfering (see GH-237).
@@ -58,7 +58,7 @@ function XUnit(runner, options) {
   if (options && options.reporterOptions) {
     if (options.reporterOptions.output) {
       if (!fs.createWriteStream) {
-        throw createNotSupportedError('file output not supported in browser');
+        throw createUnsupportedError('file output not supported in browser');
       }
 
       mkdirp.sync(path.dirname(options.reporterOptions.output));

lib/runnable.js

@@ -5,6 +5,8 @@ var Pending = require('./pending');
 var debug = require('debug')('mocha:runnable');
 var milliseconds = require('ms');
 var utils = require('./utils');
+var createInvalidExceptionError = require('./errors')
+  .createInvalidExceptionError;
 
 /**
  * Save timer references to avoid Sinon interfering (see GH-237).
@@ -355,7 +357,7 @@ Runnable.prototype.run = function(fn) {
       callFnAsync(this.fn);
     } catch (err) {
       emitted = true;
-      done(utils.getError(err));
+      done(Runnable.getError(err));
     }
     return;
   }
@@ -378,7 +380,7 @@ Runnable.prototype.run = function(fn) {
     }
   } catch (err) {
     emitted = true;
-    done(utils.getError(err));
+    done(Runnable.getError(err));
   }
 
   function callFn(fn) {
@@ -462,7 +464,8 @@ var constants = utils.defineConstants(
    * @static
    * @alias constants
    * @enum {string}
-   */ {
+   */
+  {
     /**
      * Value of `state` prop when a `Runnable` has failed
      */
@@ -474,4 +477,14 @@ var constants = utils.defineConstants(
   }
 );
 
+Runnable.getError = function(err) {
+  return (
+    err ||
+    createInvalidExceptionError(
+      'Runnable failed with falsy or undefined exception. Please throw an Error instead.',
+      err
+    )
+  );
+};
+
 Runnable.constants = constants;

lib/runner.js

@@ -17,7 +17,8 @@ var STATE_PASSED = Runnable.constants.STATE_PASSED;
 var stackFilter = utils.stackTraceFilter();
 var stringify = utils.stringify;
 var type = utils.type;
-var undefinedError = utils.undefinedError;
+var createInvalidExceptionError = require('./errors')
+  .createInvalidExceptionError;
 
 /**
  * Non-enumerable globals.
@@ -782,7 +783,10 @@ Runner.prototype.uncaught = function(err) {
     );
   } else {
     debug('uncaught undefined exception');
-    err = undefinedError();
+    err = createInvalidExceptionError(
+      'Caught falsy/undefined exception which would otherwise be uncaught. No stack trace found; try a debugger',
+      err
+    );
   }
 
   if (!isError(err)) {

lib/utils.js

@@ -18,7 +18,6 @@ var he = require('he');
 var errors = require('./errors');
 var createNoFilesMatchPatternError = errors.createNoFilesMatchPatternError;
 var createMissingArgumentError = errors.createMissingArgumentError;
-var createUndefinedError = errors.createUndefinedError;
 
 /**
  * Ignored directories.
@@ -580,29 +579,6 @@ exports.lookupFiles = function lookupFiles(filepath, extensions, recursive) {
   return files;
 };
 
-/**
- * Generate an undefined error with a message warning the user.
- *
- * @return {Error}
- */
-
-exports.undefinedError = function() {
-  return createUndefinedError(
-    'Caught undefined error, did you throw without specifying what?'
-  );
-};
-
-/**
- * Generate an undefined error if `err` is not defined.
- *
- * @param {Error} err
- * @return {Error}
- */
-
-exports.getError = function(err) {
-  return err || exports.undefinedError();
-};
-
 /**
  * process.emitWarning or a polyfill
  * @see https://nodejs.org/api/process.html#process_process_emitwarning_warning_options

test/reporters/helpers.js

@@ -1,7 +1,7 @@
 'use strict';
 
 var errors = require('../../lib/errors');
-var createNotSupportedError = errors.createNotSupportedError;
+var createUnsupportedError = errors.createUnsupportedError;
 /*
   This function prevents the constant use of creating a runnerEvent.
   runStr is the argument that defines the runnerEvent.
@@ -118,7 +118,7 @@ function createRunnerFunction(runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
         }
       };
     default:
-      throw createNotSupportedError(
+      throw createUnsupportedError(
         'This function does not support the runner string specified.'
       );
   }

test/unit/runnable.spec.js

@@ -1,7 +1,6 @@
 'use strict';
 
 var Mocha = require('../../lib/mocha');
-var utils = Mocha.utils;
 var Runnable = Mocha.Runnable;
 var Suite = Mocha.Suite;
 var sinon = require('sinon');
@@ -419,7 +418,7 @@ describe('Runnable(title, fn)', function() {
           });
 
           runnable.run(function(err) {
-            expect(err.message, 'to be', utils.undefinedError().message);
+            expect(err.message, 'to be', Runnable.getError().message);
             done();
           });
         });
@@ -713,4 +712,16 @@ describe('Runnable(title, fn)', function() {
       }, 20);
     });
   });
+
+  describe('static method', function() {
+    describe('getError', function() {
+      it('should return identity if parameter is truthy', function() {
+        expect(Runnable.getError('foo'), 'to be', 'foo');
+      });
+
+      it('should return an Error if parameter is falsy', function() {
+        expect(Runnable.getError(null), 'to be an', Error);
+      });
+    });
+  });
 });