uncaughtException: fix recovery when current test is still running (#…

…4150)

lib/runnable.js

@@ -335,7 +335,7 @@ Runnable.prototype.run = function(fn) {
     fn(err);
   }
 
-  // for .resetTimeout()
+  // for .resetTimeout() and Runner#uncaught()
   this.callback = done;
 
   if (this.fn && typeof this.fn.call !== 'function') {

lib/runner.js

@@ -724,7 +724,6 @@ Runner.prototype.runSuite = function(suite, fn) {
   var i = 0;
   var self = this;
   var total = this.grepTotal(suite);
-  var afterAllHookCalled = false;
 
   debug('run suite %s', suite.fullTitle());
 
@@ -772,21 +771,13 @@ Runner.prototype.runSuite = function(suite, fn) {
     self.suite = suite;
     self.nextSuite = next;
 
-    if (afterAllHookCalled) {
-      fn(errSuite);
-    } else {
-      // mark that the afterAll block has been called once
-      // and so can be skipped if there is an error in it.
-      afterAllHookCalled = true;
-
-      // remove reference to test
-      delete self.test;
+    // remove reference to test
+    delete self.test;
 
-      self.hook(HOOK_TYPE_AFTER_ALL, function() {
-        self.emit(constants.EVENT_SUITE_END, suite);
-        fn(errSuite);
-      });
-    }
+    self.hook(HOOK_TYPE_AFTER_ALL, function() {
+      self.emit(constants.EVENT_SUITE_END, suite);
+      fn(errSuite);
+    });
   }
 
   this.nextSuite = next;
@@ -861,36 +852,13 @@ Runner.prototype.uncaught = function(err) {
 
   // we cannot recover gracefully if a Runnable has already passed
   // then fails asynchronously
-  var alreadyPassed = runnable.isPassed();
-  // this will change the state to "failed" regardless of the current value
-  this.fail(runnable, err);
-  if (!alreadyPassed) {
-    // recover from test
-    if (runnable.type === constants.EVENT_TEST_BEGIN) {
-      this.emit(constants.EVENT_TEST_END, runnable);
-      this.hookUp(HOOK_TYPE_AFTER_EACH, this.next);
-      return;
-    }
+  if (runnable.isPassed()) {
+    this.fail(runnable, err);
+    this.abort();
+  } else {
     debug(runnable);
-
-    // recover from hooks
-    var errSuite = this.suite;
-
-    // XXX how about a less awful way to determine this?
-    // if hook failure is in afterEach block
-    if (runnable.fullTitle().indexOf('after each') > -1) {
-      return this.hookErr(err, errSuite, true);
-    }
-    // if hook failure is in beforeEach block
-    if (runnable.fullTitle().indexOf('before each') > -1) {
-      return this.hookErr(err, errSuite, false);
-    }
-    // if hook failure is in after or before blocks
-    return this.nextSuite(errSuite);
+    return runnable.callback(err);
   }
-
-  // bail
-  this.abort();
 };
 
 /**

test/integration/fixtures/uncaught/recover.fixture.js

@@ -0,0 +1,28 @@
+'use strict';
+const assert = require('assert');
+
+describe('uncaught', function() {
+  var hookOrder = [];
+  it('throw delayed error', (done) => {
+    setTimeout(() => {
+      throw new Error('Whoops!');
+    }, 10)
+    setTimeout(done, 10);
+  });
+  it('should wait 15ms', (done) => {      
+    setTimeout(done, 15);
+  });
+  it('test 3', () => { });
+
+  afterEach(function() {
+    hookOrder.push(this.currentTest.title);
+  });
+  after(function() {
+    hookOrder.push('after');
+    assert.deepEqual(
+      hookOrder,
+      ['throw delayed error', 'should wait 15ms', 'test 3', 'after']
+    );
+    throw new Error('should get upto here and throw');
+  });
+});

test/integration/hook-err.spec.js

@@ -194,7 +194,10 @@ describe('hook error handling', function() {
       run('hooks/before-hook-async-error-tip.fixture.js', onlyErrorTitle())
     );
     it('should verify results', function() {
-      expect(lines, 'to equal', ['1) spec 2', '"before all" hook:']);
+      expect(lines, 'to equal', [
+        '1) spec 2',
+        '"before all" hook for "skipped":'
+      ]);
     });
   });
 

test/integration/uncaught.spec.js

@@ -17,7 +17,7 @@ describe('uncaught exceptions', function() {
 
       assert.strictEqual(
         res.failures[0].fullTitle,
-        'uncaught "before each" hook'
+        'uncaught "before each" hook for "test"'
       );
       assert.strictEqual(res.code, 1);
       done();
@@ -87,6 +87,33 @@ describe('uncaught exceptions', function() {
     });
   });
 
+  it('handles uncaught exceptions within open tests', function(done) {
+    run('uncaught/recover.fixture.js', args, function(err, res) {
+      if (err) {
+        return done(err);
+      }
+
+      expect(
+        res,
+        'to have failed with error',
+        'Whoops!',
+        'Whoops!', // JSON reporter does not show the second error message
+        'should get upto here and throw'
+      )
+        .and('to have passed test count', 2)
+        .and('to have failed test count', 3)
+        .and('to have passed test', 'should wait 15ms', 'test 3')
+        .and(
+          'to have failed test',
+          'throw delayed error',
+          'throw delayed error',
+          '"after all" hook for "test 3"'
+        );
+
+      done();
+    });
+  });
+
   it('removes uncaught exceptions handlers correctly', function(done) {
     run('uncaught/listeners.fixture.js', args, function(err, res) {
       if (err) {