Deprecate this.skip() for "after all" hooks

[juergba]

Description

There have been long discussions about the behavior of this.skip() in hooks. The before hooks have already been clarified and fixed in #3225. The purpose of this PR is to make a decision about the behavior of after and afterEach hooks.

#2571 (comment)

Skipping a test/suite after running it is a bit counter logical. The only purpose would be marking it as pending. Which is not very clean because you've already emitted pass/fail events for such test. Not sure there's value in changing the test state/outcome post-mortem.

I propose to emit a DeprecationWarning when this.skip() is used in after and afterEach hooks and to remove it in a future release. Besides marking an already passed/failed test as pending, there are actually no obvious effects of calling this.skip() in mentioned hooks.

 describe('outer', function () {
    beforeEach(function () {
        console.log("A");
    });
    describe('inner', function () {
        beforeEach(function () {
            console.log("B");
        });
        it('test case', function () {
            console.log("C");
        });
        it('test case', function () {
            console.log("C2");
        });
        afterEach(function () {          // or after()
            console.log("Y");
            this.skip();                 // has no effect
            console.log("Y2");           // indeed there is an effect, this line is skipped.
        });
    });
    afterEach(function () {
        console.log("Z");
    });
    after(function () {
        console.log("ZZ");
    });
});

  outer
      inner
A
B
C
        √ test case
Y
Z
A
B
C2
        √ test case
Y
Z
ZZ

  2 passing (13ms)

Description of the Change

Print a DeprecationWarning when this.skip() is used in after/afterEach hooks
Print a DeprecationWarning when this.skip() is used in after hooks.

Applicable issues

#3740 partially

[coveralls]

Coverage increased (+0.04%) to 91.73% when pulling e3c9d7a on juergba:juergba/issue/2571 into e654253 on mochajs:master.

[juergba]

@boneskull please tell me wether it makes sense to continue with this PR.
I could add two tests and a short note in the documentation.

[plroebuck]

Think this should go forward. Don't see any reason to logically allow marking a test skipped after its execution.

[boneskull]

I don't see any point to a this.skip() in these hooks. Even if you're using something like --retries, the beforeEach hook is again run, and you could conditionally skip there.

Skipping in "after each" marks the test as pending, which seems problematic. Has the runner already emit failed/passed at this point?

Besides marking an already passed/failed test as pending, there are actually no obvious effects of calling this.skip() in mentioned hooks.

This is not quite correct; this.skip() will abort further execution of the hook (except stuff that's already in the event loop's queue).

Because of this, you may want to advise users to exit from the hook via other means in the deprecation message.

Please add a test.

[juergba]

I don't see any point to a this.skip() in these hooks. Even if you're using something like --retries, the beforeEach hook is again run, and you could conditionally skip there.

--retries has no effect on pending tests, even when it.skip() is used.

Skipping in "after each" marks the test as pending, which seems problematic. Has the runner already emit failed/passed at this point?

Yes, the pass and test end events have already been emitted.

This is not quite correct; this.skip() will abort further execution of the hook (except stuff that's already in the event loop's queue).

You are right, I will recommend the return statement instead in the deprecation message.

[plroebuck]

@juergba, if you do exactly what the review comment says, please comment "done" and press the related "Resolve Conversation" button. If you do something else, explain that in comment and let the other reviewer handle decide about the button.

Makes it easier each time we re-review the code to tell what issues were addressed and which are still outstanding.

[plroebuck]

This isn't annotated as such, but wasn't it intended for the 6.0 milestone? If so, might want to mark it as such so it gets in before they do the release this weekend.

[juergba]

I don't want to delay the release of v6.0.0 by this PR, therefore I haven't set the milestone.
Without @boneskull 's approval, it shouldn't be merged.

[plroebuck]

@boneskull ping! This should get in...

[juergba]

@plroebuck @boneskull @craigtaub please review.
It's a soft deprecation without any change to the program flow. So I would like to have this in v6.1. Thanks

[plroebuck]

some sort of implicit return is definitely not what I would expect.

[juergba]

Really? This "implicit return" was discussed here four weeks ago. It's current state and also described in the documentation. I don't understand what you want now?

[plroebuck]

Pretend I review dozens of PRs here -- I lose track of what's actual and what's proposed, especially if I'm not deeply involved with that area of the code. I do recall discussing implicit return on one of your other PRs; was unaware it was implemented.

It's current state and also described in the documentation.

Where? This in the documentation (L611):

Best practice: To avoid confusion, do not execute further instructions in a test or hook after calling this.skip().

That sentence certainly doesn't imply an implicit return will occur. I find it horrid that this.skip does without using return.

[juergba]

Climb one line up:

It's also important to note that calling this.skip() will effectively abort the test.

If you are terrified by the "implicit return", act accordingly and open your PR.
It's current state and out of scope of this PR.

[plroebuck]

Effectively aborting the test within a before to me only implies setting a flag, not implicit return (since the test has not yet begun execution).

As already stated, the implicit return behavior itself surprised me -- "terrified" is the wrong word -- "stunned" is more accurate. Nowhere else in normal JavaScript coding (to my knowledge) would invoking a function cause implicit return from the caller's function.

Well aware it is outside scope of your PR here.

[plroebuck]

Much of today's conversation has nothing to do with your PR. You do (to my knowledge) possess the most up-to-date knowledge on hook processing, so you're the person most likely to know the answers. Sorry if my questions came off as some sort of "last-minute-redo-this-PR" thing, as that was never my intention.

Was kinda hoping you'd have simply pointed me at the PR that implemented the behavior.

[juergba]

@plroebuck I have re-read this PR and your comments several times.
I don't understand what you want and why you come up with this stuff after four weeks. It's just like you have never read this PR from beginning to the end.
I rebased and forced-pushed this PR today, without any changes. Code and test all the same, since several weeks.
So what exactly are you asking for?

[plroebuck]

@juergba, guess part of this was never having read your fixture closely enough, and not realizing existing behavior of this.skip in a before hook.

Would request documentation changes:

• L611: Denote that invoking this.skip causes implicit return in a hook.
• L616: Denote codefence is antipattern

Realize neither is strictly related to your code changes, so new issue acceptable.

[juergba]

@plroebuck ok, thank you.

L611: I'm not english native, so please just pass me the specific text.

[boneskull]

Thinking about this further, I don't see any use case for a this.skip() in an "after all" hook. There's no further test execution that can occur at that point.
It should be deprecated, 100% agree.

I'm having a tough time with skipping in "after each", however. Consider this:

describe('some filesystem code', function() {
  afterEach(function() {
    if (require('os').type() === 'FooOS') {
      this.skip();
    }
  });

  it('should modify some file', function() {
    // this is run regardless of os
  });
  
  describe('extended file attributes', function() {
    it('should set extended file attributes', function() {
      // does not work on FooOS
    });
  });

  describe('extended permissions', function() {
    it('should set extended permissions', function() {
      // does not work on FooOS
    });
  });
});

Functionally equivalent code using "before each" hooks requires objectively more lines of code:

describe('some filesystem code', function() {
  it('should modify some file', function() {
    // this is run regardless of os
  });
  
  describe('extended file attributes', function() {
    beforeEach(function() {
      if (require('os').type() === 'FooOS') {
        this.skip();
      }
    });

    it('should set extended file attributes', function() {
      // does not work on FooOS
    });
  });

  describe('extended permissions', function() {
    beforeEach(function() {
      if (require('os').type() === 'FooOS') {
        this.skip();
      }
    });

    it('should set extended permissions', function() {
      // does not work on FooOS
    });
  });
});

While this can be mitigated using another nested suite, I suppose I'd rather not force users into doing this if we can help it.

What do you think?

[boneskull]

(As much as we'd like to say "don't rely on the order in which tests are executed", Mocha does execute tests in a certain order and IMO it's unreasonable to believe people won't write test code with that in mind)

[juergba]

In short, what you are saying is:

• already passed/failed tests are no longer set to pending retroactively
• this.skip() affects all subsequent tests in the current suite and all subsuites
• the hook pattern is similar to the one of failing hooks, there is one important difference though. In your example above:
  • when afterEach throws an error, the second and third tests just disappear. They are not run and are not listed in the report. So afterEach runs only once, but affects all three tests.
  • with this.skip(), afterEach runs three times and decides (conditionally) at runtime wether to skip or not. The third skip is senseless since there is no test to come.

[juergba]

If you agree with my previous comment, I would open a new PR for afterEach hook.

With this PR I would like to finish after hook.
The failing hook pattern for after hook states:

Failed after hook does not alter execution order

So this definition would confirm your statement:

It should be deprecated, 100% agree.

I need to know:

1. deprecate this.skip() in after hooks: yes or no ?
2. soft deprecation: just an deprecation warning ?
3. changes to the logic: passed/failed test should not be marked as pending ?

[boneskull]

deprecate this.skip() in after hooks: yes or no ?

yes

soft deprecation: just an deprecation warning ?

yes

changes to the logic: passed/failed test should not be marked as pending ?

no; if a test has already completed execution, it should not be marked pending. only any future tests will be marked as pending. there may not be any future tests, mind you.

this is as much of a limitation of Mocha's architecture as anything, because it might be useful to retroactively mark a failed test as pending. but we can't retroactively change test statuses once they've been emitted to reporters... at least not in a way that doesn't cause wonky stats.

[juergba]

Finally I applied the following changes:

• a soft deprecation of this.skip() in after hooks => prints a deprecation message.
• no other changes have been applied. Tests are still marked as pending retroactively, this behavior will have to be changed later, provided we don't get any negative feedback by users.

Note: afterEach hooks are not affected by this PR.

[boneskull]

this can be output: /"after all" hook is DEPRECATED/ per the semantics of "to satisfy"