fix to bail option works properly with hooks

[outsideris]

Description of the Change

With bail option, all tests must be stopped after first test failure. However, bail option doesn't stop hooks such as after or afterEach.

Alternate Designs

I have no idea.

Why should this be in core?

It's a bug. It makes bail works correctly.

Benefits

A user can use bail intentionally.

Possible Drawbacks

No.

Applicable issues

Fix #3096 .
It is patch release.

[coveralls]

Coverage increased (+0.05%) to 90.086% when pulling e798eec on outsideris:issue-3096 into 0060884 on mochajs:master.

[boneskull]

Awesome, thanks. I only have one nitpick. If you agree, please fix & go ahead and merge this.

[boneskull]

please remove the done parameter in this function and below; they aren't necessary for what these tests are proving.

[outsideris]

I removed done parameter. I will merge this after confirm that CI passed.

[plroebuck]

IMO, this should be backed out. Believe it's completely wrong.

[boneskull]

Why?

[boneskull]

bail should call those events you mentioned. if it’s not, I agree it’s wrong. past that, whether more hooks should run is a matter of opinion. some would prefer an immediate process exit with exception, others would like cleanup. but fwiw I’m feeling wishy-washy on it

[plroebuck]

I think:

• if either before/beforeEach fail, don't run test and die.
• if test fails, afterEach/after should be run, then die.

If we manage to do the setup, we should be responsible for the teardown.

[boneskull]

Was this fix to #3096 overkill, then? @outsideris?

[plroebuck]

IMO, it definitely doesn't seem to be semver-patch either.
The "fix" has major ramifications to event logic.

[outsideris]

When I try to fix bail issues, I just treated them like a bug. Now, I realized many people have different expectationsΩ for bail option. However, I still didn't understand why people want to clean up them when they run mocha with bail.

I think current bail's behavior is fit for the definition.

Only interested in the first exception? use --bail!

If we run after/afterEach, we should report multiple exceptions not only first exception.

[plroebuck]

However, I still didn't understand why people want to clean up them when they run Mocha with bail.

If you launch a background server (in setup), it will be using a port. If you terminate without shutting the server down, that port will still be in use. Now you can't trivially restart your test without going back to shell to determine the server pid to kill it manually. Would make --watch (or your nodemon equivalent) rather cumbersome to use, no?

If we run after/afterEach, we should report multiple exceptions not only first exception.

That would be the best case scenario, but I'm realistic about it. The point was to try to recover.
If we just can't handle the cascading errors, then let's eulogize in the documentation.

[outsideris]

Why the case which you provide is handled by --bail? Without --bail, there is no problem.
I think a purpose of --bail is prechecking for all tests are passed before deployment or publishing like there is no reason to proceed further if there is a failure already.
Please tell me If I missed something. I don't use --bail heavily.

[plroebuck]

I don't use --bail much either. But I can definitely see it potentially being used with --watch during refactoring. One purpose is prechecking, but there are others...

[outsideris]

Users of two cases, only --bail and --bail with --watch have different needs. I guess former case want to stop running at first exception and latter case want to run clean up.
I'm not sure how we handle both cases gracefully in `--bail.