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

[s100]

This with Mocha 3.1.2. Source test file:

"use strict";
describe('outer', function() {

  beforeEach(function() {
    console.log("A");
    this.skip();
  });

  describe('inner', function() {
    beforeEach(function() {
      console.log("B");
    });

    it('test case', function() {
      console.log("C");
    });

    afterEach(function() {
      console.log("Y");
    });
  });

  afterEach(function() {
    console.log("Z");
  });
});

Expected output: either "B" and "Y" should be printed, or neither "B" nor "Y" should be printed.

Actual output: "B" is printed, but not "Y":

  outer
    inner
A
B
      - test case


  0 passing (11ms)
  1 pending

In general,

• If I call skip(), then I would not expect that inner beforeEach blocks would be executed.
• If a beforeEach block is executed, then the corresponding afterEach blocks should also be executed, for symmetry.

[ORESoftware]

I do agree that it's strange and probably a bug that Y and Z do not run, but B does. That's just wrong.

However, I think the inner beforeEach will always run unless you call

    it.skip('test case', function() {
      console.log("C");
    });

the this.skip() functionality is fundamentally different than it.skip etc, if you want to know why, I can explain more. It's merely bad naming on the part of Mocha authors, I think.

I don't it's well specified or understand what will happen if you call this.skip() inside a hook. Ideally it would be named, this.continue() or something like that, because all it means is that "we are done here" let's move to the next item.

[nullin]

Looks like a dup of #2286. I'll be great to get #2623 merged as that might solve this too.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[philipwalton]

This is an issue for several projects I work on, so I'd like to see it stay open.

[philipwalton]

Looks like this issue might be related to this line of code:

mocha/lib/runner.js

Lines 671 to 673 in ade8b90

	if (err instanceof Pending) {
	return next();
	}

Which I discovered when reporting this on a related issue here: #2571 (comment)

Commenting out the above code makes this work as expected (for me), but I'm not sure if that code is needed for other features.

[plroebuck]

@juergba could you take a look at this and provide your opinion?

[juergba]

@plroebuck see above, I have a PR pending on this.

Commenting out the above code makes this work as expected

Yes, everybody has his own expectations, so first you would have to define/agree on a hook pattern before changing the code.

[philipwalton]

I think most people assume that, if their beforeEach() hook runs, then their afterEach() hook should also run.

In many cases tests will error if a beforeEach() hook runs and then it runs again without the afterEach() hook running (e.g. stubbing methods and then re-stubbing before unstubbing). This is the case with me, and mocha's current behavior means I have to do all my test reseting in both beforeEach() and afterEach() hooks, just in case.

[plroebuck]

@philipwalton see #3740. We have many identified issues, but it's not as trivial to determine the ramifications of proposed changes against a myriad of possible user settings and configurations (e.g., should it still run "afterEach" if test fails when --bail given).