Allow async tests to optionally pass a verify fn to done() to improve error reporting

[jeffbski]

Summary

Allow async tests to optionally pass done() a verify fn
(as alternative to just passing an Error object). This verify
function is executed and if any exceptions occur they are
caught and reported rather than only getting a generic timeout
error. Without this fix we have no output about what assert
failed nor do we have any of the details of the failure.
This is an alternate way to use done(), it can also just
receive an Error object as before.

Details of problem

If an assertion exception is thrown in the callback
of an async test, currently mocha only reports a timeout
rather than which assertion failed (and the details of the
failure).

For example with the following code, mocha will not report
the assert failure but the error will just be a timeout.
The details of which assert failed or what throw an error
is not known.

test('foo', function (done) {
  myAsyncFn(function (err, result) {
    assert.ok(true);
    assert.equal(result, 'bar');  // mocha does not report this
    done();
  });
});

One can work around this by wrapping your test assertions in a
try/catch and then calling done with the err, but this is
cumbersome and redundant for such a common pattern.

test('foo', function (done) {
  myAsyncFn(function (err, result) {
    try { // boilerplate to be able to get the assert failures
      assert.ok(true);
      assert.equal(result, 'bar');
      done();
    } catch (x) {
      done(x);
    }
  });
});

Solution - allow done() to optionally take a verify function

This commit allows done() to optionally be passed a verify function
which is executed and if any exceptions or assertions are
thrown then done will be automatically called with the error.
This cleans up the pattern and eliminates most of the
boilerplate, and provides full details to mocha reporter.
If done is passed an Error object it works as it did before,
so this is simply an alternate way of using done().

test('foo', function (done) {
  myAsyncFn(function (err, result) {
    done(function () {  // verify fn
      assert.ok(true);
      assert.equal(result, 'bar');  // mocha now reports the assert failure
    });
  });
});

[jeffbski]

A success and failure test is included along with the modifications which enable this feature.

[tj]

mocha should be able too catch those. is your function catching immediate errors and not passing them to the callback? if mocha is not reporting uncaught exceptions it's a bug

[jeffbski]

I will take a closer look and update this issue with any new info. My asyncFn is completing and calling its callback, but when the asserts fail I am not getting the details only a timeout.

I can comment out that single assert and everything passes like I would expect.

Even just replacing my failing assert with

assert.equal('foo', 'bar')

in my function still only gives a timeout. The async part of my asyncFn comes from using jsdom to create a new window and load jQuery, would that explain anything about why mocha wouldn't be able to catch the async error? (it is not failing in any of that, only in the assert after everything).

Surprisingly if I create a simpler test which just uses process.nextTick then mocha reports things properly as you had mentioned.

So could it be related to using jsdom and its jsdom.jQuerify?

Let me know if you have any ideas. I will update this issue if I can figure anything more out.

Thanks!

[jeffbski]

I guess it is not exclusively a problem with jsdom because when I run the test in the browser (where it doesn't need jsdom) and simply put the code in a setTimeout to make it async, mocha doesn't catch the exception either.

It reports script error in the mocha report and if I open the javascript console, I can see uncaught AssertionError..., so something is preventing it from catching assertion error in the browser too.

[logicalparadox]

Not sure if this is relevant but I remember something similar happening awhile back with 'ScriptError' for async stuff. Here are a few relevant links:

initial report, async script error: https://github.com/logicalparadox/chai/issues/4
mocha's related issue: #165

[jeffbski]

I found that in the browser that even a simple setTimeout assert fail doesn't get reported properly (is script error and in the console.log comes as uncaught exception.

Thanks @logicalparadox, it looks like it might be same type of issue.

if (typeof(chai) === 'undefined') {
  var chai = require('chai');
}

var t = chai.assert;

test('foo', function (done) {
  setTimeout(function () {
    t.equal('foo', 'bar');
    done();
  }, 100);
});

[tj]

@jeffbski I'll take a look.

@logicalparadox I think we still have that ScriptError issue

[tj]

@jeffbski it works for me however I do get the stupid ScriptError thing

[jeffbski]

@visionmedia Do you mean that in the browser you see the assertion details in the mocha report, or do you mean that you only see that the test case failed and the details are Script Error (and you have to go to the developer js console to see the actual details)?

[tj]

@jeffbski I just see ScriptError, but that's a different issue, there's an issue open for it

[jeffbski]

I looked at mocha's code and I would think that process.on('uncaughtException') should catch the exception but unless I manually wrap the asserts with try/catch in node.js 0.6.11 (also tested 0.6.7 and is same) it is not getting passed up like it should (timeout error), and in Chrome 17.0.963.56 OS X it is not caught and is generic Script Error.

PS. Firefox 9.01 and Safari 5.1.2 both do report the assert details properly.

So only if I wrap the callback section in try/catch and pass to done will I get the details in all browsers and node.js

    try {
      assert.equal('foo', 'bar');
      done();
    } catch (x) {
      done(x);
    }

So my done(fn) enhancement just makes that easier, just pass an anonymous function and it is auto wrapped and exception passed to done.

The above becomes simply:

  done(function () {
    assert.equal('foo', 'bar');
  });

@visionmedia Would you consider allowing the done(fn) addition? It does fix all of the above (chrome, node). No more ScriptError you get the assert details as you would expect. I'm out of ideas other than just manually wrapping with try/catch.

[tj]

it works as expected for me in node, chrome is the annoying one, requiring that everyone always performs the assertions in a callback is not ideal

[jeffbski]

To clarify, the simple setTimeout assert error also works for me in node too, but the specific test case where I am using jsdom does not, unless I wrap the asserts. So for some reason, process.on('uncaughtException') is not being called when coming from callback passing through jsdom. I can't see anything in jsdom or my code that would stop process.on('uncaughtException') from being fired, but it isn't.

done(fn) just gives a more surefire approach regardless of any inconsistencies, any assert errors will catch and pass the error.

Thanks for your time and consideration of this.

[perezd]

What happened to this? Its still a very real problem.

[rdingwall]

+1 I am also getting uncaught exceptions from should.xx making all my async tests fail (using 1.0.2). This would be a nice way to fix it. Please merge!

[tj]

Im hesitant to merge because it's a work-around for something that should work (and has worked in my uses)

[rdingwall]

Aha I've tracked it down - it only seems to happen when I'm using the Teamcity reporter. With the HTML reporter, everything works great. Raised as a separate issue #404 (and submitted pull request).

[Almad]

I am running into those too, and often (chai, node, mocha, all up2date).

If it should not be happening...it there a test for that in mocha test suite? :)

[rdingwall]

@Almad what reporter are you using?

[Almad]

@rdingwall spec, but will try with others.

[Almad]

Replicated with dot and tap. Will try to give minimal test case later.

[lightsofapollo]

@visionmedia

The patch seems a bit dated now but I think this should be considered not because of node's error handling but because of how terrible window.onerror is at catching errors (in async tests).

In firefox (and other browsers) once you hit window.onerror thats it you can't get a backtrace or a meaningful Error object.

There are ways to work around this by structuring code in a async before each then running your assertions in a follow up sync "it/test/etc..." block but by introducing the done /w function syntax there will never be a risk of losing the precious stack trace information.

[tj]

@lightsofapollo yeah I agree as far as browser stuff goes.. it's very tempting but it feels very dirty I'm not going to lie haha. Node has subtle issues with this as well that we've seen lately, where the http module has invalid internal state after dropping the stack

[lightsofapollo]

@visionmedia for now I hacked it locally by wrapping the test methods (which works fine) but its basically a huge hack =( is there a better solution that you have in mind? If its not something you wish to support I can release it as a standalone npm module/browser module that adds support by wrapping method that accept done as an argument.

As an alternative what do you think about abstracting "done" further so its possible to swap out the built in "done" method while preserving the current default?

[tj]

you're using AMD within node or some other servers-side env thing?

[lightsofapollo]

(firefox only) browser only in this case. No AMD, alot of the async cases are things like IndexedDB, events, etc...

[tj]

oh sorry wow I was thinking this was a different issue, the --requirejs one hahaha.. my bad

[tj]

I guess my main issue with this is that we're optimizing for the failure cases, which of course happen when testing, but I'd prefer that we don't have to change how we code in order to get reasonable reporting. I dont think this is a terrible solution at all, IMO it beats forcing everyone to use a mocha-specific assertion lib, the only other alternative really is that mocha provides a single .assert(expr, msg) method that others tap into conditionally, then mocha can of course not throw and just deal with things appropriately. "real" uncaught exceptions would still be a problem

[lightsofapollo]

@visionmedia IMO one of my favorite things about mocha is that it uses no assertion libs itself. It has been a huge selling point when advocating the use of mocha.

I just live without the stack in many of my libs ( I have at least 4-5 projects using mocha but only one using the done(fn) syntax). So it would not be a huge deal to live without it and just use it in new project (for me anyway)

[yaniswang]

It's a good idea, but why not merged to master?

I like this:

test('foo', function (done) {
  myAsyncFn(function (err, result) {
    done(function () {  // verify fn
      assert.ok(true);
      assert.equal(result, 'bar');  // mocha now reports the assert failure
    });
  });
});

[uiteoi]

I had the same issue, solved it using a wrapper check(). This is an alternative to the modification of done() and is only slightly more verbose (7 chars including a space) that the proposed solution.

function check( done, f ) {
  try {
    f()
    done()
  } catch( e ) {
    done( e )
  }
}

test( 'foo', function( done ) {
  myAsyncFn( function ( err, result ) {
    check( done, function () {
      assert.ok( true );
      assert.equal( result, 'bar' );  // mocha now reports the assert failure
    } );
  } );
} );

[yaniswang]

Or like this?

test('foo', function (done) {
  myAsyncFn(function (err, result) {
      assert.ok(true);
      assert.equal(result, 'bar');  // mocha now reports the assert failure
  }.dotry(done));
});

[uiteoi]

@yaniswang I love your solution, it prevents the additional function() {} and is very easy to comment out, or remove, if mocha provides a fix that works across all use cases. I'm gonna implement this write away.

[yaniswang]

Function.prototype.dotry = function(){
    var thisFunc = this,
        args = Array.prototype.slice.apply(arguments)
        callback = args.pop(),
        _this = args.shift();
    if(_this !== undefined){
        try{
            thisFunc.apply(_this?_this:window, args);
        }
        catch(e){
            callback(e);
        }
    }
    else{
        return function(){
            try{
                thisFunc.apply(this, arguments);
            }
            catch(e){
                callback(e);
            }
        }
    }
}

try this:

function test(a,b){
    console.log(a,b);
}.dotry(this,'a','b', done);

var func = function test(a,b){
    console.log(a,b);
}.dotry(done);

func('a', 'b');

[bitwiseman]

Next steps on this? Can we either merge it or close it?

[Tolmark12]

Any movement? Seems like a pretty massive issue to just let sit.

[jpbochi]

I'm also interested. Is this going to be merged? Or maybe we need a newer version of the change, so that @visionmedia feels better about merging it? :)

[gzip]

At first I didn't even notice my test failures since they were only logging. Then I struggled with the docs about async tests before finally finding this issue. I ended up with the following which generates a generic and useless "script error", but at least it triggers a failure.

it('should execute callback', function(done) {
    try {
        runAsyncCode(function(err) {
            assert.equal(err, null);
            done();
        });
    } catch(e) {
        done(e);
    }
});

What's the hold up on providing something more elegant? The done(verifyFn) syntax looks good to me.

[park9140]

I was reading this and was having trouble figuring out why this problem stopped happening for me. At first pass it seemed like it would happen all the time. Then I realized that while testing I almost never run tests async. When I am testing async responses I do setup, including waiting for the callback, in a before each. The beforeEach happens async and then I run my tests against the result thus bypassing this issue entirely.

beforeEach(function(done){
  var self = this;
  myAsyncFunc(function(err,result){
    self.err = err;
    self.result = result;
    done();
  });
});
it('should not have an error',function(){
  expect(this.err).to.not.exist
});

Might be overkill for some things where you only have one test after an async call but I find that it sets you up well when you inevitably have to build a couple more test cases based on the same call.

Just another solution to a complex problem. I find it works well in my workflow and keeps my tests down to just expectations more often than not.

I find the alternatives listed reasonably compelling but for some reason they don't feel terribly clean to me and make the tests themselves slightly less readable.

As an alternative has anyone considered a more semantic approach?

given('my async function has been called',
  function(it){
    myAsyncFunction(it('should not have an error', function(err,result){
      expect(err).to.not.exist
    });
  });
});

One benefit being that it could be set up to more clearly describe a future unimplemented async test.

given('my async function has been called',
  function(it){
    it('should not have an error');
  });
});

[jpbochi]

@gzip In order to get a proper failure, you need to put the try-catch block directly inside the callback function passed to runAsyncCode. More specifically, it has to catch the possible exception thrown by your assertion.

[jpbochi]

@park9140, The beforeEach approach that you found by accident is an interesting workaround.

I think the given+it way of defining would be quite a big change. Consider that mocha already supports several ways of defining tests.

For what's worth, I think that the best approach would be one that just worked without any special code. The problem is that it is simply not possible.

Other frameworks like QUnit and Jasmine have their own embedded assertion libraries, so they can catch failed assertions even it they were started asynchronously. Mocha made a design decision of letting the developer choose which assertion library to use. This forces mocha to rely on window.onerror when running tests on browsers.

Unfortunately, window.onerror doesn't sends you the Error itself (which might contain useful information like actual, expected, stack). It sends on the message of the error. Everything else is lost. In some cases, even the message is lost, being replaced by a cryptic "Script Error." message.

[park9140]

@jpbochi, I just think first in bdd style as it is my prefered.

As is I visualize the given+it pattern in this scenario is just a slightly specialized version of the 'test' where thedone function is wrapped in a similar handler to what was proposed above where the done takes the callback function. In the tdd scenario for instance it could look like this.

when('my async function is called', function(test){
  myAsyncFunc(test('that there is no error returned', function(err,result){
    expect(err).to.not.exist;
  });
});

Not being as familiar with tdd descriptors this is probably outright wrong, but it serves as an example of how it can easily be manipulated. Similar solutions can be applied in the other scenarios as well, given the similarity to the done(verifyFunc) scenario.

[jpbochi]

Let me try to summarise the discussion until now.

Problem: Asynchronous assertions might throw exceptions that, on a browser, are caught by window.onerror. This erases extra assertion info (like actual, expected, and expected) to be lost.

Here are the major approaches I see for supporting async assertions.

1. Report assertion failures directly to test runner instead of throwing exceptions. This is, by far, the best approach because no special code on the tests is necessary. QUnit and Jasmine do that because they have their own assertion libraries. Mocha, by design, doesn't. There might be possible to build bridges to adapt existing libraries so that mocha can catch assertion failures asynchronously. Unfortunately, the complexity of such a thing could be very high, or even be impossible.
2. Wrap all async assertions in a try-catch block. People can do it manually or the test library can provide a shortcut for doing so. This PR provides such a shortcut. I have a similar PR with a slightly different syntax. The main problem with this approach is that it forces the developers to put noise on their tests.
3. Move all asynchronous functions to a beforeEach and make the it be synchronous. The advantage is that this is already available right now. The disadvantage is that it constrains a lot the way people can write their tests.

Did I get anything wrong, or missed something?

[tj]

it would be pretty easy to allow assertion libraries to interface with mocha, however that definitely loses some elegance. I'd definitely prefer to stay away from adding these sort of wrapper functions

[gzip]

@jpbochi, makes sense, missed that before. I ended up with something like @uiteoi's example.

@visionmedia, is there anything to be had here then? If nothing else, it would be useful to list this as a known issue in the async docs, along with an example workaround.

[tj]

tough call, I think the best action to take would be to allow these assertion libraries to hook into mocha. I'm not a fan of the done(callback) sort of thing

[jpbochi]

@visionmedia I totally agree that making the assertion libraries hook into mocha beats any other approach.

I've done one attempt to implement it on #985. I would love to hear your thoughts about it.

[travisjeffery]

ok I've merged in #985

[flyingsky]

I cannot find this merge, so I still have this issue when fail to assert in async test.

As another solution, #985 , even I use chai instead nodejs built in assert, i still get this issue.

Any suggestion? Now I just as another work around to wrap all assert into "try...catch", which is too ugly.

[jpbochi]

@flyingsky unfortunately, although #985 was merged, it never got into chai.

I believe you could write a try catch helper function that calls mocha.throwError on the catch block. Or you could try your luck submitting a PR to chai.

[flyingsky]

@jpbochi: i don't think i will be the lucky guy. But why mocha doesn't accept this fix. Actually it doesn't make sense to require other assert library to support mocha.throwError.

[jpbochi]

@flyingsky: As @tj said, it's a tough call.

[hackhat]

I totally agree that making the assertion libraries hook into mocha beats any other approach.

What about other errors like calling an undefined function? - Those will still be swallowed...

I'm using async and await so here is how I've solved:

// Util function
const asyncWrap = (fn) => {
  return async(done)=> {
    try {
      await fn(done);
      done();
    } catch (error) {
      done(error);
    }
  }
};

// Test
it.only('I can add a new student', asyncWrap(async() => {
  // No need to try catch the following code:
  await asyncTask();
  // No need to call done() here
}));

Features:

• It will catch any errors, not only assertion errors;
• No need to add call done() at the end;