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: ofrobots@712678c. @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.

[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.

[juergba]

In Mocha v8.2.0 we added an unhandledRejection listener.
Going to close this issue.

[pie-r]

@juergba To get unhandledRejection in the xxxx.test.ts at the beginning I need to add the following rows in order to grab the Exception raised in any async function.

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

E.G. In the following example mocha doesn't print anything. [email protected]

private async XXX() {
    throw new Error('fooo error');

It seems Mocha doesn't handle this type of event and silently hides them. Is the expected behavior?

[eyalroth]

@juergba Please reopen this issue. The faulty behavior was reintroduced in 8.2.1 - #4489

[juergba]

@eyalroth You would have to be more precise in order to know what is your "faulty behavior".
I have not implemented PR4489.

[eyalroth]

@juergba The same faulty/unwanted behavior described in this issue and fixed in 8.2.0 or earlier; i.e, that mocha silently ignores unhandled promise rejections that occur in tests instead of failing the tests.

[the-spyke]

Was debugging this issue. All Mocha does on unhandled rejections is just

process.emit('unhandledRejection', reason, promise);

This is useless and the issue must be reopen.

I could try to mitigate this by throwing myself in process.on('unhandledRejection'), which will lead to Mocha going into uncaught route, but it is a hack.

[GCSBOSS]

I seem to have this same issue in 10.2.0

[killagu]

Use this env NODE_OPTIONS=--unhandled-rejections=strict can convert unhandledRejection event to uncaughtException .