From 562b94af817f45770393978a0daa997845b46418 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Thu, 29 Oct 2020 15:51:46 -0700 Subject: [PATCH 1/2] delegate to Node on non-Mocha unhandled rejections This is not intended as a _fix_ for #4481, since it's possible that Mocha's behavior in v8.2.0 uncovers false positives. In other cases--depending on the use-case--they are not false positives at all, but rather annoyances that depended on the pre-v15 behavior of Node.js to only issue warnings. This PR changes the behavior of Mocha so that it will re-emit unhandled rejections to `process` _if_ they did not generate from Mocha. If the rejection generated from Mocha, then we treat it as an uncaught exception, because Mocha should not be in the business of ignoring its own unhandled rejections. The logic for detecting an "error originating from Mocha" is not exhaustive. Once an unhandled rejection is re-emitted to `process`, Node decides what to do with it based on how it is configured to handle unhandled rejections (strict, warn, quiet, etc.). Added a public method to `errors` module; `isMochaError()` Ref: #4481 --- lib/errors.js | 13 ++++ lib/runner.js | 44 +++++++++--- .../fixtures/uncaught/unhandled.fixture.js | 7 ++ test/integration/uncaught.spec.js | 69 +++++++++++++++---- test/unit/errors.spec.js | 26 +++++++ 5 files changed, 135 insertions(+), 24 deletions(-) create mode 100644 test/integration/fixtures/uncaught/unhandled.fixture.js diff --git a/lib/errors.js b/lib/errors.js index 6f6c8e6a00..29f76c61b5 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -129,6 +129,8 @@ var constants = { INVALID_PLUGIN_DEFINITION: 'ERR_MOCHA_INVALID_PLUGIN_DEFINITION' }; +const MOCHA_ERRORS = new Set(Object.values(constants)); + /** * Creates an error object to be thrown when no files to be tested could be found using specified pattern. * @@ -419,6 +421,16 @@ function createInvalidPluginImplementationError( return err; } +/** + * Returns `true` if an error came out of Mocha. + * _Can suffer from false negatives, but not false positives._ + * @public + * @param {*} err - Error, or anything + * @returns {boolean} + */ +const isMochaError = err => + Boolean(err && typeof err === 'object' && MOCHA_ERRORS.has(err.code)); + module.exports = { constants, createFatalError, @@ -439,5 +451,6 @@ module.exports = { createNoFilesMatchPatternError, createUnsupportedError, deprecate, + isMochaError, warn }; diff --git a/lib/runner.js b/lib/runner.js index d3349ca7a0..9ff34171d4 100644 --- a/lib/runner.js +++ b/lib/runner.js @@ -24,10 +24,13 @@ var sQuote = utils.sQuote; var stackFilter = utils.stackTraceFilter(); var stringify = utils.stringify; -var errors = require('./errors'); -var createInvalidExceptionError = errors.createInvalidExceptionError; -var createUnsupportedError = errors.createUnsupportedError; -var createFatalError = errors.createFatalError; +const { + createInvalidExceptionError, + createUnsupportedError, + createFatalError, + isMochaError, + constants: errorConstants +} = require('./errors'); /** * Non-enumerable globals. @@ -179,6 +182,29 @@ class Runner extends EventEmitter { this.globals(this.globalProps()); this.uncaught = this._uncaught.bind(this); + this.unhandled = (reason, promise) => { + if (isMochaError(reason)) { + debug( + 'trapped unhandled rejection coming out of Mocha; forwarding to uncaught handler:', + reason + ); + this.uncaught(reason); + } else { + debug( + 'trapped unhandled rejection from (probably) user code; re-emitting on process' + ); + this._removeEventListener( + process, + 'unhandledRejection', + this.unhandled + ); + try { + process.emit('unhandledRejection', reason, promise); + } finally { + this._addEventListener(process, 'unhandledRejection', this.unhandled); + } + } + }; } } @@ -414,7 +440,7 @@ Runner.prototype.fail = function(test, err, force) { return; } if (this.state === constants.STATE_STOPPED) { - if (err.code === errors.constants.MULTIPLE_DONE) { + if (err.code === errorConstants.MULTIPLE_DONE) { throw err; } throw createFatalError( @@ -1025,9 +1051,7 @@ Runner.prototype.run = function(fn, opts = {}) { this.emit(constants.EVENT_RUN_BEGIN); debug('run(): emitted %s', constants.EVENT_RUN_BEGIN); - this.runSuite(rootSuite, async () => { - end(); - }); + this.runSuite(rootSuite, end); }; const prepare = () => { @@ -1061,9 +1085,9 @@ Runner.prototype.run = function(fn, opts = {}) { }); this._removeEventListener(process, 'uncaughtException', this.uncaught); - this._removeEventListener(process, 'unhandledRejection', this.uncaught); + this._removeEventListener(process, 'unhandledRejection', this.unhandled); this._addEventListener(process, 'uncaughtException', this.uncaught); - this._addEventListener(process, 'unhandledRejection', this.uncaught); + this._addEventListener(process, 'unhandledRejection', this.unhandled); if (this._delay) { // for reporters, I guess. diff --git a/test/integration/fixtures/uncaught/unhandled.fixture.js b/test/integration/fixtures/uncaught/unhandled.fixture.js new file mode 100644 index 0000000000..1267942b72 --- /dev/null +++ b/test/integration/fixtures/uncaught/unhandled.fixture.js @@ -0,0 +1,7 @@ +it('should emit an unhandled rejection', async function() { + setTimeout(() => { + Promise.resolve().then(() => { + throw new Error('yikes'); + }); + }); +}); diff --git a/test/integration/uncaught.spec.js b/test/integration/uncaught.spec.js index c517bb0577..9c67dd16a9 100644 --- a/test/integration/uncaught.spec.js +++ b/test/integration/uncaught.spec.js @@ -1,14 +1,17 @@ 'use strict'; -var helpers = require('./helpers'); -var run = helpers.runMochaJSON; -var runMocha = helpers.runMocha; -var invokeNode = helpers.invokeNode; +const { + runMocha, + runMochaJSON: run, + invokeMochaAsync, + invokeNode, + resolveFixturePath +} = require('./helpers'); var args = []; describe('uncaught exceptions', function() { it('handles uncaught exceptions from hooks', function(done) { - run('uncaught/hook.fixture.js', args, function(err, res) { + run('uncaught/hook', args, function(err, res) { if (err) { return done(err); } @@ -24,7 +27,7 @@ describe('uncaught exceptions', function() { }); it('handles uncaught exceptions from async specs', function(done) { - run('uncaught/double.fixture.js', args, function(err, res) { + run('uncaught/double', args, function(err, res) { if (err) { return done(err); } @@ -44,7 +47,7 @@ describe('uncaught exceptions', function() { }); it('handles uncaught exceptions from which Mocha cannot recover', function(done) { - run('uncaught/fatal.fixture.js', args, function(err, res) { + run('uncaught/fatal', args, function(err, res) { if (err) { return done(err); } @@ -61,7 +64,7 @@ describe('uncaught exceptions', function() { }); it('handles uncaught exceptions within pending tests', function(done) { - run('uncaught/pending.fixture.js', args, function(err, res) { + run('uncaught/pending', args, function(err, res) { if (err) { return done(err); } @@ -84,7 +87,7 @@ describe('uncaught exceptions', function() { }); it('handles uncaught exceptions within open tests', function(done) { - run('uncaught/recover.fixture.js', args, function(err, res) { + run('uncaught/recover', args, function(err, res) { if (err) { return done(err); } @@ -111,8 +114,7 @@ describe('uncaught exceptions', function() { }); it('removes uncaught exceptions handlers correctly', function(done) { - var path = require.resolve('./fixtures/uncaught/listeners.fixture.js'); - invokeNode([path], function(err, res) { + invokeNode([resolveFixturePath('uncaught/listeners')], function(err, res) { if (err) { return done(err); } @@ -124,7 +126,7 @@ describe('uncaught exceptions', function() { it("handles uncaught exceptions after runner's end", function(done) { runMocha( - 'uncaught/after-runner.fixture.js', + 'uncaught/after-runner', args, function(err, res) { if (err) { @@ -145,7 +147,7 @@ describe('uncaught exceptions', function() { }); it('issue-1327: should run the first test and then bail', function(done) { - run('uncaught/issue-1327.fixture.js', args, function(err, res) { + run('uncaught/issue-1327', args, function(err, res) { if (err) { return done(err); } @@ -159,7 +161,7 @@ describe('uncaught exceptions', function() { }); it('issue-1417: uncaught exceptions from async specs', function(done) { - run('uncaught/issue-1417.fixture.js', args, function(err, res) { + run('uncaught/issue-1417', args, function(err, res) { if (err) { return done(err); } @@ -174,4 +176,43 @@ describe('uncaught exceptions', function() { done(); }); }); + + describe('issue-4481: behavior of non-Mocha-originating unhandled rejections', function() { + describe('when Node is in "warn" mode', function() { + it('should warn', async function() { + const [, promise] = invokeMochaAsync( + [ + resolveFixturePath('uncaught/unhandled'), + '--unhandled-rejections=warn' + ], + {stdio: 'pipe'} + ); + + return expect( + promise, + 'when fulfilled', + 'to have passed with output', + /UnhandledPromiseRejectionWarning: Error: yikes/ + ); + }); + }); + + describe('when Node is in "strict" mode', function() { + it('should fail', async function() { + const [, promise] = invokeMochaAsync( + [ + resolveFixturePath('uncaught/unhandled'), + '--unhandled-rejections=strict' + ], + {stdio: 'pipe'} + ); + return expect( + promise, + 'when fulfilled', + 'to have failed with output', + /'ERR_UNHANDLED_REJECTION'/ + ); + }); + }); + }); }); diff --git a/test/unit/errors.spec.js b/test/unit/errors.spec.js index 4d28a0420c..9c57e55bc2 100644 --- a/test/unit/errors.spec.js +++ b/test/unit/errors.spec.js @@ -2,6 +2,7 @@ var errors = require('../../lib/errors'); const sinon = require('sinon'); +const {createNoFilesMatchPatternError} = require('../../lib/errors'); describe('Errors', function() { afterEach(function() { @@ -143,4 +144,29 @@ describe('Errors', function() { expect(process.emitWarning, 'was not called'); }); }); + + describe('isMochaError()', function() { + describe('when provided an Error object having a known Mocha error code', function() { + it('should return true', function() { + expect( + errors.isMochaError(createNoFilesMatchPatternError('derp')), + 'to be true' + ); + }); + }); + + describe('when provided an Error object with a non-Mocha error code', function() { + it('should return false', function() { + const err = new Error(); + err.code = 'ENOTEA'; + expect(errors.isMochaError(err), 'to be false'); + }); + }); + + describe('when provided a non-error', function() { + it('should return false', function() { + expect(errors.isMochaError(), 'to be false'); + }); + }); + }); }); From fc0b6cda02c01be0242a48e9e88a7ad0e53d7a75 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Fri, 30 Oct 2020 11:16:24 -0700 Subject: [PATCH 2/2] fix uncaught/strict test Signed-off-by: Christopher Hiller --- test/integration/uncaught.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/uncaught.spec.js b/test/integration/uncaught.spec.js index 9c67dd16a9..0e538d4687 100644 --- a/test/integration/uncaught.spec.js +++ b/test/integration/uncaught.spec.js @@ -198,7 +198,7 @@ describe('uncaught exceptions', function() { }); describe('when Node is in "strict" mode', function() { - it('should fail', async function() { + it('should fail with an uncaught exception', async function() { const [, promise] = invokeMochaAsync( [ resolveFixturePath('uncaught/unhandled'), @@ -210,7 +210,7 @@ describe('uncaught exceptions', function() { promise, 'when fulfilled', 'to have failed with output', - /'ERR_UNHANDLED_REJECTION'/ + /Error: yikes/ ); }); });