Fix hook pattern of 'this.skip()' in beforeEach hooks

[juergba]

Description

#3745 is required for this PR.

The current hook pattern is incorrect. Skipping a test within a beforeEach hook executes all inner beforeEach hooks, but skips afterEach hooks completely.

Some of the afterEach hooks should run to ensure a correct cleanup. This same pattern has already been implemented with failing hooks and also while using the --bail option.

The correct hook pattern should be:

• beforeEach runs for each test individually and determines at runtime wether to skip or not.
  (The failing hook pattern is different: the first hook failure skips all linked tests.)
• since this.skip() aborts hook execution, subsequent inner beforeEach should be skipped ("B").
• skipped beforeEach => skip corresponding afterEach hooks as well ("Y").
• afterEach hooks of the same/higher suite level than the skipping one should run ("Z").

describe('outer', function() {
  beforeEach(function() {
    console.log("A");              // yes, does run
    this.skip();
  });
  describe('inner', function() {
    beforeEach(function() {
      console.log("B");            // yes, does run  => should not run
    });
    it('test case', function() {
      console.log("C");            // pending
    });
    afterEach(function() {
      console.log("Y");            // no, does not run
    });
  });
  afterEach(function() {
    console.log("Z");              // no, does not run  => should run
  });
});

Description of the Change

We abort hookDown() at skipping suite level in order to skip subsequent inner beforeEach hooks. The same skipping suite level is used as entry point for hookUp() to skip corresponding inner afterEach hooks.

Applicable issues

closes #2546
closes #2623 PR
#3740 partially

[plroebuck]

Curious on your thoughts -- outside scope of this PR. Think there's any value to adding a type field to Pending where values would be one of: sync, async, or promise? You're more experienced with this aspect of related code.

[juergba]

Yes, I was thinking about extending Pending with additional properties, like error code or the hook causing the pending-error. I don't know yet, I'm still working on it.
I'm worried about the async version of this.skip(), it has not been working before, now getting worse as a mixture of uncaught error and multiple done calls. (The async skip() already includes a done call.)
Promises? I have just seen sync and async so far. I will have a look.

[coveralls]

Coverage increased (+0.02%) to 92.869% when pulling dc9bf3b on juergba:juergba/issue/2546 into 1412dc8 on mochajs:master.

[juergba]

@boneskull please tell me wether it makes sense to continue with this PR. Thanks.

[boneskull]

@juergba I think what you're proposing sounds good.
Would also like to hear your thoughts about whether this can land in a patch release.

[boneskull]

I've been ignoring this because of the "WIP" in the title. If it's ready to review, please let me know.

[juergba]

Patch release is risky. IMO it certainly is a bug fix, but there have been very few (one?) opened issue about this topic. Either users don't use this.skip() frequently or they are happy with the current hook pattern. I suggest at least minor release.

Anyway there are more hook pattern fixes to come, for this.skip() in it() and afterEach().

Please review, it works, but I need to be confirmed with one or two doubts.

[boneskull]

I think this needs to be semver-major then

[dasilvacontin]

I'm thoroughly reviewing this one. It's taking me a bit due to having stepped away from the code for a while, and because our vars / function names are not very friendly (in runner.js, at least).

[mrstux]

FWIW, this is a major issue for us. We are trying to find a solution, but other than forking mocha and fixing the bug, we're not sure what to do.

We have beforeEach functions which acquire resources, and we have afterEach(done) functions which release them. The tests are predicated on the assumption that if a beforeEach runs, and there is an afterEach, then it will be run, whether the test passes, fails or is skipped.

End of story.

If the afterEach is not run because a test is skipped via this.skip(), then the resources will not be cleaned up, and tests will eventually cascade fail.

I know this is not what this exact PR is about, but you were wondering why you haven't had any other issues reported. Most likely because there is a very good issue, which documents the exact issue, and has a number of PRs in progress. Its only when you dig into the PR that you find some debate about when or if to actually release the fix.

Yes, this will possibly be a "breaking fix", and yes, you may want to update the major semver. your call :)

But please fix!

Thankyou 👍

[juergba]

@mrstux It would be awesome if you could do some test runs with this branch.

@boneskull @craigtaub @outsideris
I will merge this branch within the next two/three weeks. I would like to have this PR published with v7.0.0.

[juergba]

@HarshaNalluru I would highly appreciate some feedback on this PR. Could you do some test runs with this branch?

[HarshaNalluru]

afterEach hooks of the same/higher suite level than the skipping one should run ("Z").

This .skip() behaviour seems to be more appropriate.
Just tested this branch, works as expected. Thanks a lot for fixing this!! 🧡

[juergba]

@HarshaNalluru thank you! 👍