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

fixes #1093: missing hooks after failed test with bail #1409

Merged
merged 1 commit into from
Nov 27, 2014
Merged

fixes #1093: missing hooks after failed test with bail #1409

merged 1 commit into from
Nov 27, 2014

Conversation

dasilvacontin
Copy link
Contributor

Issue #1093.

Line 247 in lib/runner.js (which was causing the bug) is pointless:

  • If a test fails and the suite has bail, no more tests will be executed due to line 419. After finishing the hooks for the current failed test and since no more tests will be executed, no more hooks will be executed in the suite.
  • Independently of having bail or not, if beforeAll fails, the suite will be ended due to line 521.
  • If beforeEach fails, runner will proceed with afterEach due to 447. If the suite has bail, no more tests will be ran due to line 419 and the suite will end.
  • If afterEach fails and the suit has bail, the suite will end due to line 419.

All tests pass after removing the line.

I've also added a test that checks that afterEach and afterAll hooks are called after a failed test if the suite has bail. I wasn't sure where to add the test, though. Is it good there? Suggestions?

@dasilvacontin dasilvacontin changed the title fix missing hooks after failed test with bail fixes #1093: missing hooks after failed test with bail Nov 1, 2014
@boneskull
Copy link
Contributor

please squash

@boneskull
Copy link
Contributor

@travisjeffery Would you call this breaking?

@dasilvacontin
Copy link
Contributor Author

Soz, squashed.

@dasilvacontin
Copy link
Contributor Author

It's a bugfix that doesn't affect the API. People shouldn't be relying on afterBefore/afterHooks not being called, since it's a bug, not a feature.

I couldn't find any documentation regarding the behaviour of bail, anyways.

Waiting for @travisjeffery's opinion.

@dasilvacontin
Copy link
Contributor Author

ping @travisjeffery

@boneskull boneskull added status: waiting for author waiting on response from OP - more information needed and removed pr-needs-work labels Nov 21, 2014
@rstacruz
Copy link
Contributor

👍 it's awesome how the fix was just the omission of one line.

travisjeffery pushed a commit that referenced this pull request Nov 27, 2014
fixes #1093: missing hooks after failed test with bail
@travisjeffery travisjeffery merged commit a3454e3 into mochajs:master Nov 27, 2014
@travisjeffery
Copy link
Contributor

i'd consider this a bugfix/patch. awesome, thanks homies!

@dasilvacontin dasilvacontin deleted the fix/missingHooksWithBail branch November 27, 2014 15:09
@dasilvacontin
Copy link
Contributor Author

Cool! Glad to contribute :)

@rstacruz Less code to maintain!

@runk
Copy link
Contributor

runk commented Dec 23, 2014

Looks like those changes aren't in latest release yet. Can someone do npm publish please?

@travisjeffery
Copy link
Contributor

ah yep will push a release tonight

@dasilvacontin
Copy link
Contributor Author

@runk, it shipped now with mocha v2.1.0 :)

@runk
Copy link
Contributor

runk commented Dec 23, 2014

Awesome 👍

@boneskull boneskull removed the status: waiting for author waiting on response from OP - more information needed label Dec 12, 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.

5 participants