From e87c689f9c007b7a9a05b246fa271382b8577d39 Mon Sep 17 00:00:00 2001
From: Juerg B <44573692+juergba@users.noreply.github.com>
Date: Tue, 2 Apr 2019 23:55:41 +0200
Subject: [PATCH] Deprecate this.skip() for "after all" hooks (#3719)

Print a DeprecationWarning when `this.skip()` is used in `after` hooks.
---
 docs/index.md                                 |  6 +++--
 lib/runner.js                                 | 10 +++++++--
 .../pending/skip-sync-after.fixture.js        | 18 +++++++++++++++
 test/integration/pending.spec.js              | 22 +++++++++++++++++++
 4 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 test/integration/fixtures/pending/skip-sync-after.fixture.js

diff --git a/docs/index.md b/docs/index.md
index 34dd8973a0..0f99a5e39e 100644
--- a/docs/index.md
+++ b/docs/index.md
@@ -626,7 +626,7 @@ Because this test _does nothing_, it will be reported as _passing_.
 
 > _Best practice_: Don't do nothing! A test should make an assertion or use `this.skip()`.
 
-To skip _multiple_ tests in this manner, use `this.skip()` in a "before" hook:
+To skip _multiple_ tests in this manner, use `this.skip()` in a "before all" hook:
 
 ```js
 before(function() {
@@ -662,6 +662,8 @@ describe('outer', function() {
 });
 ```
 
+Skipping a test within an "after all" hook is deprecated and will throw an exception in a future version of Mocha. Use a return statement or other means to abort hook execution.
+
 > Before Mocha v3.0.0, `this.skip()` was not supported in asynchronous tests and hooks.
 
 ## Retry Tests
@@ -910,7 +912,7 @@ Enforce a rule that tests must be written in "async" style, meaning each test pr
 
 ### `--bail, -b`
 
-Causes Mocha to stop running tests after the first test failure it encounters. Corresponding `after()` and `afterEach()` hooks are executed for potential cleanup.
+Causes Mocha to stop running tests after the first test failure it encounters. Corresponding "after each" and "after all" hooks are executed for potential cleanup.
 
 `--bail` does _not_ imply `--exit`.
 
diff --git a/lib/runner.js b/lib/runner.js
index d13e37677b..e41f6c5f97 100644
--- a/lib/runner.js
+++ b/lib/runner.js
@@ -363,9 +363,9 @@ Runner.prototype.hook = function(name, fn) {
     }
     self.currentRunnable = hook;
 
-    if (name === 'beforeAll') {
+    if (name === HOOK_TYPE_BEFORE_ALL) {
       hook.ctx.currentTest = hook.parent.tests[0];
-    } else if (name === 'afterAll') {
+    } else if (name === HOOK_TYPE_AFTER_ALL) {
       hook.ctx.currentTest = hook.parent.tests[hook.parent.tests.length - 1];
     } else {
       hook.ctx.currentTest = self.test;
@@ -388,6 +388,12 @@ Runner.prototype.hook = function(name, fn) {
       }
       if (err) {
         if (err instanceof Pending) {
+          if (name === HOOK_TYPE_AFTER_ALL) {
+            utils.deprecate(
+              'Skipping a test within an "after all" hook is DEPRECATED and will throw an exception in a future version of Mocha. ' +
+                'Use a return statement or other means to abort hook execution.'
+            );
+          }
           if (name === HOOK_TYPE_BEFORE_EACH || name === HOOK_TYPE_AFTER_EACH) {
             if (self.test) {
               self.test.pending = true;
diff --git a/test/integration/fixtures/pending/skip-sync-after.fixture.js b/test/integration/fixtures/pending/skip-sync-after.fixture.js
new file mode 100644
index 0000000000..45c6521f3c
--- /dev/null
+++ b/test/integration/fixtures/pending/skip-sync-after.fixture.js
@@ -0,0 +1,18 @@
+'use strict';
+
+describe('skip in after', function () {
+  it('should run this test-1', function () {});
+
+  after('should print DeprecationWarning', function () {
+    this.skip();
+    throw new Error('never throws this error');
+  });
+
+  describe('inner suite', function () {
+    it('should run this test-2', function () {});
+  });
+});
+
+describe('second suite', function () {
+  it('should run this test-3', function () {});
+});
diff --git a/test/integration/pending.spec.js b/test/integration/pending.spec.js
index b6a7039201..7b96d36001 100644
--- a/test/integration/pending.spec.js
+++ b/test/integration/pending.spec.js
@@ -71,6 +71,28 @@ describe('pending', function() {
       });
     });
 
+    describe('in after', function() {
+      it('should run all tests', function(done) {
+        runMocha(
+          'pending/skip-sync-after.fixture.js',
+          args,
+          function(err, res) {
+            if (err) {
+              return done(err);
+            }
+            expect(res, 'to have passed').and('to satisfy', {
+              passing: 3,
+              failing: 0,
+              pending: 0,
+              output: expect.it('to contain', '"after all" hook is DEPRECATED')
+            });
+            done();
+          },
+          'pipe'
+        );
+      });
+    });
+
     describe('in before', function() {
       it('should skip all suite specs', function(done) {
         run('pending/skip-sync-before.fixture.js', args, function(err, res) {