multiple async done() calls result in failure; closes #4151

- added a method in `errors` module to create a "multiple done" err - modernize `multiple-done.spec.js` - refactor errors into constants in `errors` module - remove `Runner#started` prop; replace with `Runner#state` prop + constants - add a catchall `createFatalError()` function to `errors` module; this is called when a test fails twice by other means (unsure what those means are yet)
mochajs · Jan 14, 2020 · f5c142b · f5c142b
1 parent 0e1ccbb
commit f5c142b
Show file tree

Hide file tree

Showing 8 changed files with 277 additions and 102 deletions.
diff --git a/lib/errors.js b/lib/errors.js
@@ -1,10 +1,24 @@
 'use strict';
+
 /**
+ * Factory functions to create throwable error objects
  * @module Errors
  */
+
 /**
- * Factory functions to create throwable error objects
+ * Error constants
  */
+var constants = {
+  ERR_MOCHA_FATAL: 'ERR_MOCHA_FATAL',
+  ERR_MOCHA_INVALID_ARG_TYPE: 'ERR_MOCHA_INVALID_ARG_TYPE',
+  ERR_MOCHA_INVALID_ARG_VALUE: 'ERR_MOCHA_INVALID_ARG_VALUE',
+  ERR_MOCHA_INVALID_EXCEPTION: 'ERR_MOCHA_INVALID_EXCEPTION',
+  ERR_MOCHA_INVALID_INTERFACE: 'ERR_MOCHA_INVALID_INTERFACE',
+  ERR_MOCHA_INVALID_REPORTER: 'ERR_MOCHA_INVALID_REPORTER',
+  ERR_MOCHA_MULTIPLE_DONE: 'ERR_MOCHA_MULTIPLE_DONE',
+  ERR_MOCHA_NO_FILES_MATCH_PATTERN: 'ERR_MOCHA_NO_FILES_MATCH_PATTERN',
+  ERR_MOCHA_UNSUPPORTED: 'ERR_MOCHA_UNSUPPORTED'
+};
 
 /**
  * Creates an error object to be thrown when no files to be tested could be found using specified pattern.
@@ -16,7 +30,7 @@
  */
 function createNoFilesMatchPatternError(message, pattern) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_NO_FILES_MATCH_PATTERN';
+  err.code = constants.ERR_MOCHA_NO_FILES_MATCH_PATTERN;
   err.pattern = pattern;
   return err;
 }
@@ -31,7 +45,7 @@ function createNoFilesMatchPatternError(message, pattern) {
  */
 function createInvalidReporterError(message, reporter) {
   var err = new TypeError(message);
-  err.code = 'ERR_MOCHA_INVALID_REPORTER';
+  err.code = constants.ERR_MOCHA_INVALID_REPORTER;
   err.reporter = reporter;
   return err;
 }
@@ -46,7 +60,7 @@ function createInvalidReporterError(message, reporter) {
  */
 function createInvalidInterfaceError(message, ui) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_INVALID_INTERFACE';
+  err.code = constants.ERR_MOCHA_INVALID_INTERFACE;
   err.interface = ui;
   return err;
 }
@@ -60,7 +74,7 @@ function createInvalidInterfaceError(message, ui) {
  */
 function createUnsupportedError(message) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_UNSUPPORTED';
+  err.code = constants.ERR_MOCHA_UNSUPPORTED;
   return err;
 }
 
@@ -88,7 +102,7 @@ function createMissingArgumentError(message, argument, expected) {
  */
 function createInvalidArgumentTypeError(message, argument, expected) {
   var err = new TypeError(message);
-  err.code = 'ERR_MOCHA_INVALID_ARG_TYPE';
+  err.code = constants.ERR_MOCHA_INVALID_ARG_TYPE;
   err.argument = argument;
   err.expected = expected;
   err.actual = typeof argument;
@@ -107,7 +121,7 @@ function createInvalidArgumentTypeError(message, argument, expected) {
  */
 function createInvalidArgumentValueError(message, argument, value, reason) {
   var err = new TypeError(message);
-  err.code = 'ERR_MOCHA_INVALID_ARG_VALUE';
+  err.code = constants.ERR_MOCHA_INVALID_ARG_VALUE;
   err.argument = argument;
   err.value = value;
   err.reason = typeof reason !== 'undefined' ? reason : 'is invalid';
@@ -123,12 +137,57 @@ function createInvalidArgumentValueError(message, argument, value, reason) {
  */
 function createInvalidExceptionError(message, value) {
   var err = new Error(message);
-  err.code = 'ERR_MOCHA_INVALID_EXCEPTION';
+  err.code = constants.ERR_MOCHA_INVALID_EXCEPTION;
+  err.valueType = typeof value;
+  err.value = value;
+  return err;
+}
+
+/**
+ * Creates an error object to be thrown when an unrecoverable error occurs.
+ *
+ * @public
+ * @param {string} message - Error message to be displayed.
+ * @returns {Error} instance detailing the error condition
+ */
+function createFatalError(message, value) {
+  var err = new Error(message);
+  err.code = constants.ERR_MOCHA_FATAL;
   err.valueType = typeof value;
   err.value = value;
   return err;
 }
 
+/**
+ * Creates an error object to be thrown when done() is called multiple times in a test
+ *
+ * @public
+ * @param {string} message - Error message to be displayed.
+ * @param {Runnable} runnable - Original runnable
+ * @param {Error} [originalErr] - Original error, if any
+ * @returns {Error} instance detailing the error condition
+ */
+function createMultipleDoneError(message, runnable, originalErr) {
+  var err = new Error(message);
+  err.code = constants.ERR_MOCHA_MULTIPLE_DONE;
+  var title = runnable.title;
+  try {
+    title = runnable.fullTitle();
+  } catch (ignored) {
+    title += ' (unknown suite)';
+  }
+
+  err.runnable = {
+    file: runnable.file,
+    type: runnable.type,
+    title: title,
+    body: runnable.body
+  };
+  err.valueType = typeof originalErr;
+  err.value = originalErr;
+  return err;
+}
+
 module.exports = {
   createInvalidArgumentTypeError: createInvalidArgumentTypeError,
   createInvalidArgumentValueError: createInvalidArgumentValueError,
@@ -137,5 +196,8 @@ module.exports = {
   createInvalidReporterError: createInvalidReporterError,
   createMissingArgumentError: createMissingArgumentError,
   createNoFilesMatchPatternError: createNoFilesMatchPatternError,
-  createUnsupportedError: createUnsupportedError
+  createUnsupportedError: createUnsupportedError,
+  createFatalError: createFatalError,
+  createMultipleDoneError: createMultipleDoneError,
+  constants: constants
 };
diff --git a/lib/runnable.js b/lib/runnable.js
@@ -5,8 +5,9 @@ var Pending = require('./pending');
 var debug = require('debug')('mocha:runnable');
 var milliseconds = require('ms');
 var utils = require('./utils');
-var createInvalidExceptionError = require('./errors')
-  .createInvalidExceptionError;
+var errors = require('./errors');
+var createInvalidExceptionError = errors.createInvalidExceptionError;
+var createMultipleDoneError = errors.createMultipleDoneError;
 
 /**
  * Save timer references to avoid Sinon interfering (see GH-237).
@@ -306,13 +307,16 @@ Runnable.prototype.run = function(fn) {
       return;
     }
     emitted = true;
-    var msg = 'done() called multiple times';
-    if (err && err.message) {
-      err.message += " (and Mocha's " + msg + ')';
-      self.emit('error', err);
-    } else {
-      self.emit('error', new Error(msg));
-    }
+    self.emit(
+      'error',
+      createMultipleDoneError(
+        err && err.message
+          ? err.message + " (and Mocha's done() called multiple times)"
+          : 'done() called multiple times',
+        self,
+        err
+      )
+    );
   }
 
   // finished

diff --git a/lib/runner.js b/lib/runner.js
@@ -27,6 +27,7 @@ var type = utils.type;
 var errors = require('./errors');
 var createInvalidExceptionError = errors.createInvalidExceptionError;
 var createUnsupportedError = errors.createUnsupportedError;
+var createFatalError = errors.createFatalError;
 
 /**
  * Non-enumerable globals.
@@ -109,7 +110,19 @@ var constants = utils.defineConstants(
     /**
      * Emitted when {@link Test} execution has failed, but will retry
      */
-    EVENT_TEST_RETRY: 'retry'
+    EVENT_TEST_RETRY: 'retry',
+    /**
+     * Initial state of Runner
+     */
+    STATE_IDLE: 'idle',
+    /**
+     * State set to this value when the Runner has started running
+     */
+    STATE_RUNNING: 'running',
+    /**
+     * State set to this value when the Runner has stopped
+     */
+    STATE_STOPPED: 'stopped'
   }
 );
 
@@ -131,7 +144,7 @@ function Runner(suite, delay) {
   this._abort = false;
   this._delay = delay;
   this.suite = suite;
-  this.started = false;
+  this.state = constants.STATE_IDLE;
   this.total = suite.total();
   this.failures = 0;
   this.on(constants.EVENT_TEST_END, function(test) {
@@ -284,11 +297,21 @@ Runner.prototype.checkGlobals = function(test) {
  * @param {Error} err
  */
 Runner.prototype.fail = function(test, err) {
+  if (this.state === constants.STATE_STOPPED) {
+    if (err.code === errors.constants.ERR_MOCHA_MULTIPLE_DONE) {
+      throw err;
+    }
+    throw createFatalError(
+      'Test failed after root suite execution completed!',
+      err
+    );
+  }
   if (test.isPending()) {
     return;
   }
 
   ++this.failures;
+  debug('total number of failures: %d', this.failures);
   test.state = STATE_FAILED;
 
   if (!isError(err)) {
@@ -834,7 +857,7 @@ Runner.prototype.uncaught = function(err) {
     runnable = new Runnable('Uncaught error outside test suite');
     runnable.parent = this.suite;
 
-    if (this.started) {
+    if (this.state === constants.STATE_RUNNING) {
       this.fail(runnable, err);
     } else {
       // Can't recover from this failure
@@ -928,7 +951,7 @@ Runner.prototype.run = function(fn) {
     if (rootSuite.hasOnly()) {
       rootSuite.filterOnly();
     }
-    self.started = true;
+    self.state = constants.STATE_RUNNING;
     if (self._delay) {
       self.emit(constants.EVENT_DELAY_END);
     }
@@ -949,6 +972,7 @@ Runner.prototype.run = function(fn) {
 
   // callback
   this.on(constants.EVENT_RUN_END, function() {
+    this.state = constants.STATE_STOPPED;
     debug(constants.EVENT_RUN_END);
     process.removeListener('uncaughtException', uncaught);
     process.on('uncaughtException', self.uncaughtEnd);

diff --git a/package-lock.json b/package-lock.json
diff --git a/package-scripts.js b/package-scripts.js
@@ -17,7 +17,7 @@ function test(testName, mochaParams) {
   }
   return `${
     process.env.COVERAGE ? coverageCommand : ''
-  } ${mochaCommand} ${mochaParams}`.trim();
+  } ${mochaCommand} ${mochaParams} --color`.trim();
 }
 
 module.exports = {

diff --git a/test/integration/fixtures/multiple-done-async.fixture.js b/test/integration/fixtures/multiple-done-async.fixture.js
@@ -0,0 +1,20 @@
+'use strict';
+
+// The suite below should result in an additional error, but does
+// not. Uncomment once this bug is resolved.
+
+// describe('suite', function() {
+//   beforeEach(function(done) {
+//     done();
+//     done();
+//   });
+
+//   it('test', function() {});
+// });
+
+it('should fail in an async test case', function (done) {
+  process.nextTick(function () {
+    done();
+    setTimeout(done);
+  });
+});