From 9dfeeada36f3067b69d77ed6fcdba598a78df49d Mon Sep 17 00:00:00 2001 From: juergba Date: Mon, 13 Jan 2020 13:53:41 +0100 Subject: [PATCH 1/4] remove recovery out of uncaught --- lib/runnable.js | 2 +- lib/runner.js | 54 ++++++++++--------------------------------------- 2 files changed, 12 insertions(+), 44 deletions(-) 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(); }; /** From 1e14663f2a7406ceac893f75886fb04c4c6f0f2a Mon Sep 17 00:00:00 2001 From: juergba Date: Mon, 13 Jan 2020 13:54:25 +0100 Subject: [PATCH 2/4] adapt existing tests --- test/integration/hook-err.spec.js | 5 +- test/integration/uncaught.spec.js | 2 +- test/unit/runner.spec.js | 111 ++++++------------------------ 3 files changed, 26 insertions(+), 92 deletions(-) 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..001c89429a 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(); diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index b3b3a903a5..a24aa4b517 100644 --- a/test/unit/runner.spec.js +++ b/test/unit/runner.spec.js @@ -12,6 +12,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; @@ -783,14 +784,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 +841,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(); - }); - - afterEach(function() { - delete runner.next; + runnable.callback = sandbox.fake(); }); - 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 +877,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 +917,7 @@ describe('Runner', function() { }, 'not to emit from', runner, - 'end' + EVENT_RUN_END ); }); }); From 4604d7d18319ec02d6ce4c1e8cef70ab78670726 Mon Sep 17 00:00:00 2001 From: juergba Date: Wed, 15 Jan 2020 10:00:10 +0100 Subject: [PATCH 3/4] additional test --- .../fixtures/uncaught/recover.fixture.js | 28 +++++++++++++++++++ test/integration/uncaught.spec.js | 27 ++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 test/integration/fixtures/uncaught/recover.fixture.js 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/uncaught.spec.js b/test/integration/uncaught.spec.js index 001c89429a..5b193280bc 100644 --- a/test/integration/uncaught.spec.js +++ b/test/integration/uncaught.spec.js @@ -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) { From 0250f677b86d758bc2d7436481a51bfd9d5a99e8 Mon Sep 17 00:00:00 2001 From: juergba Date: Thu, 16 Jan 2020 14:42:18 +0100 Subject: [PATCH 4/4] more tests --- test/unit/runner.spec.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/unit/runner.spec.js b/test/unit/runner.spec.js index a24aa4b517..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; @@ -445,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); @@ -691,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; @@ -714,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() {