Behavior of after/afterEach hooks with --bail flag

[juergba]

Description

It hasn't been clearly defined yet wether after/afterEach hooks should run after a failing test. Since merging issue #3278 after/afterEach hooks are not beeing executed anymore.
This was a big change in event behavior and quite a few users complained about.
Reading many posts (of maintainers also) I dare to claim that those hooks should run in order to guarantee a correct cleanup.

current pattern with failing test:

• --bail: the end event is triggered twice (which is bug). The first time after/afterEach hooks are not executed, with the second end event they are actually called.
• --bail and --exit: none of the after/afterEach hooks is called.

proposed pattern: (required changes are marked bold)

• before hook:
  • bail: false: failed before hook skips all tests in current suite/subsuites, and jumps to corresponding after hook. Subsequent suites/tests are executed.
  • bail: true: as above, but all subsequent suites/tests are skipped.
• before each hook:
  • bail: false: failed before each hook skips remaining tests in current suite/subsuites, and jumps to corresponding after each hook, then afterhook. Subsequent suites/tests are executed.
  • bail: true: as above, but all subsequent suites/tests are skipped.
• test:
  • bail: false: failed test does not alter execution order.
  • bail: true: failed test jumps to corresponding after each hook, then afterhook. All remaining tests in current suite, subsuites and subsequent suites are skipped.
• after hook:
  • bail: false: failed after hook does not alter execution order.
  • bail: true: failed after hook executes remaining after hooks, then all subsequent suites are skipped.
• after each hook:
  • bail: false: failed after each hook executes remaining after each and after hooks, but skips remaining tests in current suite/subsuites. Subsequent suites/tests are executed.
  • bail: true: as above, but all subsequent suites/tests are skipped.

Description of the Change

1. remove added end event:

Emitting an end event just somewhere in a test tree doesn't work correctly. The runner is not aborted and keeps going, until a second end event (the real one) is emitted.

describe('level A', () => {
    beforeEach(() => {
      console.log('beforeEach A');
    });  
    it('test A should fail', () => {
      throw new Error();
    });
    describe('level A2', () => {
        it('should log "test A2', () => {
            console.log('test A2');
        });
    });
    afterEach(() => {
        console.log('afterEach A');
      });
});

with --bail flag and "debug":

mocha:runner start +1ms
  mocha:runner run suite  +1ms
  mocha:runner run suite level A +4ms
  level A
beforeEach A
    1) test A should fail

  0 passing (7ms)
  1 failing
  1) level A
       test A should fail:
  Error
      at Context.it [...]

  mocha:runner end +10ms                      <<<<<< first end: runner doesn't abort

afterEach A
  mocha:runner run suite level A level A2 +1ms
  mocha:runner finished running +2ms
  mocha:runner end +0ms                       <<<<<< second end

2. Correct hook pattern after failing after each and after hooks:

All remainig after each and after hooks - the entire suite tree upwards - have to be executed.

Applicable issues

#3398
#3598
#3457

[coveralls]

Coverage increased (+0.005%) to 90.608% when pulling 8336eb9 on juergba:issue/3398 into 3d3e833 on mochajs:master.

[boneskull]

Excellent work on this, thanks. I think this is needed for v6.0.0, as the issue it's fixing is pretty severe.

I only have the question/comment on bail.fixture.js. After that's addressed, LGTM!

[boneskull]

was this causing a problem? perhaps your changes should actually be additions; two tests that use done and two that don't. this would give us confidence that the same thing works properly regardless of the Runnable's strategy (async or sync)

[juergba]

no, no problem arose, the 'suite2' describe is skipped (correctly) anyway. I added a second test, one sync and one async.

[boneskull]

This is semver-patch since it effectively reverts semver-major-type changes introduced in #3278, so both are bug fixes.

After this is accepted, let's please evaluate if #3398, #3598, and #3457 are no longer reproducible.

[juergba]

After this is accepted, let's please evaluate if #3398, #3598, and #3457 are no longer reproducible.

All three issues are fixed and work correctly now.

[boneskull]

@juergba thanks! awesome.