The "Full-app integration test" can simply return its promise from the test
Closed this issue · 6 comments
In the Full-app integration test example, you don't need to use done and then convert your promise to a callback to call done.
Instead, you can simply return the promise from your beforeDone and test functions.
http://mochajs.org/#working-with-promises
Returning a promise from a test will make the test asynchronous (in the same way that passing in the done argument does), and the test runner will wait for the returned promise to resolve or reject before moving on.
What if somehow the code branches in a way that doesn't return anything? Will it just silently pass? Right now, when you add done as an argument, mocha understands that it should wait for that to be called, so you can't accidentally make it pass anyway.
I'd be a bit worried if just not returning a promise would make the test pass.
Sadly that won't help with most cases where you forget to return a promise.
do_one_thing()
.then(() => {
another_thing(); // oops, missing return statement here
})
.nodeify(done);
Now you still have the same problem: the test silently passes, even if another_thing() fails.
The right solution to this issue is to use a promise library such as Bluebird which:
- If a dangling promise is rejected, displays the error instead of silently ignoring it.
- Automatically detects when a promise was created in a handler but none were returned.
Now if you write the test incorrectly it can still incorrectly pass (which can happen with any incorrect test you write), but it won't silently pass: you'll get a warning or error printed to the console where you can see that there's a problem you need to fix.
P.S. If you'd like to specifically ensure that you return a promise from an async test, the right way to do that is to write an async_it function to do that check for you:
function async_it(title, f) {
it(title, function () {
var result = f();
if (! (result && typeof result.then === 'function'))
throw new Error('async test did not return a promise');
return result;
});
}
and then instead of
it('has all public lists at homepage', function () { ... });
use
async_it('has all public lists at homepage', function () { ... });
You don't need to do this if you use Bluebird and watch for warnings (it will tell you if you have a rejected promise that isn't being reported in the test runner), but it is a nice way to clearly indicate which tests are intended to be async.
Ah, I just had a better idea than writing a special async_it.
A principle of writing promise-based code is that a function which has an API of returning a promise should never throw. If it has any kind of error, it should return a promise which rejects on the error. This allows the caller to work with the returned promise, without in addition also having to surround the call to the function with a try...catch.
If we have code like
function do_stuff() {
return step_one()
.then(function () {
return step_two();
});
}
This doesn't fulfill the principle because if you misspell step_one or step_one throws an error, the function throws instead of returning a promise that rejects.
The recommended practice is to always start your promise chain with a Promise.try
function do_stuff() {
return Promise.try(function () {
return step_one();
})
.then(function () {
return step_two();
});
}
Now any errors thrown will be converted into a rejected promise.
If we use this principle in Mocha tests
it('my async test', function () {
return Promise.try(function () {
...
});
});
We're guaranteed to be returning a promise so Mocha knows it's an async test.
And it has the advantage that the guideline "always start your promise chain with a Promise.try" is universally applicable to all your promise code, while the suggestion for async_it is a special case just for Mocha.
Ooh! I really like this. I'm 100% in favor of changing over to this approach. If someone sees this issue, please file a PR to improve this. Perhaps I will get around to it later.