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

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

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`.
 

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;

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 () {});
+});

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) {