From db4f885ce0f536c401273e92fd103e69b24fecab Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Mon, 13 Jan 2020 16:55:14 -0800 Subject: [PATCH 1/5] 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) - force color in Travis CI b/c my eyes --- lib/cli/collect-files.js | 3 +- lib/errors.js | 140 +++++++++++++++-- lib/runnable.js | 23 ++- lib/runner.js | 32 +++- package-scripts.js | 3 + .../fixtures/multiple-done-async.fixture.js | 20 +++ ...s => multiple-done-before-each.fixture.js} | 0 test/integration/multiple-done.spec.js | 148 +++++++++++------- test/unit/runnable.spec.js | 10 +- test/unit/runner.spec.js | 48 ++++-- 10 files changed, 322 insertions(+), 105 deletions(-) create mode 100644 test/integration/fixtures/multiple-done-async.fixture.js rename test/integration/fixtures/{multiple-done-beforeEach.fixture.js => multiple-done-before-each.fixture.js} (100%) diff --git a/lib/cli/collect-files.js b/lib/cli/collect-files.js index 61d54ac4b3..4145f4333c 100644 --- a/lib/cli/collect-files.js +++ b/lib/cli/collect-files.js @@ -5,6 +5,7 @@ const ansi = require('ansi-colors'); const debug = require('debug')('mocha:cli:run:helpers'); const minimatch = require('minimatch'); const utils = require('../utils'); +const {NO_FILES_MATCH_PATTERN} = require('../errors').constants; /** * Exports a function that collects test files from CLI parameters. @@ -34,7 +35,7 @@ module.exports = ({ignore, extension, file, recursive, sort, spec} = {}) => { try { newFiles = utils.lookupFiles(arg, extension, recursive); } catch (err) { - if (err.code === 'ERR_MOCHA_NO_FILES_MATCH_PATTERN') { + if (err.code === NO_FILES_MATCH_PATTERN) { unmatched.push({message: err.message, pattern: err.pattern}); return; } diff --git a/lib/errors.js b/lib/errors.js index a85c8c24b9..1e665e5fb3 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -1,10 +1,73 @@ 'use strict'; + +var format = require('util').format; + /** + * Factory functions to create throwable error objects * @module Errors */ + /** - * Factory functions to create throwable error objects + * When Mocha throw exceptions (or otherwise errors), it attempts to assign a + * `code` property to the `Error` object, for easier handling. These are the + * potential values of `code`. */ +var constants = { + /** + * An unrecoverable error. + */ + FATAL: 'ERR_MOCHA_FATAL', + + /** + * The type of an argument to a function call is invalid + */ + INVALID_ARG_TYPE: 'ERR_MOCHA_INVALID_ARG_TYPE', + + /** + * The value of an argument to a function call is invalid + */ + INVALID_ARG_VALUE: 'ERR_MOCHA_INVALID_ARG_VALUE', + + /** + * Something was thrown, but it wasn't an `Error` + */ + INVALID_EXCEPTION: 'ERR_MOCHA_INVALID_EXCEPTION', + + /** + * An interface (e.g., `Mocha.interfaces`) is unknown or invalid + */ + INVALID_INTERFACE: 'ERR_MOCHA_INVALID_INTERFACE', + + /** + * A reporter (.e.g, `Mocha.reporters`) is unknown or invalid + */ + INVALID_REPORTER: 'ERR_MOCHA_INVALID_REPORTER', + + /** + * `done()` was called twice in a `Test` or `Hook` callback + */ + MULTIPLE_DONE: 'ERR_MOCHA_MULTIPLE_DONE', + + /** + * No files matched the pattern provided by the user + */ + NO_FILES_MATCH_PATTERN: 'ERR_MOCHA_NO_FILES_MATCH_PATTERN', + + /** + * Known, but unsupported behavior of some kind + */ + UNSUPPORTED: 'ERR_MOCHA_UNSUPPORTED', + + /** + * Invalid state transition occuring in `Mocha` instance + */ + INSTANCE_ALREADY_RUNNING: 'ERR_MOCHA_INSTANCE_ALREADY_RUNNING', + + /** + * Invalid state transition occuring in `Mocha` instance + */ + INSTANCE_ALREADY_DISPOSED: 'ERR_MOCHA_INSTANCE_ALREADY_DISPOSED' +}; /** * Creates an error object to be thrown when no files to be tested could be found using specified pattern. @@ -16,7 +79,7 @@ */ function createNoFilesMatchPatternError(message, pattern) { var err = new Error(message); - err.code = 'ERR_MOCHA_NO_FILES_MATCH_PATTERN'; + err.code = constants.NO_FILES_MATCH_PATTERN; err.pattern = pattern; return err; } @@ -31,7 +94,7 @@ function createNoFilesMatchPatternError(message, pattern) { */ function createInvalidReporterError(message, reporter) { var err = new TypeError(message); - err.code = 'ERR_MOCHA_INVALID_REPORTER'; + err.code = constants.INVALID_REPORTER; err.reporter = reporter; return err; } @@ -46,7 +109,7 @@ function createInvalidReporterError(message, reporter) { */ function createInvalidInterfaceError(message, ui) { var err = new Error(message); - err.code = 'ERR_MOCHA_INVALID_INTERFACE'; + err.code = constants.INVALID_INTERFACE; err.interface = ui; return err; } @@ -60,7 +123,7 @@ function createInvalidInterfaceError(message, ui) { */ function createUnsupportedError(message) { var err = new Error(message); - err.code = 'ERR_MOCHA_UNSUPPORTED'; + err.code = constants.UNSUPPORTED; return err; } @@ -88,7 +151,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.INVALID_ARG_TYPE; err.argument = argument; err.expected = expected; err.actual = typeof argument; @@ -107,7 +170,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.INVALID_ARG_VALUE; err.argument = argument; err.value = value; err.reason = typeof reason !== 'undefined' ? reason : 'is invalid'; @@ -123,7 +186,22 @@ function createInvalidArgumentValueError(message, argument, value, reason) { */ function createInvalidExceptionError(message, value) { var err = new Error(message); - err.code = 'ERR_MOCHA_INVALID_EXCEPTION'; + err.code = constants.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.FATAL; err.valueType = typeof value; err.value = value; return err; @@ -161,7 +239,7 @@ function createMochaInstanceAlreadyDisposedError( instance ) { var err = new Error(message); - err.code = 'ERR_MOCHA_INSTANCE_ALREADY_DISPOSED'; + err.code = constants.INSTANCE_ALREADY_DISPOSED; err.cleanReferencesAfterRun = cleanReferencesAfterRun; err.instance = instance; return err; @@ -173,11 +251,48 @@ function createMochaInstanceAlreadyDisposedError( */ function createMochaInstanceAlreadyRunningError(message, instance) { var err = new Error(message); - err.code = 'ERR_MOCHA_INSTANCE_ALREADY_RUNNING'; + err.code = constants.INSTANCE_ALREADY_RUNNING; err.instance = instance; return err; } +/* + * Creates an error object to be thrown when done() is called multiple times in a test + * + * @public + * @param {Runnable} runnable - Original runnable + * @param {Error} [originalErr] - Original error, if any + * @returns {Error} instance detailing the error condition + */ +function createMultipleDoneError(runnable, originalErr) { + var title; + try { + title = format('<%s>', runnable.fullTitle()); + if (runnable.parent.root) { + title += ' (of root suite)'; + } + } catch (ignored) { + title = format('<%s> (of unknown suite)', runnable.title); + } + var message = format( + 'done() called multiple times in %s %s', + runnable.type ? runnable.type : 'unknown runnable', + title + ); + if (runnable.file) { + message += format(' of file %s', runnable.file); + } + if (originalErr) { + message += format('; in addition, done() received error: %s', originalErr); + } + + var err = new Error(message); + err.code = constants.MULTIPLE_DONE; + err.valueType = typeof originalErr; + err.value = originalErr; + return err; +} + module.exports = { createInvalidArgumentTypeError: createInvalidArgumentTypeError, createInvalidArgumentValueError: createInvalidArgumentValueError, @@ -189,5 +304,8 @@ module.exports = { createUnsupportedError: createUnsupportedError, createInvalidPluginError: createInvalidPluginError, createMochaInstanceAlreadyDisposedError: createMochaInstanceAlreadyDisposedError, - createMochaInstanceAlreadyRunningError: createMochaInstanceAlreadyRunningError + createMochaInstanceAlreadyRunningError: createMochaInstanceAlreadyRunningError, + createFatalError: createFatalError, + createMultipleDoneError: createMultipleDoneError, + constants: constants }; diff --git a/lib/runnable.js b/lib/runnable.js index 4d58070f5d..342152c3c2 100644 --- 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). @@ -262,7 +263,7 @@ Runnable.prototype.run = function(fn) { var start = new Date(); var ctx = this.ctx; var finished; - var emitted; + var errorWasHandled = false; if (this.isPending()) return fn(); @@ -273,17 +274,11 @@ Runnable.prototype.run = function(fn) { // called multiple times function multiple(err) { - if (emitted) { + if (errorWasHandled) { 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)); - } + errorWasHandled = true; + self.emit('error', createMultipleDoneError(self, err)); } // finished @@ -334,7 +329,7 @@ Runnable.prototype.run = function(fn) { callFnAsync(this.fn); } catch (err) { // handles async runnables which actually run synchronously - emitted = true; + errorWasHandled = true; if (err instanceof Pending) { return; // done() is already called in this.skip() } else if (this.allowUncaught) { @@ -349,7 +344,7 @@ Runnable.prototype.run = function(fn) { try { callFn(this.fn); } catch (err) { - emitted = true; + errorWasHandled = true; if (err instanceof Pending) { return done(); } else if (this.allowUncaught) { diff --git a/lib/runner.js b/lib/runner.js index 7eabb5a1cf..d37bccdd30 100644 --- 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' } ); @@ -140,8 +153,8 @@ function Runner(suite, opts) { this._globals = []; this._abort = false; this.suite = suite; - this.started = false; this._opts = opts; + this.state = constants.STATE_IDLE; this.total = suite.total(); this.failures = 0; this._eventListeners = []; @@ -351,8 +364,18 @@ Runner.prototype.fail = function(test, err, force) { if (test.isPending() && !force) { return; } + if (this.state === constants.STATE_STOPPED) { + if (err.code === errors.constants.MULTIPLE_DONE) { + throw err; + } + throw createFatalError( + 'Test failed after root suite execution completed!', + err + ); + } ++this.failures; + debug('total number of failures: %d', this.failures); test.state = STATE_FAILED; if (!isError(err)) { @@ -881,7 +904,7 @@ Runner.prototype.uncaught = function(err) { debug('uncaught(): no current Runnable; created a phony one'); runnable.parent = this.suite; - if (this.started) { + if (this.state === constants.STATE_RUNNING) { debug('uncaught(): failing gracefully'); this.fail(runnable, err); } else { @@ -957,7 +980,7 @@ Runner.prototype.run = function(fn) { rootSuite.filterOnly(); debug('run(): filtered exclusive Runnables'); } - self.started = true; + self.state = constants.STATE_RUNNING; if (self._delay) { self.emit(constants.EVENT_DELAY_END); debug('run(): "delay" ended'); @@ -985,6 +1008,7 @@ Runner.prototype.run = function(fn) { // callback this.on(constants.EVENT_RUN_END, function() { + self.state = constants.STATE_STOPPED; debug(constants.EVENT_RUN_END); self._removeEventListener(process, 'uncaughtException', uncaught); self._addEventListener(process, 'uncaughtException', self.uncaughtEnd); diff --git a/package-scripts.js b/package-scripts.js index f3775db177..7662053795 100644 --- a/package-scripts.js +++ b/package-scripts.js @@ -15,6 +15,9 @@ function test(testName, mochaParams) { if (process.env.CI && !/^only-/.test(testName)) { mochaParams += ' --forbid-only'; } + if (process.env.TRAVIS) { + mochaParams += ' --color'; // force color in travis-ci + } return `${ process.env.COVERAGE ? coverageCommand : '' } ${mochaCommand} ${mochaParams}`.trim(); diff --git a/test/integration/fixtures/multiple-done-async.fixture.js b/test/integration/fixtures/multiple-done-async.fixture.js new file mode 100644 index 0000000000..36f0dd336d --- /dev/null +++ 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); + }); +}); diff --git a/test/integration/fixtures/multiple-done-beforeEach.fixture.js b/test/integration/fixtures/multiple-done-before-each.fixture.js similarity index 100% rename from test/integration/fixtures/multiple-done-beforeEach.fixture.js rename to test/integration/fixtures/multiple-done-before-each.fixture.js diff --git a/test/integration/multiple-done.spec.js b/test/integration/multiple-done.spec.js index 5b592c8877..2019df25c5 100644 --- a/test/integration/multiple-done.spec.js +++ b/test/integration/multiple-done.spec.js @@ -1,129 +1,159 @@ 'use strict'; -var assert = require('assert'); -var run = require('./helpers').runMochaJSON; -var args = []; +var runMochaJSON = require('./helpers').runMochaJSON; +var invokeMocha = require('./helpers').invokeMocha; +var MULTIPLE_DONE = require('../../lib/errors').constants.MULTIPLE_DONE; describe('multiple calls to done()', function() { var res; describe('from a spec', function() { before(function(done) { - run('multiple-done.fixture.js', args, function(err, result) { + runMochaJSON('multiple-done', function(err, result) { res = result; done(err); }); }); - it('results in failures', function() { - assert.strictEqual(res.stats.pending, 0, 'wrong "pending" count'); - assert.strictEqual(res.stats.passes, 1, 'wrong "passes" count'); - assert.strictEqual(res.stats.failures, 1, 'wrong "failures" count'); + it('results in failure', function() { + expect(res, 'to have failed test count', 1) + .and('to have passed test count', 1) + .and('to have pending test count', 0) + .and('to have failed'); }); it('throws a descriptive error', function() { - assert.strictEqual( - res.failures[0].err.message, - 'done() called multiple times' - ); + expect(res, 'to have failed with error', { + message: /done\(\) called multiple times in test \(of root suite\) of file.+multiple-done\.fixture\.js/, + code: MULTIPLE_DONE + }); }); }); describe('with error passed on second call', function() { before(function(done) { - run('multiple-done-with-error.fixture.js', args, function(err, result) { + runMochaJSON('multiple-done-with-error', function(err, result) { res = result; done(err); }); }); - it('results in failures', function() { - assert.strictEqual(res.stats.pending, 0, 'wrong "pending" count'); - assert.strictEqual(res.stats.passes, 1, 'wrong "passes" count'); - assert.strictEqual(res.stats.failures, 1, 'wrong "failures" count'); + it('results in failure', function() { + expect(res, 'to have failed test count', 1) + .and('to have passed test count', 1) + .and('to have pending test count', 0) + .and('to have failed'); }); it('should throw a descriptive error', function() { - assert.strictEqual( - res.failures[0].err.message, - "second error (and Mocha's done() called multiple times)" - ); + expect(res, 'to have failed with error', { + message: /done\(\) called multiple times in test \(of root suite\) of file.+multiple-done-with-error\.fixture\.js; in addition, done\(\) received error: Error: second error/, + code: MULTIPLE_DONE + }); }); }); describe('with multiple specs', function() { before(function(done) { - run('multiple-done-specs.fixture.js', args, function(err, result) { + runMochaJSON('multiple-done-specs', function(err, result) { res = result; done(err); }); }); - it('results in a failure', function() { - assert.strictEqual(res.stats.pending, 0); - assert.strictEqual(res.stats.passes, 2); - assert.strictEqual(res.stats.failures, 1); - assert.strictEqual(res.code, 1); + it('results in failure', function() { + expect(res, 'to have failed test count', 1) + .and('to have passed test count', 2) + .and('to have pending test count', 0) + .and('to have failed'); }); it('correctly attributes the error', function() { - assert.strictEqual(res.failures[0].fullTitle, 'suite test1'); - assert.strictEqual( - res.failures[0].err.message, - 'done() called multiple times' - ); + expect(res.failures[0], 'to satisfy', { + fullTitle: 'suite test1', + err: { + message: /done\(\) called multiple times in test of file.+multiple-done-specs\.fixture\.js/, + code: MULTIPLE_DONE + } + }); }); }); describe('from a before hook', function() { before(function(done) { - run('multiple-done-before.fixture.js', args, function(err, result) { + runMochaJSON('multiple-done-before', function(err, result) { res = result; done(err); }); }); - it('results in a failure', function() { - assert.strictEqual(res.stats.pending, 0); - assert.strictEqual(res.stats.passes, 1); - assert.strictEqual(res.stats.failures, 1); - assert.strictEqual(res.code, 1); + it('results in failure', function() { + expect(res, 'to have failed test count', 1) + .and('to have passed test count', 1) + .and('to have pending test count', 0) + .and('to have failed'); }); it('correctly attributes the error', function() { - assert.strictEqual( - res.failures[0].fullTitle, - 'suite "before all" hook in "suite"' - ); - assert.strictEqual( - res.failures[0].err.message, - 'done() called multiple times' - ); + expect(res.failures[0], 'to satisfy', { + fullTitle: 'suite "before all" hook in "suite"', + err: { + message: /done\(\) called multiple times in hook of file.+multiple-done-before\.fixture\.js/ + } + }); }); }); - describe('from a beforeEach hook', function() { + describe('from a "before each" hook', function() { before(function(done) { - run('multiple-done-beforeEach.fixture.js', args, function(err, result) { + runMochaJSON('multiple-done-before-each', function(err, result) { res = result; done(err); }); }); it('results in a failure', function() { - assert.strictEqual(res.stats.pending, 0); - assert.strictEqual(res.stats.passes, 2); - assert.strictEqual(res.stats.failures, 2); - assert.strictEqual(res.code, 2); + expect(res, 'to have failed test count', 2) + .and('to have passed test count', 2) + .and('to have pending test count', 0) + .and('to have exit code', 2); }); it('correctly attributes the errors', function() { - assert.strictEqual(res.failures.length, 2); - res.failures.forEach(function(failure) { - assert.strictEqual( - failure.fullTitle, - 'suite "before each" hook in "suite"' - ); - assert.strictEqual(failure.err.message, 'done() called multiple times'); + expect(res.failures, 'to satisfy', [ + { + fullTitle: 'suite "before each" hook in "suite"', + err: { + message: /done\(\) called multiple times in hook of file.+multiple-done-before-each\.fixture\.js/ + } + }, + { + fullTitle: 'suite "before each" hook in "suite"', + err: { + message: /done\(\) called multiple times in hook of file.+multiple-done-before-each\.fixture\.js/ + } + } + ]); + }); + }); + + describe('when done() called asynchronously', function() { + before(function(done) { + // we can't be sure that mocha won't fail with an uncaught exception here, which would cause any JSON + // output to be befouled; we need to run "raw" and capture STDERR + invokeMocha( + require.resolve('./fixtures/multiple-done-async.fixture.js'), + function(err, result) { + res = result; + done(err); + }, + 'pipe' + ); + }); + + it('results in error', function() { + expect(res, 'to satisfy', { + code: expect.it('to be greater than', 0), + output: /done\(\) called multiple times in test \(of root suite\) of file.+multiple-done-async\.fixture\.js/ }); }); }); diff --git a/test/unit/runnable.spec.js b/test/unit/runnable.spec.js index bdd2dc145e..4f00dadfd7 100644 --- a/test/unit/runnable.spec.js +++ b/test/unit/runnable.spec.js @@ -325,7 +325,11 @@ describe('Runnable(title, fn)', function() { runnable.on('error', errorSpy).on('error', function(err) { process.nextTick(function() { expect(errorSpy, 'was called times', 1); - expect(err.message, 'to be', 'done() called multiple times'); + expect( + err.message, + 'to match', + /done\(\) called multiple times/ + ); expect(callbackSpy, 'was called times', 1); done(); }); @@ -355,8 +359,8 @@ describe('Runnable(title, fn)', function() { expect(errorSpy, 'was called times', 1); expect( err.message, - 'to be', - "fail (and Mocha's done() called multiple times)" + 'to match', + /done\(\) called multiple times.+received error: Error: fail/ ); expect(callbackSpy, 'was called times', 1); done(); diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index d36d0f2f1f..b3f475f199 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -17,6 +17,9 @@ var EVENT_TEST_END = Runner.constants.EVENT_TEST_END; var EVENT_RUN_END = Runner.constants.EVENT_RUN_END; var EVENT_SUITE_END = Runner.constants.EVENT_SUITE_END; var STATE_FAILED = Runnable.constants.STATE_FAILED; +var STATE_IDLE = Runner.constants.STATE_IDLE; +var STATE_RUNNING = Runner.constants.STATE_RUNNING; +var STATE_STOPPED = Runner.constants.STATE_STOPPED; describe('Runner', function() { var sandbox; @@ -851,7 +854,7 @@ describe('Runner', function() { describe('when Runner has already started', function() { beforeEach(function() { - runner.started = true; + runner.state = STATE_RUNNING; }); it('should not emit start/end events', function() { @@ -866,20 +869,39 @@ describe('Runner', function() { }); }); - describe('when Runner has not already started', function() { - beforeEach(function() { - runner.started = false; + describe('when Runner not running', function() { + describe('when idle', function() { + beforeEach(function() { + runner.state = STATE_IDLE; + }); + + it('should emit start/end events for the benefit of reporters', function() { + expect( + function() { + runner.uncaught(err); + }, + 'to emit from', + runner, + 'start' + ).and('to emit from', runner, 'end'); + }); }); - it('should emit start/end events for the benefit of reporters', function() { - expect( - function() { - runner.uncaught(err); - }, - 'to emit from', - runner, - 'start' - ).and('to emit from', runner, 'end'); + describe('when stopped', function() { + beforeEach(function() { + runner.state = STATE_STOPPED; + }); + + it('should emit start/end events for the benefit of reporters', function() { + expect( + function() { + runner.uncaught(err); + }, + 'to emit from', + runner, + 'start' + ).and('to emit from', runner, 'end'); + }); }); }); }); From cf66f002ee544766ffdf1277cd2fe67ca92725cc Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Wed, 13 May 2020 17:24:06 -0700 Subject: [PATCH 2/5] remove Runner#uncaughtEnd move logic to `Runner#uncaught`, since we can now rely on the value of `Runner#state`. Signed-off-by: Christopher Hiller --- lib/runner.js | 20 ++++++------------- .../fixtures/uncaught/listeners.fixture.js | 6 +++--- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/lib/runner.js b/lib/runner.js index d37bccdd30..aeef92eb1d 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -881,6 +881,11 @@ Runner.prototype.uncaught = function(err) { throw err; } + if (this.state === constants.STATE_STOPPED) { + debug('uncaught(): throwing after run has completed!'); + throw err; + } + if (err) { debug('uncaught(): got truthy exception %O', err); } else { @@ -943,17 +948,6 @@ Runner.prototype.uncaught = function(err) { } }; -/** - * Handle uncaught exceptions after runner's end event. - * - * @param {Error} err - * @private - */ -Runner.prototype.uncaughtEnd = function uncaughtEnd(err) { - if (err instanceof Pending) return; - throw err; -}; - /** * Run the root suite and invoke `fn(failures)` * on completion. @@ -1010,14 +1004,12 @@ Runner.prototype.run = function(fn) { this.on(constants.EVENT_RUN_END, function() { self.state = constants.STATE_STOPPED; debug(constants.EVENT_RUN_END); - self._removeEventListener(process, 'uncaughtException', uncaught); - self._addEventListener(process, 'uncaughtException', self.uncaughtEnd); debug('run(): emitted %s', constants.EVENT_RUN_END); fn(self.failures); }); // uncaught exception - self._removeEventListener(process, 'uncaughtException', self.uncaughtEnd); + self._removeEventListener(process, 'uncaughtException', uncaught); self._addEventListener(process, 'uncaughtException', uncaught); if (this._delay) { diff --git a/test/integration/fixtures/uncaught/listeners.fixture.js b/test/integration/fixtures/uncaught/listeners.fixture.js index 69c4059294..fa7ba4106d 100644 --- a/test/integration/fixtures/uncaught/listeners.fixture.js +++ b/test/integration/fixtures/uncaught/listeners.fixture.js @@ -1,13 +1,13 @@ 'use strict'; const assert = require('assert'); -const mocha = require("../../../../lib/mocha"); +const mocha = require('../../../../lib/mocha'); // keep this low to avoid warning for (let i = 0; i < 5; i++) { - const r = new mocha.Runner(new mocha.Suite("" + i, undefined)); + const r = new mocha.Runner(new mocha.Suite('' + i, undefined)); r.run(); } assert.equal(process.listenerCount('uncaughtException'), 1); -assert.equal(process.listeners('uncaughtException')[0].name, 'uncaughtEnd'); +assert.equal(process.listeners('uncaughtException')[0].name, 'uncaught'); From 770659d10aaa96362ac55469735146987ff6a1c7 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Mon, 18 May 2020 17:01:47 -0700 Subject: [PATCH 3/5] upgrade unexpected-eventemitter Signed-off-by: Christopher Hiller --- package-lock.json | 6 +++--- package.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index d848f8692e..71777a3890 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17440,9 +17440,9 @@ "dev": true }, "unexpected-eventemitter": { - "version": "1.1.3", - "resolved": "https://registry.npmjs.org/unexpected-eventemitter/-/unexpected-eventemitter-1.1.3.tgz", - "integrity": "sha512-30MfVuCOCSEvUzNUErYZ3ZzLiPOgADcJsyxi+0Z5bhwgI/Yv4xHR/2v/YEe2alaEXDdkteCQ4gLBfa5/J2iTPA==", + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/unexpected-eventemitter/-/unexpected-eventemitter-2.2.0.tgz", + "integrity": "sha512-ciEWOd9kJ9sgyzP8Sai9VdMDiGeoKjeaovyzIUgGw82o0OW1/LbLD2Cf8Cs6cBrtLQdzElBFOe8veMt/bSC49g==", "dev": true }, "unexpected-sinon": { diff --git a/package.json b/package.json index 7ceda0d79a..42df478855 100644 --- a/package.json +++ b/package.json @@ -129,7 +129,7 @@ "through2": "^3.0.1", "to-vfile": "^6.1.0", "unexpected": "^11.13.0", - "unexpected-eventemitter": "^1.1.3", + "unexpected-eventemitter": "^2.2.0", "unexpected-sinon": "^10.11.2", "uslug": "^1.0.4", "watchify": "^3.11.1" From 55283e335a8ce472a53113a2eeac414c6816bda2 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Mon, 18 May 2020 17:01:51 -0700 Subject: [PATCH 4/5] fix test Signed-off-by: Christopher Hiller --- test/unit/runner.spec.js | 112 ++++++++++++++++++++++++++++----------- 1 file changed, 80 insertions(+), 32 deletions(-) diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index b3f475f199..7bd2e96a1a 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -10,6 +10,7 @@ var Test = Mocha.Test; var Runnable = Mocha.Runnable; var Hook = Mocha.Hook; var noop = Mocha.utils.noop; +var errors = require('../../lib/errors'); var EVENT_HOOK_BEGIN = Runner.constants.EVENT_HOOK_BEGIN; var EVENT_TEST_FAIL = Runner.constants.EVENT_TEST_FAIL; var EVENT_TEST_RETRY = Runner.constants.EVENT_TEST_RETRY; @@ -255,8 +256,8 @@ describe('Runner', function() { }); }); - describe('.fail(test, err)', function() { - it('should increment .failures', function() { + describe('fail()', function() { + it('should increment `Runner#failures`', function() { expect(runner.failures, 'to be', 0); runner.fail(new Test('one', noop), {}); expect(runner.failures, 'to be', 1); @@ -264,7 +265,7 @@ describe('Runner', function() { expect(runner.failures, 'to be', 2); }); - it('should set test.state to "failed"', function() { + it('should set `Test#state` to "failed"', function() { var test = new Test('some test', noop); runner.fail(test, 'some error'); expect(test.state, 'to be', STATE_FAILED); @@ -376,6 +377,47 @@ describe('Runner', function() { runner.fail(test, new Error()); expect(runner.failures, 'to be', 0); }); + + describe('when Runner has stopped', function() { + beforeEach(function() { + runner.state = STATE_STOPPED; + }); + + describe('when test is not pending', function() { + describe('when error is the "multiple done" variety', function() { + it('should throw the "multiple done" error', function() { + var test = new Test('test', function() {}); + suite.addTest(test); + var err = new Error(); + err.code = errors.constants.MULTIPLE_DONE; + expect( + function() { + runner.fail(test, err); + }, + 'to throw', + err + ); + }); + }); + + describe('when error is not of the "multiple done" variety', function() { + it('should throw a "fatal" error', function() { + var test = new Test('test', function() {}); + suite.addTest(test); + var err = new Error(); + expect( + function() { + runner.fail(test, err); + }, + 'to throw', + { + code: errors.constants.FATAL + } + ); + }); + }); + }); + }); }); describe('.failHook(hook, err)', function() { @@ -852,7 +894,7 @@ describe('Runner', function() { ]).and('was called once'); }); - describe('when Runner has already started', function() { + describe('when Runner is RUNNING', function() { beforeEach(function() { runner.state = STATE_RUNNING; }); @@ -869,39 +911,45 @@ describe('Runner', function() { }); }); - describe('when Runner not running', function() { - describe('when idle', function() { - beforeEach(function() { - runner.state = STATE_IDLE; - }); + describe('when Runner is IDLE', function() { + beforeEach(function() { + runner.state = STATE_IDLE; + }); - it('should emit start/end events for the benefit of reporters', function() { - expect( - function() { - runner.uncaught(err); - }, - 'to emit from', - runner, - 'start' - ).and('to emit from', runner, 'end'); - }); + it('should emit start/end events for the benefit of reporters', function() { + expect( + function() { + runner.uncaught(err); + }, + 'to emit from', + runner, + 'start' + ).and('to emit from', runner, 'end'); }); + }); - describe('when stopped', function() { - beforeEach(function() { - runner.state = STATE_STOPPED; - }); + describe('when Runner is STOPPED', function() { + beforeEach(function() { + runner.state = STATE_STOPPED; + }); - it('should emit start/end events for the benefit of reporters', function() { - expect( - function() { + it('should not emit start/end events, since this presumably would have already happened', function() { + expect( + function() { + try { runner.uncaught(err); - }, - 'to emit from', - runner, - 'start' - ).and('to emit from', runner, 'end'); - }); + } catch (ignored) {} + }, + 'not to emit from', + runner, + 'start' + ).and('not to emit from', runner, 'end'); + }); + + it('should throw', function() { + expect(function() { + runner.uncaught(err); + }, 'to throw'); }); }); }); From 7b39f4cf806e0e590ab418fde4023e2e1f7d9cce Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Tue, 19 May 2020 15:17:45 -0700 Subject: [PATCH 5/5] fix potential listener leak Signed-off-by: Christopher Hiller --- lib/runner.js | 47 +++++++++++++++++++++++++++------------- test/unit/runner.spec.js | 31 +++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/lib/runner.js b/lib/runner.js index aeef92eb1d..3c857294f7 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -172,6 +172,8 @@ function Runner(suite, opts) { this._defaultGrep = /.*/; this.grep(this._defaultGrep); this.globals(this.globalProps()); + + this.uncaught = this._uncaught.bind(this); } /** @@ -189,9 +191,9 @@ inherits(Runner, EventEmitter); /** * Replacement for `target.on(eventName, listener)` that does bookkeeping to remove them when this runner instance is disposed. - * @param target {EventEmitter} - * @param eventName {string} - * @param fn {function} + * @param {EventEmitter} target - The `EventEmitter` + * @param {string} eventName - The event name + * @param {string} fn - Listener function */ Runner.prototype._addEventListener = function(target, eventName, listener) { target.on(eventName, listener); @@ -200,9 +202,9 @@ Runner.prototype._addEventListener = function(target, eventName, listener) { /** * Replacement for `target.removeListener(eventName, listener)` that also updates the bookkeeping. - * @param target {EventEmitter} - * @param eventName {string} - * @param fn {function} + * @param {EventEmitter} target - The `EventEmitter` + * @param {string} eventName - The event anme + * @param {function} listener - Listener function */ Runner.prototype._removeEventListener = function(target, eventName, listener) { var eventListenerIndex = -1; @@ -867,10 +869,30 @@ Runner.prototype.runSuite = function(suite, fn) { /** * Handle uncaught exceptions within runner. * - * @param {Error} err + * This function is bound to the instance as `Runner#uncaught` at instantiation + * time. It's intended to be listening on the `Process.uncaughtException` event. + * In order to not leak EE listeners, we need to ensure no more than a single + * `uncaughtException` listener exists per `Runner`. The only way to do + * this--because this function needs the context (and we don't have lambdas)--is + * to use `Function.prototype.bind`. We need strict equality to unregister and + * _only_ unregister the _one_ listener we set from the + * `Process.uncaughtException` event; would be poor form to just remove + * everything. See {@link Runner#run} for where the event listener is registered + * and unregistered. + * @param {Error} err - Some uncaught error * @private */ -Runner.prototype.uncaught = function(err) { +Runner.prototype._uncaught = function(err) { + // this is defensive to prevent future developers from mis-calling this function. + // it's more likely that it'd be called with the incorrect context--say, the global + // `process` object--than it would to be called with a context that is not a "subclass" + // of `Runner`. + if (!(this instanceof Runner)) { + throw createFatalError( + 'Runner#uncaught() called with invalid context', + this + ); + } if (err instanceof Pending) { debug('uncaught(): caught a Pending'); return; @@ -963,10 +985,6 @@ Runner.prototype.run = function(fn) { fn = fn || function() {}; - function uncaught(err) { - self.uncaught(err); - } - function start() { debug('run(): starting'); // If there is an `only` filter @@ -1008,9 +1026,8 @@ Runner.prototype.run = function(fn) { fn(self.failures); }); - // uncaught exception - self._removeEventListener(process, 'uncaughtException', uncaught); - self._addEventListener(process, 'uncaughtException', uncaught); + self._removeEventListener(process, 'uncaughtException', self.uncaught); + self._addEventListener(process, 'uncaughtException', self.uncaught); if (this._delay) { // for reporters, I guess. diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index 7bd2e96a1a..1843a4daa9 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -527,6 +527,26 @@ describe('Runner', function() { runner.emit(EVENT_SUITE_END, suite); expect(cleanReferencesStub, 'was not called'); }); + + it('should not leak `Process.uncaughtException` listeners', function(done) { + var normalUncaughtExceptionListenerCount = process.listenerCount( + 'uncaughtException' + ); + + runner.run(); + runner.run(); + runner.run(); + expect( + process.listenerCount('uncaughtException'), + 'to be', + normalUncaughtExceptionListenerCount + 1 + ); + done(); + }); + + afterEach(function() { + runner.dispose(); + }); }); describe('.dispose', function() { @@ -550,7 +570,6 @@ describe('Runner', function() { var normalUncaughtExceptionListenerCount = process.listenerCount( 'uncaughtException' ); - sandbox.stub(); runner.run(noop); // sanity check expect( @@ -815,6 +834,16 @@ describe('Runner', function() { }); }); + describe('_uncaught()', function() { + describe('when called with a non-Runner context', function() { + it('should throw', function() { + expect(runner._uncaught.bind({}), 'to throw', { + code: errors.constants.FATAL + }); + }); + }); + }); + describe('uncaught()', function() { beforeEach(function() { sandbox.stub(runner, 'fail');