uncaughtException: fix "--allow-uncaught" with "this.skip()" (#4030)

lib/runnable.js

@@ -135,7 +135,8 @@ Runnable.prototype.enableTimeouts = function(enabled) {
  * @public
  */
 Runnable.prototype.skip = function() {
-  throw new Pending('sync skip');
+  this.pending = true;
+  throw new Pending('sync skip; aborting execution');
 };
 
 /**
@@ -343,34 +344,27 @@ Runnable.prototype.run = function(fn) {
 
     // allows skip() to be used in an explicit async context
     this.skip = function asyncSkip() {
-      done(new Pending('async skip call'));
-      // halt execution.  the Runnable will be marked pending
-      // by the previous call, and the uncaught handler will ignore
-      // the failure.
+      this.pending = true;
+      done();
+      // halt execution, the uncaught handler will ignore the failure.
       throw new Pending('async skip; aborting execution');
     };
 
-    if (this.allowUncaught) {
-      return callFnAsync(this.fn);
-    }
     try {
       callFnAsync(this.fn);
     } catch (err) {
+      // handles async runnables which actually run synchronously
       emitted = true;
+      if (err instanceof Pending) {
+        return; // done() is already called in this.skip()
+      } else if (this.allowUncaught) {
+        throw err;
+      }
       done(Runnable.toValueOrError(err));
     }
     return;
   }
 
-  if (this.allowUncaught) {
-    if (this.isPending()) {
-      done();
-    } else {
-      callFn(this.fn);
-    }
-    return;
-  }
-
   // sync or promise-returning
   try {
     if (this.isPending()) {
@@ -380,6 +374,11 @@ Runnable.prototype.run = function(fn) {
     }
   } catch (err) {
     emitted = true;
+    if (err instanceof Pending) {
+      return done();
+    } else if (this.allowUncaught) {
+      throw err;
+    }
     done(Runnable.toValueOrError(err));
   }
 

lib/runner.js

@@ -386,34 +386,30 @@ Runner.prototype.hook = function(name, fn) {
       if (testError) {
         self.fail(self.test, testError);
       }
-      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;
-            }
-          } else {
-            suite.tests.forEach(function(test) {
-              test.pending = true;
-            });
-            suite.suites.forEach(function(suite) {
-              suite.pending = true;
-            });
-            // a pending hook won't be executed twice.
-            hook.pending = true;
+      // conditional this.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_BEFORE_EACH || name === HOOK_TYPE_AFTER_EACH) {
+          if (self.test) {
+            self.test.pending = true;
           }
         } else {
-          self.failHook(hook, err);
-
-          // stop executing hooks, notify callee of hook err
-          return fn(err);
+          suite.tests.forEach(function(test) {
+            test.pending = true;
+          });
+          suite.suites.forEach(function(suite) {
+            suite.pending = true;
+          });
         }
+      } else if (err) {
+        self.failHook(hook, err);
+        // stop executing hooks, notify callee of hook err
+        return fn(err);
       }
       self.emit(constants.EVENT_HOOK_END, hook);
       delete hook.ctx.currentTest;
@@ -525,6 +521,9 @@ 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) {
@@ -652,14 +651,20 @@ Runner.prototype.runTests = function(suite, fn) {
       self.currentRunnable = self.test;
       self.runTest(function(err) {
         test = self.test;
-        if (err) {
-          var retry = test.currentRetry();
-          if (err instanceof Pending && self.forbidPending) {
+        // conditional this.skip()
+        if (test.pending) {
+          if (self.forbidPending) {
+            test.isPending = alwaysFalse;
             self.fail(test, new Error('Pending test forbidden'));
-          } else if (err instanceof Pending) {
-            test.pending = true;
+            delete test.isPending;
+          } else {
             self.emit(constants.EVENT_TEST_PENDING, test);
-          } else if (retry < test.retries()) {
+          }
+          self.emit(constants.EVENT_TEST_END, test);
+          return self.hookUp(HOOK_TYPE_AFTER_EACH, next);
+        } else if (err) {
+          var retry = test.currentRetry();
+          if (retry < test.retries()) {
             var clonedTest = test.clone();
             clonedTest.currentRetry(retry + 1);
             tests.unshift(clonedTest);
@@ -910,6 +915,12 @@ Runner.prototype.run = function(fn) {
   this.on(constants.EVENT_RUN_END, function() {
     debug(constants.EVENT_RUN_END);
     process.removeListener('uncaughtException', uncaught);
+    process.on('uncaughtException', function(err) {
+      if (err instanceof Pending) {
+        return;
+      }
+      throw err;
+    });
     fn(self.failures);
   });
 

test/integration/fixtures/options/allow-uncaught/this-skip-it.fixture.js

@@ -0,0 +1,21 @@
+'use strict';
+
+describe('test suite', () => {
+    it('test1', function () { });
+    it('test2', function (done) {
+        var self = this;
+        setTimeout(function () {
+          self.skip();
+          throw new Error("should not throw");
+        }, 10);
+    });
+    it('test3', function () {
+        this.skip();
+        throw new Error("should not throw");
+    });
+    it('test4', function () { });
+    it('test5', function () {
+        this.skip();
+        throw new Error("should not throw");
+    });
+});

test/integration/options/allowUncaught.spec.js

@@ -0,0 +1,25 @@
+'use strict';
+
+var path = require('path').posix;
+var helpers = require('../helpers');
+var runMochaJSON = helpers.runMochaJSON;
+
+describe('--allow-uncaught', function() {
+  var args = ['--allow-uncaught'];
+
+  it('should run with conditional `this.skip()`', function(done) {
+    var fixture = path.join('options', 'allow-uncaught', 'this-skip-it');
+    runMochaJSON(fixture, args, function(err, res) {
+      if (err) {
+        return done(err);
+      }
+
+      expect(res, 'to have passed')
+        .and('to have passed test count', 2)
+        .and('to have pending test count', 3)
+        .and('to have passed test', 'test1', 'test4')
+        .and('to have pending test order', 'test2', 'test3', 'test5');
+      done();
+    });
+  });
+});

test/unit/runnable.spec.js

@@ -4,7 +4,6 @@ var Mocha = require('../../lib/mocha');
 var Runnable = Mocha.Runnable;
 var Suite = Mocha.Suite;
 var sinon = require('sinon');
-var Pending = require('../../lib/pending');
 var STATE_FAILED = Runnable.constants.STATE_FAILED;
 
 describe('Runnable(title, fn)', function() {
@@ -644,13 +643,14 @@ describe('Runnable(title, fn)', function() {
     });
 
     describe('if async', function() {
-      it('this.skip() should call callback with Pending', function(done) {
+      it('this.skip() should set runnable to pending', function(done) {
         var runnable = new Runnable('foo', function(done) {
           // normally "this" but it gets around having to muck with a context
           runnable.skip();
         });
         runnable.run(function(err) {
-          expect(err.constructor, 'to be', Pending);
+          expect(err, 'to be undefined');
+          expect(runnable.pending, 'to be true');
           done();
         });
       });
@@ -663,8 +663,10 @@ describe('Runnable(title, fn)', function() {
           aborted = false;
         });
         runnable.run(function() {
-          expect(aborted, 'to be true');
-          done();
+          process.nextTick(function() {
+            expect(aborted, 'to be true');
+            done();
+          });
         });
       });
     });