after/afterEach blocks are not executed if both --bail and --exit args are passed

[rainder]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

Mocha does not execute after and afterEach blocks if both --exit and --bail arguments are passed.

Steps to Reproduce

// test.spec.js

describe('test', () => {
  afterEach(() => {
    console.log('hello world');
  });

  it('should log "hello world" to the console', () => {
    throw new Error();
  });
});

Run

$ node_modules/.bin/mocha test.spec.js --exit --bail

Expected behavior:

text "hello world" should be logged to the console

Actual behavior:

the text is not logged to the console

Reproduces how often:

100%

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version:
  mocha is not instaleld globally
  5.2.0 locally

• The output of node --version:
  v9.11.1

• The version and architecture of your operating system:
  macOS High Sierra 10.13.4

• Your shell (bash, zsh, PowerShell, cmd, etc.):
  fish, version 2.4.0

[outsideris]

Although I label it as confirmed-bug, --bail should stop all hooks after first error.
See #3278

[danielmcq]

This is a big change in behavior. My understanding of the reason for the --bail flag was that your test suite wouldn't have to continue if a test fails. However, having an after or afterEach is potentially necessary for cleanup. In a describe block, it's still necessary to do cleanup even if a test fails. I don't get why bailing should skip doing cleanup.

[outsideris]

bail means bail after first test failure. It is much more complex than it looks like.

For example:

• if before hook failed, it should stop all or run other hooks.
• if befroeEach hook failed, it should stop all or run other hooks.
• if afterEach hooks failed, it should stop all or run other hooks.(nested hooks?)
• if after hooks failed, it should report there was 2 errors or 1 errors?

I don't have a strong opinion about this. But, current is the simplest solution as bail's definition.

[rainder]

Not sure how failures of before and beforeEach hooks are related to this issue. I always thought after and afterEach hooks purpose is to do a cleanup after it statements. And bail flag stops running tests after the first failed test. bail does what is supposed to only if --exit is not passed. Why --exit and --bail together are changing the behaviour of mocha - that's the question.

[danielmcq]

It makes sense that the it blocks shouldn't get called if a before or a beforeEach throws an error. However, if any before, beforeEach, or it block successfully executes, the after block should always run before stopping execution of the describe block or the rest of the test suite. Other wise mocha is likely to leave a bunch of mess which potentially affects future test runs. This behavior was changed in #3346 and added to version 5.2.0. This is a major change in behavior, not just a simple fix. If this is supposed to be how mocha works going forward, the change deserves a major version change.

For me, for now, I have to downgrade to 5.1.1 until this gets sorted out or I have the bandwidth to rework a lot of test suites.

[outsideris]

Indeed, it changed in #3278 .
At that time, I didn't fully understand all use cases with --bail.

I want to hear from @mochajs/core

[ArkeologeN]

A soft reminder here - it became a breaking change for us today, as we expect afterEach to make cleanup onto the nested context block.

[plroebuck]

Think #3278 should be backed out.
IMO, it's completely wrong: --bail was not supposed to stop the after or afterEach hooks of the failed test.