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 hook pattern of 'this.skip()' in beforeEach hooks #3741

Merged
merged 2 commits into from
Jan 1, 2020

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Feb 18, 2019

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

plroebuck commented Feb 18, 2019

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

juergba commented Feb 18, 2019

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.

lib/runner.js Outdated Show resolved Hide resolved
lib/runner.js Outdated Show resolved Hide resolved
@juergba juergba self-assigned this Mar 7, 2019
@coveralls
Copy link

coveralls commented Mar 7, 2019

Coverage Status

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

@juergba
Copy link
Contributor Author

juergba commented Mar 7, 2019

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

@boneskull
Copy link
Contributor

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

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

@juergba
Copy link
Contributor Author

juergba commented Mar 8, 2019

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.

@juergba juergba changed the title WIP: this.skip() in beforeEach hooks Fix hook pattern of this.skip() in beforeEach hooks Mar 8, 2019
@boneskull
Copy link
Contributor

I think this needs to be semver-major then

@boneskull boneskull self-requested a review March 14, 2019 19:40
@juergba juergba added semver-major implementation requires increase of "major" version number; "breaking changes" status: needs review a maintainer should (re-)review this pull request labels Apr 22, 2019
@dasilvacontin
Copy link
Contributor

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

mrstux commented Jul 5, 2019

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 juergba added this to the v7.0.0 milestone Dec 18, 2019
@juergba juergba added the type: bug a defect, confirmed by a maintainer label Dec 18, 2019
@juergba
Copy link
Contributor Author

juergba commented Dec 18, 2019

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

juergba commented Dec 22, 2019

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

@HarshaNalluru
Copy link

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

juergba commented Dec 24, 2019

@HarshaNalluru thank you! 👍

@juergba juergba merged commit 24c22be into mochajs:master Jan 1, 2020
@juergba juergba deleted the juergba/issue/2546 branch January 1, 2020 09:01
@juergba juergba removed the status: needs review a maintainer should (re-)review this pull request label Jan 1, 2020
@juergba juergba changed the title Fix hook pattern of this.skip() in beforeEach hooks Improve hook pattern of 'this.skip()' in beforeEach hooks Apr 6, 2020
@juergba juergba changed the title Improve hook pattern of 'this.skip()' in beforeEach hooks Fix hook pattern of 'this.skip()' in beforeEach hooks Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test case skip()ed, beforeEach() hook still executed, afterEach() missed
7 participants