Forbid this.skip() within afterAll hooks (#4136)

mochajs · Jan 3, 2020 · 3bd2d28 · 3bd2d28
1 parent 24c22be
commit 3bd2d28
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 32 deletions.
diff --git a/docs/index.md b/docs/index.md
@@ -666,9 +666,9 @@ 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.
+> _Updated in v7.0.0. Skipping a test within an "after all" hook is disallowed and will throw an exception. 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.
+Before Mocha v3.0.0, `this.skip()` was not supported in asynchronous tests and hooks.
 
 ## Retry Tests
 

diff --git a/lib/runner.js b/lib/runner.js
@@ -24,8 +24,9 @@ var sQuote = utils.sQuote;
 var stackFilter = utils.stackTraceFilter();
 var stringify = utils.stringify;
 var type = utils.type;
-var createInvalidExceptionError = require('./errors')
-  .createInvalidExceptionError;
+var errors = require('./errors');
+var createInvalidExceptionError = errors.createInvalidExceptionError;
+var createUnsupportedError = errors.createUnsupportedError;
 
 /**
  * Non-enumerable globals.
@@ -387,12 +388,6 @@ Runner.prototype.hook = function(name, fn) {
       }
       // conditional skip
       if (hook.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_AFTER_EACH) {
           // TODO define and implement use case
           if (self.test) {
@@ -405,14 +400,18 @@ Runner.prototype.hook = function(name, fn) {
           self.emit(constants.EVENT_HOOK_END, hook);
           hook.pending = false; // activates hook for next test
           return fn(new Error('abort hookDown'));
-        } else {
-          // TODO throw error for afterAll
+        } else if (name === HOOK_TYPE_BEFORE_ALL) {
           suite.tests.forEach(function(test) {
             test.pending = true;
           });
           suite.suites.forEach(function(suite) {
             suite.pending = true;
           });
+        } else {
+          hook.pending = false;
+          var errForbid = createUnsupportedError('`this.skip` forbidden');
+          self.failHook(hook, errForbid);
+          return fn(errForbid);
         }
       } else if (err) {
         self.failHook(hook, err);

diff --git a/test/integration/fixtures/pending/skip-sync-after.fixture.js b/test/integration/fixtures/pending/skip-sync-after.fixture.js
@@ -3,9 +3,8 @@
 describe('skip in after', function () {
   it('should run this test-1', function () {});
 
-  after('should print DeprecationWarning', function () {
+  after('should throw "this.skip forbidden"', function () {
     this.skip();
-    throw new Error('never throws this error');
   });
 
   describe('inner suite', function () {

diff --git a/test/integration/pending.spec.js b/test/integration/pending.spec.js
@@ -73,24 +73,23 @@ 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'
-        );
+      it('should throw, but run all tests', function(done) {
+        run('pending/skip-sync-after.fixture.js', args, function(err, res) {
+          if (err) {
+            return done(err);
+          }
+          expect(res, 'to have failed with error', '`this.skip` forbidden')
+            .and('to have failed test count', 1)
+            .and('to have pending test count', 0)
+            .and('to have passed test count', 3)
+            .and(
+              'to have passed test order',
+              'should run this test-1',
+              'should run this test-2',
+              'should run this test-3'
+            );
+          done();
+        });
       });
     });