What the heck Mocha and bluebird.done() is causing a crash
By: 
Hey Brad I joined a new project and was starting out by fixing some linter issues in our test code when I noticed that when running the full test run the tests just crashed. They were working before I fixed up some linter issues but now I get the error that superagent (the NPM module we use in our tests to make HTTP request to our service we’re testing) failed with a 500 but instead of just failing the test and moving on the process terminated. What gives?
Well you might be suprised to know that I’ve seen the same issue, where superagent requests just crash my tests, turns out it is because I was using the non-standard .done() API provided by both bluebird and Q Promise libraries. In my case the test code it self was using it which is why the crash was only happening in our tests.
The offending test was calling mocha’s done callback instead of just returning the Promise and the issue was that it was invoking the done callback via the bluebird.done(cb) API.
Mocha and Async Testing
I go into more detail about how to test async code with mocha here but in short mocha will assume the test is async if the test function passed as the second argument to it() returns a Promise or if the test function takes a single parameter refereed to as the done callback. If mocha sees a test which does either of these things it will wait for the promise to be resolved or the the done callback to be invoked before moving on to the next test.
My Crashing Test
In the case of the crashing test my team had written it using the done callback form even though it was dealing with promises (probably simply the author didn’t know that they could return a promise or maybe it was not supported at the time; promises were fairly new at the time after all).
How the test was written was to fire off the promise and use the Q or bluebird done() function to call the done callback.
it('should pass', function (done) {
myPromise.doStuff()
.then((result) => {...}) // evaluate result
.done(done);
});
So what’s the Problem?
The issue is how the .done() API works. In both Q and Bluebird if the promise rejects and the rejection is not handled before hitting the .done() API an exception is thrown.
The idea behind the .done() API is that it is used to terminate a promise chain. You use the .done() API to indicate that no further processing will be handled after this point.
Therefore if you’ve not caught and handled any errors by this point your going to get an unhandled rejection. Both libraries take the stance that unhandled rejections should not be ignored and thus will throw exceptions which will crash the node process. Think of .done() like the sealed or final keyword in languages like C#, Java or C++ in that the author is saying this chain can not be extended and that if any rejections are not handled by now something has gone really wrong up stream.
Q
promise.done(onFulfilled, onRejected, onProgress)
Much like then, but with different behavior around unhandled rejection. If there is an unhandled rejection, either because promise is rejected and no onRejected callback was provided, or because onFulfilled or onRejected threw an error or returned a rejected promise, the resulting rejection reason is thrown as an exception in a future turn of the event loop.
This method should be used to terminate chains of promises that will not be passed elsewhere. Since exceptions thrown in then callbacks are consumed and transformed into rejections, exceptions at the end of the chain are easy to accidentally, silently ignore. By arranging for the exception to be thrown in a future turn of the event loop, so that it won’t be caught, it causes an onerror event on the browser window, or an uncaughtException event on Node.js’s process object.
Bluebird
Like .then, but any unhandled rejection that ends up here will crash the process (in node) or be thrown as an error (in browsers). The use of this method is heavily discouraged and it only exists for historical reasons.
To understand why this is causes a crashing issue in the tests you have to remember how mocha works. You can get more information about how mocha works here but basically when the test assertion fails an Error is throw which mocha handles by marking the test as failed. In the case of a test with a Promise the assertion rejects the promise. To handle Promises mocha attaches a .then()/.catch() block to the end of the promise, effectively extending the promise chain, so that it can (a) tell when the test is over and (b) catch any assertion failures so it can mark the test failed. By using the .done() API I was preventing mocha from catching any assertion errors and since I didn’t explicitly add my own catch before the .done() the assertion failure makes it to the .done() which then terminated the process per design.
Conclusion
There is nothing wrong with using .done() at the end of your promise chains if you are really terminating the chain and want any unhandled rejections to crash the process. I think crashing quickly is better then dealing with gremlins if you have unhandled rejections; however, in your tests its not needed. I’d also stay clear of it in any shared code such as services which return promises and if you are going to seal the chain do it in the top levels such as the controller as otherwise it prevents the chain from being extended and used in different ways.
In test code, at least when using mocha, an unhandled rejection on the promise returned in the it() block is handled by mocha and treated as a test failure.
For tests I’d suggested letting mocha handle the rejections instead of having .done() crash the process. At least that way you can complete your test run and view all errors to know what assertion failed which is preferable to hunting down a process crash.
Until next time think imaginatively and design creatively
