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 to bail option works properly with hooks #3278

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

outsideris
Copy link
Contributor

@outsideris outsideris commented Mar 11, 2018

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.

@outsideris outsideris added semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer labels Mar 11, 2018
@coveralls
Copy link

coveralls commented Mar 11, 2018

Coverage Status

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

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'use strict';

describe('suite1', function () {
it('should only display this error', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@boneskull boneskull added this to the next milestone Mar 18, 2018
@outsideris
Copy link
Contributor Author

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

@plroebuck
Copy link
Contributor

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

@boneskull
Copy link
Contributor

Why?

@boneskull
Copy link
Contributor

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
Copy link
Contributor

plroebuck commented Oct 19, 2018

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
Copy link
Contributor

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

@plroebuck
Copy link
Contributor

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

@outsideris
Copy link
Contributor Author

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
Copy link
Contributor

plroebuck commented Oct 20, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

plroebuck commented Oct 20, 2018

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bail with failing after() runs suite twice
5 participants