Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Throw assertion errors for asynchronous tests by default. #338

Merged
merged 2 commits into from Jan 30, 2013

Conversation

Projects
None yet
3 participants
Owner

dwittner commented Jan 29, 2013

The flag buster.assertions.throwOnFailure should be set to true for asynchronous tests as well. Otherwise the flow control is not aborted. Before the error is thrown, a "failure"-event is emitted in any case. Therefore it is guaranteed that the error is tracked.
There should be no need to prevent throwing assertion errors in asynchronous tests.

This would solve the issue #266 test execution does not always stop in async tests after a failed assertion.

@dwittner dwittner Update lib/buster/buster-wiring.js
Throw assertion errors for asynchronous tests by default.
7b44f1c
Owner

cjohansen commented Jan 29, 2013

Given that the only thing the async listener does is to flip the flag, I guess you can just remove those three lines all together?

Owner

dwittner commented Jan 29, 2013

Indeed, the listener is no longer needed. Besides, removing the listener makes it easier for the author of the metioned issue to switch the flag to false for asynchronous tests. ;)

@dwittner dwittner Update lib/buster/buster-wiring.js
Flag buster.assertions.throwOnFailure is no longer changed/updated for asynchronous tests.
b163b1d
Member

meisl commented Jan 29, 2013

What was the rationale behind switching it off for async, at all?

Owner

cjohansen commented Jan 30, 2013

@meisl We discussed this on irc. This was originally done in order to 1) make sure Buster always catches errors, and 2) avoid having assertion errors duplicated as uncaught exceptions. However, under scrutiny these seem to only be speculation. So, as it presents no practical problems we decided to remove it. Now assertions will behave consistently.

@cjohansen cjohansen added a commit that referenced this pull request Jan 30, 2013

@cjohansen cjohansen Merge pull request #338 from dwittner/master
Throw assertion errors for asynchronous tests by default.
862f897

@cjohansen cjohansen merged commit 862f897 into busterjs:master Jan 30, 2013

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