Improve hook pattern of 'this.skip()' in beforeAll (mochajs#4223)

lib/runnable.js

@@ -295,6 +295,8 @@ Runnable.prototype.run = function(fn) {
   var finished;
   var emitted;
 
+  if (this.isPending()) return fn();
+
   // Sometimes the ctx exists, but it is not runnable
   if (ctx && ctx.runnable) {
     ctx.runnable(this);
@@ -376,11 +378,7 @@ Runnable.prototype.run = function(fn) {
 
   // sync or promise-returning
   try {
-    if (this.isPending()) {
-      done();
-    } else {
-      callFn(this.fn);
-    }
+    callFn(this.fn);
   } catch (err) {
     emitted = true;
     if (err instanceof Pending) {
@@ -481,7 +479,11 @@ var constants = utils.defineConstants(
     /**
      * Value of `state` prop when a `Runnable` has passed
      */
-    STATE_PASSED: 'passed'
+    STATE_PASSED: 'passed',
+    /**
+     * Value of `state` prop when a `Runnable` has been skipped by user
+     */
+    STATE_PENDING: 'pending'
   }
 );
 

lib/runner.js

@@ -18,6 +18,7 @@ var HOOK_TYPE_BEFORE_ALL = Suite.constants.HOOK_TYPE_BEFORE_ALL;
 var EVENT_ROOT_SUITE_RUN = Suite.constants.EVENT_ROOT_SUITE_RUN;
 var STATE_FAILED = Runnable.constants.STATE_FAILED;
 var STATE_PASSED = Runnable.constants.STATE_PASSED;
+var STATE_PENDING = Runnable.constants.STATE_PENDING;
 var dQuote = utils.dQuote;
 var ngettext = utils.ngettext;
 var sQuote = utils.sQuote;
@@ -122,8 +123,7 @@ module.exports = Runner;
  * @public
  * @class
  * @param {Suite} suite Root suite
- * @param {boolean} [delay] Whether or not to delay execution of root suite
- * until ready.
+ * @param {boolean} [delay] Whether to delay execution of root suite until ready.
  */
 function Runner(suite, delay) {
   var self = this;
@@ -285,11 +285,13 @@ Runner.prototype.checkGlobals = function(test) {
  * Fail the given `test`.
  *
  * @private
- * @param {Test} test
+ * @param {Runnable} test
  * @param {Error} err
+ * @param {boolean} [force=false] - Whether to fail a pending test.
  */
-Runner.prototype.fail = function(test, err) {
-  if (test.isPending()) {
+Runner.prototype.fail = function(test, err, force) {
+  force = force === true;
+  if (test.isPending() && !force) {
     return;
   }
 
@@ -386,7 +388,7 @@ Runner.prototype.hook = function(name, fn) {
       });
     }
 
-    hook.run(function(err) {
+    hook.run(function cbHookRun(err) {
       var testError = hook.error();
       if (testError) {
         self.fail(self.test, testError);
@@ -412,6 +414,7 @@ Runner.prototype.hook = function(name, fn) {
           suite.suites.forEach(function(suite) {
             suite.pending = true;
           });
+          hooks = [];
         } else {
           hook.pending = false;
           var errForbid = createUnsupportedError('`this.skip` forbidden');
@@ -533,9 +536,6 @@ Runner.prototype.runTest = function(fn) {
     test.asyncOnly = true;
   }
   test.on('error', function(err) {
-    if (err instanceof Pending) {
-      return;
-    }
     self.fail(test, err);
   });
   if (this.allowUncaught) {
@@ -634,10 +634,9 @@ Runner.prototype.runTests = function(suite, fn) {
     // static skip, no hooks are executed
     if (test.isPending()) {
       if (self.forbidPending) {
-        test.isPending = alwaysFalse;
-        self.fail(test, new Error('Pending test forbidden'));
-        delete test.isPending;
+        self.fail(test, new Error('Pending test forbidden'), true);
       } else {
+        test.state = STATE_PENDING;
         self.emit(constants.EVENT_TEST_PENDING, test);
       }
       self.emit(constants.EVENT_TEST_END, test);
@@ -650,10 +649,9 @@ Runner.prototype.runTests = function(suite, fn) {
       // conditional skip within beforeEach
       if (test.isPending()) {
         if (self.forbidPending) {
-          test.isPending = alwaysFalse;
-          self.fail(test, new Error('Pending test forbidden'));
-          delete test.isPending;
+          self.fail(test, new Error('Pending test forbidden'), true);
         } else {
+          test.state = STATE_PENDING;
           self.emit(constants.EVENT_TEST_PENDING, test);
         }
         self.emit(constants.EVENT_TEST_END, test);
@@ -674,10 +672,9 @@ Runner.prototype.runTests = function(suite, fn) {
         // conditional skip within it
         if (test.pending) {
           if (self.forbidPending) {
-            test.isPending = alwaysFalse;
-            self.fail(test, new Error('Pending test forbidden'));
-            delete test.isPending;
+            self.fail(test, new Error('Pending test forbidden'), true);
           } else {
+            test.state = STATE_PENDING;
             self.emit(constants.EVENT_TEST_PENDING, test);
           }
           self.emit(constants.EVENT_TEST_END, test);
@@ -714,10 +711,6 @@ Runner.prototype.runTests = function(suite, fn) {
   next();
 };
 
-function alwaysFalse() {
-  return false;
-}
-
 /**
  * Run the given `suite` and invoke the callback `fn()` when complete.
  *
@@ -850,9 +843,7 @@ Runner.prototype.uncaught = function(err) {
     return;
   } else if (runnable.isPending()) {
     // report 'pending test' retrospectively as failed
-    runnable.isPending = alwaysFalse;
-    this.fail(runnable, err);
-    delete runnable.isPending;
+    this.fail(runnable, err, true);
     return;
   }
 

test/integration/fixtures/pending/skip-async-before-hooks.fixture.js

@@ -1,59 +1,72 @@
 'use strict';
+var assert = require('assert');
 
-describe('outer suite', function () {
-
-  before(function () {
-    console.log('outer before');
+describe('outer suite', function() {
+  var runOrder = [];
+  before(function() {
+    runOrder.push('outer before');
   });
 
-  it('should run this test', function () { });
-
-  describe('inner suite', function () {
+  it('should run test-1', function() {
+    runOrder.push('should run test-1');
+  });
 
-    before(function (done) {
-      console.log('inner before');
+  describe('inner suite', function() {
+    before(function(done) {
+      runOrder.push('inner before');
       var self = this;
-      setTimeout(function () {
+      setTimeout(function() {
         self.skip();   // done() is not required
       }, 0);
     });
 
-    beforeEach(function () {
-      throw new Error('beforeEach should not run');
+    before(function() {
+      runOrder.push('inner before-2 should not run');
     });
 
-    afterEach(function () {
-      throw new Error('afterEach should not run');
+    beforeEach(function() {
+      runOrder.push('beforeEach should not run');
     });
 
-    it('should not run this test', function () {
-      throw new Error('inner suite test should not run');
+    afterEach(function() {
+      runOrder.push('afterEach should not run');
+    });
+
+    after(function() {
+      runOrder.push('inner after');
     });
 
-    after(function () {
-      console.log('inner after');
+    it('should not run this test', function() {
+      throw new Error('inner suite test should not run');
     });
 
-    describe('skipped suite', function () {
-      before(function () {
-        console.log('skipped before');
+    describe('skipped suite', function() {
+      before(function() {
+        runOrder.push('skipped suite before should not run');
       });
 
-      it('should not run this test', function () {
+      it('should not run this test', function() {
         throw new Error('skipped suite test should not run');
       });
 
-      after(function () {
-        console.log('skipped after');
+      after(function() {
+        runOrder.push('skipped suite after should not run');
       });
     });
-
   });
 
-  it('should run this test', function () { });
-
-  after(function () {
-    console.log('outer after');
+  it('should run test-2', function() {
+    runOrder.push('should run test-2');
   });
 
+  after(function() {
+    runOrder.push('outer after');
+    assert.deepStrictEqual(runOrder, [
+      'outer before', 
+      'should run test-1', 'should run test-2',
+      'inner before', 'inner after',
+      'outer after'
+    ]);
+    throw new Error('should throw this error');
+  });
 });

test/integration/fixtures/pending/skip-sync-before-hooks.fixture.js

@@ -1,57 +1,69 @@
 'use strict';
+var assert = require('assert');
 
-describe('outer suite', function () {
-
-  before(function () {
-    console.log('outer before');
+describe('outer suite', function() {
+  var runOrder = [];
+  before(function() {
+    runOrder.push('outer before');
   });
 
-  it('should run this test', function () { });
+  it('should run test-1', function() {
+    runOrder.push('should run test-1');
+  });
 
-  describe('inner suite', function () {
-    before(function () {
+  describe('inner suite', function() {
+    before(function() {
+      runOrder.push('inner before');
       this.skip();
     });
 
-    before(function () {
-      console.log('inner before');
+    before(function() {
+      runOrder.push('inner before-2 should not run');
     });
 
-    beforeEach(function () {
-      throw new Error('beforeEach should not run');
+    beforeEach(function() {
+      runOrder.push('beforeEach should not run');
     });
 
-    afterEach(function () {
-      throw new Error('afterEach should not run');
+    afterEach(function() {
+      runOrder.push('afterEach should not run');
     });
 
-    after(function () {
-      console.log('inner after');
+    after(function() {
+      runOrder.push('inner after');
     });
 
-    it('should never run this test', function () {
+    it('should never run this test', function() {
       throw new Error('inner suite test should not run');
     });
 
-    describe('skipped suite', function () {
-      before(function () {
-        console.log('skipped before');
+    describe('skipped suite', function() {
+      before(function() {
+        runOrder.push('skipped suite before should not run');
       });
 
-      it('should never run this test', function () {
+      it('should never run this test', function() {
         throw new Error('skipped suite test should not run');
       });
 
-      after(function () {
-        console.log('skipped after');
+      after(function() {
+        runOrder.push('skipped suite after should not run');
       });
     });
   });
 
-  it('should run this test', function () { });
-
-  after(function () {
-    console.log('outer after');
-  })
+  it('should run test-2', function() {
+    runOrder.push('should run test-2');
+  });
 
+  after(function() {
+    runOrder.push('outer after');
+    assert.deepStrictEqual(runOrder, [
+      'outer before', 
+      'should run test-1', 'should run test-2',
+      'inner before', 'inner after',
+      'outer after'
+    ]);
+    throw new Error('should throw this error');
+  });
 });

test/integration/fixtures/pending/skip-sync-before-inner.fixture.js

@@ -0,0 +1,38 @@
+'use strict';
+var assert = require('assert');
+
+describe('outer suite', function() {
+  var runOrder = [];
+  before(function() {
+    runOrder.push('outer before');
+    this.skip();
+  });
+
+  it('should never run this outer test', function() {
+    throw new Error('outer suite test should not run');
+  });
+
+  describe('inner suite', function() {
+    before(function() { runOrder.push('no inner before'); });
+    before(function(done) { runOrder.push('no inner before'); done(); });
+    before(async function() { runOrder.push('no inner before'); });
+    before(function() { return Promise.resolve(runOrder.push('no inner before')) });
+
+    after(function() { runOrder.push('no inner after'); });
+    after(function(done) { runOrder.push('no inner after'); done(); });
+    after(async function() { runOrder.push('no inner after'); });
+    after(function() { return Promise.resolve(runOrder.push('no inner after')) });
+
+    it('should never run this inner test', function() {
+      throw new Error('inner suite test should not run');
+    });
+  });
+
+  after(function() {
+    runOrder.push('outer after');
+    assert.deepStrictEqual(runOrder, [
+      'outer before', 'outer after'
+    ]);
+    throw new Error('should throw this error');
+  });
+});