172

The following test is behaving oddly:

it('Should return the exchange rates for btc_ltc', function(done) {
    var pair = 'btc_ltc';

    shapeshift.getRate(pair)
        .then(function(data){
            expect(data.pair).to.equal(pair);
            expect(data.rate).to.have.length(400);
            done();
        })
        .catch(function(err){
            //this should really be `.catch` for a failed request, but
            //instead it looks like chai is picking this up when a test fails
            done(err);
        })
});

How should I properly handle a rejected promise (and test it)?

How should I properly handle a failed test (ie: expect(data.rate).to.have.length(400);?

Here is the implementation I'm testing:

var requestp = require('request-promise');
var shapeshift = module.exports = {};
var url = 'http://shapeshift.io';

shapeshift.getRate = function(pair){
    return requestp({
        url: url + '/rate/' + pair,
        json: true
    });
};
Zoltan Kochan
  • 5,180
  • 29
  • 38
chovy
  • 72,281
  • 52
  • 227
  • 295

4 Answers4

256

The easiest thing to do would be to use the built in promises support Mocha has in recent versions:

it('Should return the exchange rates for btc_ltc', function() { // no done
    var pair = 'btc_ltc';
    // note the return
    return shapeshift.getRate(pair).then(function(data){
        expect(data.pair).to.equal(pair);
        expect(data.rate).to.have.length(400);
    });// no catch, it'll figure it out since the promise is rejected
});

Or with modern Node and async/await:

it('Should return the exchange rates for btc_ltc', async () => { // no done
    const pair = 'btc_ltc';
    const data = await shapeshift.getRate(pair);
    expect(data.pair).to.equal(pair);
    expect(data.rate).to.have.length(400);
});

Since this approach is promises end to end it is easier to test and you won't have to think about the strange cases you're thinking about like the odd done() calls everywhere.

This is an advantage Mocha has over other libraries like Jasmine at the moment. You might also want to check Chai As Promised which would make it even easier (no .then) but personally I prefer the clarity and simplicity of the current version

Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • 4
    In what version of Mocha did this start? I get an `Ensure the done() callback is being called in this test` error when attempting to do this with mocha 2.2.5. – Scott Jun 19 '15 at 19:16
  • 15
    @Scott don't take a `done` parameter in the `it` that would opt out of it. – Benjamin Gruenbaum Jun 19 '15 at 19:16
  • 2
    This was very helpful to me. Removing the `done` in my `it` callback, and explicitly calling `return` (on the promise) in the callback is how I got it working, just like in the code snippet. – User 1058612 Dec 08 '15 at 15:40
  • 5
    Awesome answer, works perfect. Looking back at the docs, it is there -- just easy to miss I guess. `Alternately, instead of using the done() callback, you may return a Promise. This is useful if the APIs you are testing return promises instead of taking callbacks:` – Federico Jan 10 '16 at 20:10
  • 4
    Having the same problem as Scott. I'm not passing a `done` parameter to the `it` call, and this is still happening... –  Jul 09 '16 at 16:56
  • @BenjaminGruenbaum i agree better without chai-as-promised, the extra syntax was one less thing to worry about – timebandit Sep 21 '16 at 13:31
  • 1
    Is there not an issue that the promise may not yield and therefore you don't assert? – thomas-peter Nov 01 '16 at 16:21
  • @thomas-peter if the promise never resolves the test fails after 2 seconds for a timeout. – Benjamin Gruenbaum Nov 01 '16 at 16:22
  • 1
    You might want to start handling promise rejection. Otherwise, with Node 7 you will get this message when rejection appears: `DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.` – toadead Feb 24 '17 at 16:47
  • 1
    Thanks `// note the return` – Cheeso Dec 03 '18 at 22:15
  • 2
    This should also include how to verify that a promise rejects when expected to do so. – Simon East Oct 24 '19 at 06:19
  • 1
    Note also issue https://github.com/mochajs/mocha/issues/2640 - if you fail to include the "return", the test times out, the assertion failure and the timeout are reported in the error output, but the test is wrongly marked as succeeding and you can go for years thinking it actually worked. – Michael Kay Jun 03 '20 at 07:24
  • Although it does now show the error message, it doesn't show what line number the problem was on if something nested in production code fails. I do have an error message that points me to the location, so this is more helpful than fighting with done() – jamesmortensen Sep 18 '20 at 04:30
  • "if you fail to include the "return", the test times out, the assertion failure and the timeout are reported in the error output, but the test is wrongly marked as succeeding and you can go for years thinking it actually worked." LOL I literally came here after discovering this. We had tests passing despite being coded incorrectly for a year. >.> That's why I prefer async/await tests. No need to return. – Matt Welke Dec 02 '20 at 16:14
52

As already pointed out here, the newer versions of Mocha are already Promise-aware. But since the OP asked specifically about Chai, it's only fair to point out the chai-as-promised package which provides a clean syntax for testing promises:

using chai-as-promised

Here's how you can use chai-as-promised to test both resolve and reject cases for a Promise:

var chai = require('chai');
var expect = chai.expect;
var chaiAsPromised = require("chai-as-promised");
chai.use(chaiAsPromised);

...

it('resolves as promised', function() {
    return expect(Promise.resolve('woof')).to.eventually.equal('woof');
});

it('rejects as promised', function() {
    return expect(Promise.reject('caw')).to.be.rejectedWith('caw');
});

without chai-as-promised

To make it really clear as to what's getting tested, here's the same example coded without chai-as-promised:

it('resolves as promised', function() {
    return Promise.resolve("woof")
        .then(function(m) { expect(m).to.equal('woof'); })
        .catch(function(m) { throw new Error('was not supposed to fail'); })
            ;
});

it('rejects as promised', function() {
    return Promise.reject("caw")
        .then(function(m) { throw new Error('was not supposed to succeed'); })
        .catch(function(m) { expect(m).to.equal('caw'); })
            ;
});
Community
  • 1
  • 1
fearless_fool
  • 33,645
  • 23
  • 135
  • 217
  • 6
    The issue with second approach is that `catch` is invoked when one of the `expect(s)` fail. This gives a wrong impression that promise failed even though it didn't. It is only the expect that failed. – TheCrazyProgrammer Jun 08 '17 at 00:48
  • 3
    Holy heck thanks for telling me I have to call `Chai.use` to mount it. I'd never have picked that up from the documentation they had. |:( – Arcym Jun 29 '17 at 03:50
5

Here's my take:

  • using async/await
  • not needing extra chai modules
  • avoiding the catch issue, @TheCrazyProgrammer pointed out above

A delayed promise function, that fails, if given a delay of 0:

const timeoutPromise = (time) => {
    return new Promise((resolve, reject) => {
        if (time === 0)
            reject({ 'message': 'invalid time 0' })
        setTimeout(() => resolve('done', time))
    })
}

//                     ↓ ↓ ↓
it('promise selftest', async () => {

    // positive test
    let r = await timeoutPromise(500)
    assert.equal(r, 'done')

    // negative test
    try {
        await timeoutPromise(0)
        // a failing assert here is a bad idea, since it would lead into the catch clause…
    } catch (err) {
        // optional, check for specific error (or error.type, error. message to contain …)
        assert.deepEqual(err, { 'message': 'invalid time 0' })
        return  // this is important
    }
    assert.isOk(false, 'timeOut must throw')
    log('last')
})

Positive test is rather simple. Unexpected failure (simulate by 500→0) will fail the test automatically, as rejected promise escalates.

Negative test uses the try-catch-idea. However: 'complaining' about an undesired pass happens only after the catch clause (that way, it does not end up in the catch() clause, triggering further but misleading errors.

For this strategy to work, one must return the test from the catch clause. If you want't to test anything else, use another it()-block.

Frank N
  • 9,625
  • 4
  • 80
  • 110
1

Thre is a better solution. Just return the error with done in a catch block.

// ...

it('fail', (done) => {
  // any async call that will return a Promise 
  ajaxJson({})
  .then((req) => {
    expect(1).to.equal(11); //this will throw a error
    done(); //this will resove the test if there is no error
  }).catch((e) => {
    done(e); //this will catch the thrown error
  }); 
});

this test will fail with following message: AssertionError: expected 1 to equal 11

di3
  • 584
  • 3
  • 10