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

[rainder]

Prerequisites

• [x] Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• [x] 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.
• [x] '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
• [x] 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 https://github.com/mochajs/mocha/pull/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 https://github.com/mochajs/mocha/pull/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.

[plroebuck]

@rainder, but it does work without --exit?

[rainder]

Yes. It does work without exit.

[juergba]

Status quo:

$ node_modules/.bin/mocha test.spec.js

    1) should log "hello world" to the console
hello world
  0 passing (14ms)
  1 failing
  1) test
       should log "hello world" to the console:
  Error
      at Context.it (test\test.spec.js:9:13)

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

    1) should log "hello world" to the console
  0 passing (9ms)
  1 failing
  1) test
       should log "hello world" to the console:
  Error
      at Context.it (test\test.spec.js:9:13)

hello world

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

    1) should log "hello world" to the console
  0 passing (10ms)
  1 failing
  1) test
       should log "hello world" to the console:
  Error
      at Context.it (test\test.spec.js:9:13)

@plroebuck it does not really work without --exit. The text "hello world" is logged to the console because the end event is emitted twice. This is a bug, see issue #3457

• PR #3278 should be backed out
• hook behaviour of --bail should be defined
• in case of major changes in behaviour --> for next major release

[stheine]

I got pointed here after entering #3598, which is the same bug, only described for TDD.

my understanding is that --bail helps me to speed up testing, as it stops after the first error. but still I expect the suiteTeardown/after hooks getting executed to allow me leaving with a clean state.

The weird thing about the current implementation is that --bail and --exit interfere, so that the after hook gets called for --bail alone and for --exit also, but not for --bail --exit together. that's what makes it hard (impossible) to understand the current behavior.

[juergba]

Applying PR3617 Mocha works correctly, the output is the same for:

• without flags
• with --bail
• with --bail --exit

    1) should log "hello world" to the console
hello world
  0 passing (14ms)
  1 failing
  1) test
       should log "hello world" to the console:
  Error
      at Context.it (test\test.spec.js:9:13)