From c0f1d1456dbc068f0552a5ceaed0d9b95e940ce1 Mon Sep 17 00:00:00 2001 From: Juerg B <44573692+juergba@users.noreply.github.com> Date: Thu, 23 Jan 2020 10:45:07 +0100 Subject: [PATCH] uncaughtException: fix recovery when current test is still running (#4150) --- lib/runnable.js | 2 +- lib/runner.js | 54 ++----- .../fixtures/uncaught/recover.fixture.js | 28 ++++ test/integration/hook-err.spec.js | 5 +- test/integration/uncaught.spec.js | 29 +++- test/unit/runner.spec.js | 140 +++++++----------- 6 files changed, 122 insertions(+), 136 deletions(-) create mode 100644 test/integration/fixtures/uncaught/recover.fixture.js diff --git a/lib/runnable.js b/lib/runnable.js index 0346b36346..7d3011dc86 100644 --- a/lib/runnable.js +++ b/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') { diff --git a/lib/runner.js b/lib/runner.js index 948a9b9021..bf1d14b183 100644 --- a/lib/runner.js +++ b/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(); }; /** diff --git a/test/integration/fixtures/uncaught/recover.fixture.js b/test/integration/fixtures/uncaught/recover.fixture.js new file mode 100644 index 0000000000..945decb045 --- /dev/null +++ b/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'); + }); +}); diff --git a/test/integration/hook-err.spec.js b/test/integration/hook-err.spec.js index 55604851ef..d5fe6e858d 100644 --- a/test/integration/hook-err.spec.js +++ b/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":' + ]); }); }); diff --git a/test/integration/uncaught.spec.js b/test/integration/uncaught.spec.js index 7d673b1eaa..5b193280bc 100644 --- a/test/integration/uncaught.spec.js +++ b/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) { diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index b3b3a903a5..b36cbb04bf 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -3,6 +3,7 @@ var path = require('path'); var sinon = require('sinon'); var Mocha = require('../../lib/mocha'); +var Pending = require('../../lib/pending'); var Suite = Mocha.Suite; var Runner = Mocha.Runner; var Test = Mocha.Test; @@ -12,6 +13,7 @@ var noop = Mocha.utils.noop; var EVENT_HOOK_BEGIN = Runner.constants.EVENT_HOOK_BEGIN; var EVENT_TEST_FAIL = Runner.constants.EVENT_TEST_FAIL; var EVENT_TEST_RETRY = Runner.constants.EVENT_TEST_RETRY; +var EVENT_TEST_END = Runner.constants.EVENT_TEST_END; var EVENT_RUN_END = Runner.constants.EVENT_RUN_END; var STATE_FAILED = Runnable.constants.STATE_FAILED; @@ -444,6 +446,13 @@ describe('Runner', function() { }); }); + describe('.runTest(fn)', function() { + it('should return when no tests to run', function() { + runner.test = undefined; + expect(runner.runTest(noop), 'to be undefined'); + }); + }); + describe('allowUncaught', function() { it('should allow unhandled errors to propagate through', function() { var newRunner = new Runner(suite); @@ -690,6 +699,20 @@ describe('Runner', function() { sandbox.stub(runner, 'fail'); }); + describe('when allow-uncaught is set to true', function() { + it('should propagate error and throw', function() { + var err = new Error('should rethrow err'); + runner.allowUncaught = true; + expect( + function() { + runner.uncaught(err); + }, + 'to throw', + 'should rethrow err' + ); + }); + }); + describe('when provided an object argument', function() { describe('when argument is not an Error', function() { var err; @@ -713,6 +736,13 @@ describe('Runner', function() { }); }); + describe('when argument is a Pending', function() { + it('should ignore argument and return', function() { + var err = new Pending(); + expect(runner.uncaught(err), 'to be undefined'); + }); + }); + describe('when argument is an Error', function() { var err; beforeEach(function() { @@ -783,14 +813,10 @@ describe('Runner', function() { runnable.parent = runner.suite; sandbox.stub(runnable, 'clearTimeout'); runner.currentRunnable = runnable; - runner.nextSuite = sandbox.spy(); - }); - - afterEach(function() { - delete runner.nextSuite; }); it('should clear any pending timeouts', function() { + runnable.callback = sandbox.fake(); runner.uncaught(err); expect(runnable.clearTimeout, 'was called times', 1); }); @@ -844,75 +870,35 @@ describe('Runner', function() { }); }); - describe('when the current Runnable is currently running', function() { + describe('when the current Runnable is still running', function() { describe('when the current Runnable is a Test', function() { beforeEach(function() { runnable = new Test('goomba', noop); runnable.parent = runner.suite; runner.currentRunnable = runnable; - sandbox.stub(runner, 'hookUp'); - runner.next = sandbox.spy(); + runnable.callback = sandbox.fake(); }); - afterEach(function() { - delete runner.next; - }); - - it('should fail with the current Runnable and the error', function() { + it('should run callback(err) to handle failing and hooks', function() { runner.uncaught(err); - expect(runner.fail, 'to have all calls satisfying', [ - expect.it('to be', runnable), + expect(runner.fail, 'was not called'); + expect(runnable.callback, 'to have all calls satisfying', [ err ]).and('was called once'); }); - it('should notify test has ended', function() { - expect( - function() { - runner.uncaught(err); - }, - 'to emit from', - runner, - 'test end', - runnable - ); - }); - - it('should not notify run has ended', function() { + it('should not notify test has ended', function() { expect( function() { runner.uncaught(err); }, 'not to emit from', runner, - 'end' + EVENT_TEST_END ); }); - it('should call any remaining "after each" hooks', function() { - runner.uncaught(err); - expect(runner.hookUp, 'to have all calls satisfying', [ - 'afterEach', - expect.it('to be', runner.next) - ]).and('was called once'); - }); - }); - - describe('when the current Runnable is a "before all" or "after all" hook', function() { - beforeEach(function() { - runnable = new Hook('', noop); - runnable.parent = runner.suite; - runner.currentRunnable = runnable; - }); - - it('should continue to the next suite', function() { - runner.uncaught(err); - expect(runner.nextSuite, 'to have all calls satisfying', [ - runner.suite - ]).and('was called once'); - }); - it('should not notify run has ended', function() { expect( function() { @@ -920,64 +906,38 @@ describe('Runner', function() { }, 'not to emit from', runner, - 'end' + EVENT_RUN_END ); }); }); - describe('when the current Runnable is a "before each" hook', function() { + describe('when the current Runnable is a Hook', function() { beforeEach(function() { - runnable = new Hook('before each', noop); + runnable = new Hook(); runnable.parent = runner.suite; runner.currentRunnable = runnable; - runner.hookErr = sandbox.spy(); + runnable.callback = sandbox.fake(); }); - afterEach(function() { - delete runner.hookErr; - }); - - it('should associate its failure with the current test', function() { + it('should run callback(err) to handle failing hook pattern', function() { runner.uncaught(err); - expect(runner.hookErr, 'to have all calls satisfying', [ - err, - runner.suite, - false + + expect(runner.fail, 'was not called'); + expect(runnable.callback, 'to have all calls satisfying', [ + err ]).and('was called once'); }); - it('should not notify run has ended', function() { + it('should not notify test has ended', function() { expect( function() { runner.uncaught(err); }, 'not to emit from', runner, - 'end' + EVENT_TEST_END ); }); - }); - - describe('when the current Runnable is an "after each" hook', function() { - beforeEach(function() { - runnable = new Hook('after each', noop); - runnable.parent = runner.suite; - runner.currentRunnable = runnable; - runner.hookErr = sandbox.spy(); - }); - - afterEach(function() { - delete runner.hookErr; - }); - - it('should associate its failure with the current test', function() { - runner.uncaught(err); - expect(runner.hookErr, 'to have all calls satisfying', [ - err, - runner.suite, - true - ]).and('was called once'); - }); it('should not notify run has ended', function() { expect( @@ -986,7 +946,7 @@ describe('Runner', function() { }, 'not to emit from', runner, - 'end' + EVENT_RUN_END ); }); });