Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Mocha's duplicate error messages on failure #1167

Conversation

getsnoopy
Copy link
Contributor

Mocha fires a "fail" event for test failures and hook failures, but
fires a "test end" event only for tests. Since we want to handle
hook failures as well, the "fail" event was accounted for in #1045.
But this caused error messages to be logged twice since the handler
code was also firing for the corresonding "test end" events that
are fired for every "fail" event. This commit removes the handler
code from the "test end" event for failed tests so that it logs
errors only once.

Mocha fires a "fail" event for test failures and hook failures, but
fires a "test end" event only for tests. Since we want to handle
hook failures as well, the "fail" event was accounted for in testem#1045.
But this caused error messages to be logged twice since the handler
code was also firing for the corresonding "test end" events that
are fired for every "fail" event. This commit removes the handler
code from the "test end" event for failed tests so that it logs
errors only once.
@getsnoopy
Copy link
Contributor Author

This should properly fix #1129 without causing a regression for #1043. @job13er, can you verify?

@job13er
Copy link
Contributor

job13er commented Aug 14, 2017

Checking now

@job13er
Copy link
Contributor

job13er commented Aug 14, 2017

@getsnoopy It appears that the use-case where testem@1.18.2 was not displaying an error in beforeEach for us is still correctly displaying the error when using your branch here. So I think this is good to go.

👍

@getsnoopy
Copy link
Contributor Author

👍 @johanneswuerbach we should be good to merge

@johanneswuerbach
Copy link
Member

Sorry, thanks for the bump.

@johanneswuerbach johanneswuerbach merged commit 6d46372 into testem:master Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants