Async test case continues to execute after assertion failure

[dominykas]

This will hapilly log "before" and "after":

var buster = require("buster");
var expect = buster.referee.expect;

buster.testCase("async stuff", {
    "should not log two messages": function (done)
    {
        setTimeout(function() {
            console.log("before");
            expect(false).toBeTrue();
            console.log("after");
            done();
        }, 100);
    }
});

If there's no setTimeout(), the testcase will terminate correctly.

I've narrowed it down to here: https://github.com/busterjs/buster-test/blob/master/lib/test-runner.js#L638 - I have no idea why/how that works... [yet]

[dwittner]

@dymonaz, there is a configuration buster.referee.throwOnFailure which is set to false for asynchronous tests https://github.com/busterjs/buster/blob/master/lib/buster/buster-wiring.js#L109. You can set it to true, if you don't like that behaviour, in your case as the first statement of the function you pass to setTimeout. I hope that we will change the default value to true in the near future. I don't see why we should need it to be set to false.

@cjohansen, What speaks against a default value of true?

[dominykas]

Yeah, I eventually found the throwOnFailure piece as well. I think I even tried to trace it back in the history, but I couldn't figure out the reasoning behind it... I didn't bother to work around it - trained myself to ignore it - a failure is a failure in the end. Bad attitude :)

[dominykas]

Hmmm. @dwittner, is this a "won't fix" then?

[dwittner]

@dymonaz, the current behaviour isn't caused by a bug, it is intentional and can be changed by setting buster.referee.throwOnFailure to true. But just because it isn't a bug, doesn't mean we won't change it. To be honest, we had it already changed, but the change was accidentally undone. But as i said before, i hope we will change the behaviour (again).

[sdepold]

IMHO this behavior is overly unexpected and super dangerous especially if your whole test suite is based on an asynchronous setUp which makes all the assertions go silent.

Here is another test case: https://gist.github.com/sdepold/8c991190c6df12bb0f63

[grncdr]

As an added concern, one can't override this behaviour in their test setUp. throwOnFailure is set back to false when setUp calls the done callback. So buster.referee.throwOnFailure must be overridden inside each test case.

[dwittner]

@grncdr, that's really annoying.

I think we also should set buster.referee.throwOnFailure to true for asynchronous tests.

@cjohansen, what do you think?

[cjohansen]

I think this behavior is buggy, and should be fixed. We used to do some trickery and had issues with it, but really I think assertions should probably always throw. Are there any reasons not to?

[dwittner]

I found that information in our docs:

Example: To use referee with JsTestDriver ... ... It is possible to make the default assert.fail method only emit an event and not throw an error. This may be suitable in asynchronous test runners, where you might not be able to catch exceptions. To silence exceptions, see the throwOnFailure property.

So there are reasons, why asynchronous assertions shouldn't throw. But in my opinion, the default should be, that they throw.

[grncdr]

But in my opinion, the default should be, that they throw.

This would make the sense to me, but I really hope that in the end the buster test runner doesn't touch the throwOnFailure property at all. It's better to leave that concern to test authors and/or framework integration authors.

Thanks for re-opening the issue :+1:

[dwittner]

Fixed with version 0.7.15 of Buster.JS.

[sdepold]

Hooray!