Skip to content

Commit

Permalink
delegate to Node on non-Mocha unhandled rejections (#4489)
Browse files Browse the repository at this point in the history 
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


Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
  • Loading branch information
@boneskull
boneskull authored Nov 2, 2020
1 parent fac181b commit c3ced39
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 24 deletions.
13 changes: 13 additions & 0 deletions lib/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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,
Expand All @@ -439,5 +451,6 @@ module.exports = {
createNoFilesMatchPatternError,
createUnsupportedError,
deprecate,
isMochaError,
warn
};
44 changes: 34 additions & 10 deletions lib/runner.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
};
}
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions test/integration/fixtures/uncaught/unhandled.fixture.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
it('should emit an unhandled rejection', async function() {
setTimeout(() => {
Promise.resolve().then(() => {
throw new Error('yikes');
});
});
});
69 changes: 55 additions & 14 deletions test/integration/uncaught.spec.js
View file
Original file line number Diff line number Diff line change
@@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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) {
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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 with an uncaught exception', async function() {
const [, promise] = invokeMochaAsync(
[
resolveFixturePath('uncaught/unhandled'),
'--unhandled-rejections=strict'
],
{stdio: 'pipe'}
);
return expect(
promise,
'when fulfilled',
'to have failed with output',
/Error: yikes/
);
});
});
});
});
26 changes: 26 additions & 0 deletions test/unit/errors.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

var errors = require('../../lib/errors');
const sinon = require('sinon');
const {createNoFilesMatchPatternError} = require('../../lib/errors');

describe('Errors', function() {
afterEach(function() {
Expand Down Expand Up @@ -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');
});
});
});
});

0 comments on commit c3ced39

Please sign in to comment.