This repository has been archived by the owner. It is now read-only.

Async test case continues to execute after assertion failure #396

Closed
dominykas opened this Issue Jan 26, 2014 · 12 comments

Comments

Projects
None yet
5 participants
@dominykas
Member

dominykas commented Jan 26, 2014

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 dwittner added the Bug label Mar 7, 2014

@dwittner dwittner self-assigned this Mar 7, 2014

@dwittner

This comment has been minimized.

Member

dwittner commented Mar 18, 2014

@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

This comment has been minimized.

Member

dominykas commented Mar 18, 2014

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 :)

@dwittner dwittner closed this Mar 18, 2014

@dominykas

This comment has been minimized.

Member

dominykas commented Mar 19, 2014

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

@dwittner

This comment has been minimized.

Member

dwittner commented Mar 19, 2014

@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

This comment has been minimized.

sdepold commented Oct 7, 2014

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

This comment has been minimized.

grncdr commented Oct 7, 2014

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

This comment has been minimized.

Member

dwittner commented Oct 7, 2014

@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

This comment has been minimized.

Member

cjohansen commented Oct 7, 2014

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?

@cjohansen cjohansen reopened this Oct 7, 2014

@dwittner

This comment has been minimized.

Member

dwittner commented Oct 7, 2014

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

This comment has been minimized.

grncdr commented Oct 7, 2014

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 👍

dwittner added a commit that referenced this issue Oct 17, 2014

@dwittner

This comment has been minimized.

Member

dwittner commented Oct 20, 2014

Fixed with version 0.7.15 of Buster.JS.

@dwittner dwittner closed this Oct 20, 2014

@sdepold

This comment has been minimized.

sdepold commented Oct 20, 2014

Hooray!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.