Mocha: unhandledRejection

[stevenvachon]

These should be handled automatically, as to reduce boilerplate. A common issue as a result of this is #1128.

process.on('unhandledRejection', (reason, promise) => throw promise);

For web browsers, we could use window.addEventListener.

[ScottFreeCode]

This is definitely something we should add, since the current timeout behavior is quite uninformative and generally promise implementations support this hook; but let's make sure the resulting test failure message indicates that these errors would have been swallowed by the promise in production and should be somehow handled or communicated to the caller for handling (at which point it's easy for the test to communicate the failure back to Mocha without relying on unhandled rejection functions) -- a lot of the questions we get about the current behavior come from lack of awareness of the need for catch.

[ScottFreeCode]

One clarification to my earlier comment: I was thinking specifically of unhandled promise rejections -- I believe Mocha already catches uncaught non-promise-swallowed exceptions?

[stevenvachon]

Yes, I think that's true.

[rogierschouten]

+1 We were completely caught by surprise that mocha catches thrown errors but not unhandled rejections. Our tests passed when actually they should have failed.

describe("normal test", () => {
  it("bar", () => {
    Promise.reject(new Error("baz"));    
  });
  it("bar", () => {
    // nothing
  });
});

This is a synchronous test which happens to result in an unhandled rejection (suppose e.g. the code under test had an unintended floating promise). In this case, testing results in "foo" and "bar" succeeding and exit code 0.

You cannot prevent "foo" from succeeding since node will need a couple of ticks to decide that a promise is unhandled. However I would never expect exit code 0. If necessary, manually go through a couple of node cycles at the end of a mocha execution to wait for any unhandled rejections and then exit with an error message and non-zero code

Current work-around:

let unhandledRejectionExitCode = 0;

process.on("unhandledRejection", (reason) => {
    console.log("unhandled rejection:", reason);
    unhandledRejectionExitCode = 1;
    throw reason;
});

process.prependListener("exit", (code) => {
    if (code === 0) {
        process.exit(unhandledRejectionExitCode);
    }
});

[boneskull]

This is what I think needs to happen:

1. An unhandled rejection needs to result in the same (or similar) behavior as when an uncaught exception is thrown
2. A new flag, --allow-unhandled, should be added (if we piggybacked on --allow-uncaught, it would be more difficult to pin down either one in particular)
3. Plenty of tests.

Since Mocha doesn't use Promises internally, this shouldn't be a particularly severe change to the codebase. I think we can get away with basically copy-and-pasting the majority of the "uncaught exception" behavior, as well as the test suites.

There are likely some edge cases here I'm not considering, so this is best-case scenario. 😇

[lrowe]

Not sure if this is a separate issue or not, but I see similar behaviour without promises when deferring an error with setImmediate or process.nextTick.

// swallow.js
it("swallows deferred error", function() {
  setImmediate(() => { throw new Error("thrown error"); });
});

Running the test I get success with exit code 0.

$ node bin/mocha swallow.js


  ✓ swallows deferred error

  1 passing (5ms)

Whereas running the same code with node v8.9.0 gives prints the stack trace and exits non-zero:

$ node -e 'setImmediate(() => { throw new Error("thrown error"); });'
[eval]:1
setImmediate(() => { throw new Error("thrown error"); });
                     ^

Error: thrown error
    at Immediate.setImmediate [as _onImmediate] ([eval]:1:28)
    at runCallback (timers.js:789:20)
    at tryOnImmediate (timers.js:751:5)
    at processImmediate [as _immediateCallback] (timers.js:722:5)

[rogierschouten]

@lrowe Your example is expected behaviour - if you have an a-synchonous test then you should use a callback function or a promise-returning function. When you do, it will be ok.

[lrowe]

@rogierschouten even as an async test I see the same behaviour since the error thrown is not part of the promise chain:

it("swallows deferred error", function(done) {
  setImmediate(() => { throw new Error("thrown error"); });
  done();
});

Modifying my example to log when the error is thrown and with logging added to Runner.prototype.run's uncaughtException handler:

// swallow.js
it("swallows deferred error", function() {
  setImmediate(() => {
    console.log("throwing error");
    throw new Error("thrown error");
  });
});

We see:

$ node bin/mocha swallow.js 


  ✓ swallows deferred error
throwing error
inside Runner's uncaughtException handler

  1 passing (5ms)

removing uncaughtException handler on Runner's end event 

I understand why the test would pass in this instance, but it seems wrong for the mocha Runner.prototype.uncaught to silently // Ignore errors if complete or pending so I would expect the error to be reported and Mocha to exit with a non-zero status.

My example is a minimal reproducer from using TC39 proposal Observables in a Mocha test. Exceptions raised by the supplied observer methods are specified to report errors asynchronously to the host so that cleanup logic may first be run.

[rogierschouten]

@lrowe that is because it's your responsibility to call the done callback only when the test is finished. You're supposed to call it with the error object from within the setimmediate.

[rogierschouten]

@lrowe I re-read your post and now I get your point. Sorry for my too quick response

[nathany]

Here is another example of an UnhandledPromiseRejectionWarning that didn't fail the build.

Using a combination of Mocha, Chai, and Sinon, we had an expect(...).to.be.rejectedWith(error) expression that wasn't returned (return expect(...), and therefore logging an UnhandledPromiseRejectionWarning.

Once I added the return, I got an Cannot read property 'equal' of undefined error which was due to completely invalid usage in the then() block.

expect(fun.callCount.to.equal(1));

should have been:

expect(fun.callCount).to.equal(1);

Not sure how long we had an invalid test, but it was only by looking through the test log that we came upon UnhandledPromiseRejectionWarning.

For now, I've added this small library in the test suite to exit the process for UnhandledPromiseRejectionWarning. https://github.com/mcollina/make-promises-safe

It would be nice if the test suite continued running, so that other test failures would be shown, while still failing the build. Thanks!

[ofrobots]

I have a WIP commit that implements unhandled rejection support: https://github.com/ofrobots/mocha/commit/712678ccd3605ed95b60072a1d6059052c4f9a6b. @boneskull If this is in line with what you had in mind in this comment I can go ahead and finish this off, write tests and open a PR.

This would be a semver major change IMO, so perhaps a --disallow-unhandled makes more sense than --allow-unhandled?

[boneskull]

@ofrobots yes, please. --allow-unhandled is fine, we can put it into next major.

[inf3rno]

Note that this problem is even worse if you want it to be cross-browser. For example

new Promise((resolve, reject) => {
    throw new Error("bad");
});

logs the unhandled rejection in the console, while

Promise.resolve().then(() => {
    throw new Error("bad");
});

does not do anything in all of the browsers except Chrome and Opera, which are Chromium based. Both can be caught with .catch(), so the rejection is there, it just does not arrive to the console. Not even in Edge, which supports unhandled rejections according to MDN. I already sent a bug report about this to Firefox, but it feels like if we were 20 years ago, the core js implementation is breaking in different browsers again. :S I really hate client side js because of the constant bugfixing, workarounds and shims. :S

edit:

A simple fix for Chrome until we get unhandled rejection support in Mocha:

window.addEventListener("unhandledrejection", function (event){
    throw event.reason;
});

[plroebuck]

@ofrobots, ever finish that WIP PR?

[ofrobots]

I didn't get a chance to work on it more :(. Feel free to take the WIP commit linked above and to convert it to a PR.

[benjamingr]

In 1 week this shouldn't matter as this is the default behavior in Node 15 🎉 :]

[stevenvachon]

@benjamingr and web browsers?

[benjamingr]

@stevenvachon it doesn't make sense for browsers to exit the process on unhandled rejections in my opinion just like browsers don't exit on uncaught exceptions.

I would assume (and hope!) that the browser counterpart will never happen and browsers will not crash on unhandled rejections.

That said: browsers don't (and never did) support the unhandledRejection specification - they support a separate event that is in line with the browser event model (on window and not process, with an Event and so forth).

[stevenvachon]

@benjamingr but Mocha should still fail the same within a web browser; per this thread's original comment.