From 46cc48ccfa27ebfdc92fcdf865d81bf1af1f37aa Mon Sep 17 00:00:00 2001 From: Gregg Van Hove Date: Mon, 29 Jan 2018 16:46:30 -0800 Subject: [PATCH] Detect an Error passed to `done` and add an expectation failure - See #567 --- lib/jasmine-core/jasmine.js | 37 +++++++++++++----------- spec/core/QueueRunnerSpec.js | 41 ++++++++++++++++++++++++++ spec/core/integration/EnvSpec.js | 48 +++++++++++++++---------------- src/core/QueueRunner.js | 11 +++++-- src/core/base.js | 11 +++++++ src/core/matchers/toThrowError.js | 15 ++-------- 6 files changed, 107 insertions(+), 56 deletions(-) diff --git a/lib/jasmine-core/jasmine.js b/lib/jasmine-core/jasmine.js index f8a9f028..e90947ab 100644 --- a/lib/jasmine-core/jasmine.js +++ b/lib/jasmine-core/jasmine.js @@ -218,6 +218,17 @@ getJasmineRequireObj().base = function(j$, jasmineGlobal) { return j$.getType_(value) === '[object ' + typeName + ']'; }; + j$.isError_ = function(value) { + if (value instanceof Error) { + return true; + } + if (value && value.constructor && value.constructor.constructor && + (value instanceof (value.constructor.constructor('return this')()).Error)) { + return true; + } + return false; + }; + j$.getType_ = function(value) { return Object.prototype.toString.apply(value); }; @@ -3846,7 +3857,7 @@ getJasmineRequireObj().toThrowError = function(j$) { thrown = e; } - if (!isErrorObject(thrown)) { + if (!j$.isError_(thrown)) { return fail(function() { return 'Expected function to throw an Error, but it threw ' + j$.pp(thrown) + '.'; }); } @@ -3958,18 +3969,7 @@ getJasmineRequireObj().toThrowError = function(j$) { var Surrogate = function() {}; Surrogate.prototype = type.prototype; - return isErrorObject(new Surrogate()); - } - - function isErrorObject(thrown) { - if (thrown instanceof Error) { - return true; - } - if (thrown && thrown.constructor && thrown.constructor.constructor && - (thrown instanceof (thrown.constructor.constructor('return this')()).Error)) { - return true; - } - return false; + return j$.isError_(new Surrogate()); } } @@ -4453,7 +4453,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { return function() { if (!called) { called = true; - fn(); + fn.apply(null, arguments); } return null; }; @@ -4507,15 +4507,20 @@ getJasmineRequireObj().QueueRunner = function(j$) { var self = this, completedSynchronously = true, handleError = function handleError(error) { onException(error); - next(); + next(error); }, cleanup = once(function cleanup() { self.clearTimeout(timeoutId); self.globalErrors.popListener(handleError); }), - next = once(function next() { + next = once(function next(err) { cleanup(); + if (j$.isError_(err)) { + self.fail(err); + errored = true; + } + function runNext() { if (self.completeOnFirstError && errored) { self.skipToCleanup(iterativeIndex); diff --git a/spec/core/QueueRunnerSpec.js b/spec/core/QueueRunnerSpec.js index 3059b50b..15483cfe 100644 --- a/spec/core/QueueRunnerSpec.js +++ b/spec/core/QueueRunnerSpec.js @@ -135,6 +135,29 @@ describe("QueueRunner", function() { expect(queueableFn2.fn).toHaveBeenCalled(); }); + it("explicitly fails an async function when next is called with an Error and moves to the next function", function() { + var err = new Error('foo'), + queueableFn1 = { fn: function(done) { + setTimeout(function() { done(err); }, 100); + } }, + queueableFn2 = { fn: jasmine.createSpy('fn2') }, + failFn = jasmine.createSpy('fail'), + queueRunner = new jasmineUnderTest.QueueRunner({ + queueableFns: [queueableFn1, queueableFn2], + fail: failFn + }); + + queueRunner.execute(); + + expect(failFn).not.toHaveBeenCalled(); + expect(queueableFn2.fn).not.toHaveBeenCalled(); + + jasmine.clock().tick(100); + + expect(failFn).toHaveBeenCalledWith(err); + expect(queueableFn2.fn).toHaveBeenCalled(); + }); + it("sets a timeout if requested for asynchronous functions so they don't go on forever", function() { var timeout = 3, beforeFn = { fn: function(done) { }, type: 'before', timeout: function() { return timeout; } }, @@ -522,6 +545,24 @@ describe("QueueRunner", function() { expect(nextQueueableFn.fn).not.toHaveBeenCalled(); expect(cleanupFn.fn).toHaveBeenCalled(); }); + + it("skips to cleanup functions when next is called with an Error", function() { + var queueableFn = { fn: function(done) { + done(new Error('nope')); + } }, + nextQueueableFn = { fn: jasmine.createSpy('nextFunction') }, + cleanupFn = { fn: jasmine.createSpy('cleanup') }, + queueRunner = new jasmineUnderTest.QueueRunner({ + queueableFns: [queueableFn, nextQueueableFn], + cleanupFns: [cleanupFn], + completeOnFirstError: true, + }); + + queueRunner.execute(); + jasmine.clock().tick(); + expect(nextQueueableFn.fn).not.toHaveBeenCalled(); + expect(cleanupFn.fn).toHaveBeenCalled(); + }); }); }); diff --git a/spec/core/integration/EnvSpec.js b/spec/core/integration/EnvSpec.js index ac3852a7..4fae2454 100644 --- a/spec/core/integration/EnvSpec.js +++ b/spec/core/integration/EnvSpec.js @@ -1205,30 +1205,21 @@ describe("Env integration", function() { env.addReporter({ specDone: specDone, jasmineDone: function() { - expect(specDone).toHaveBeenCalledWith(jasmine.objectContaining({ - description: 'has a default message', - failedExpectations: [jasmine.objectContaining({ - message: 'Failed' - })] - })); - expect(specDone).toHaveBeenCalledWith(jasmine.objectContaining({ - description: 'specifies a message', - failedExpectations: [jasmine.objectContaining({ - message: 'Failed: messy message' - })] - })); - expect(specDone).toHaveBeenCalledWith(jasmine.objectContaining({ - description: 'fails via the done callback', - failedExpectations: [jasmine.objectContaining({ - message: 'Failed: done failed' - })] - })); - expect(specDone).toHaveBeenCalledWith(jasmine.objectContaining({ - description: 'has a message from an Error', - failedExpectations: [jasmine.objectContaining({ - message: 'Failed: error message' - })] - })); + expect(specDone).toHaveFailedExpectationsForRunnable('failing has a default message', + ['Failed'] + ); + expect(specDone).toHaveFailedExpectationsForRunnable('failing specifies a message', + ['Failed: messy message'] + ); + expect(specDone).toHaveFailedExpectationsForRunnable('failing fails via the done callback', + ['Failed: done failed'] + ); + expect(specDone).toHaveFailedExpectationsForRunnable('failing has a message from an Error', + ['Failed: error message'] + ); + expect(specDone).toHaveFailedExpectationsForRunnable('failing has a message from an Error to done', + ['Failed: done error'] + ); jasmine.clock().tick(1); realSetTimeout(done); @@ -1274,6 +1265,15 @@ describe("Env integration", function() { jasmine.clock().tick(1); jasmine.clock().tick(1); }); + + env.it('has a message from an Error to done', function(innerDone) { + setTimeout(function() { + innerDone(new Error('done error')); + }, 1); + jasmine.clock().tick(1); + jasmine.clock().tick(1); + jasmine.clock().tick(1); + }); }); env.execute(); diff --git a/src/core/QueueRunner.js b/src/core/QueueRunner.js index cc8dc1e0..10a88f10 100644 --- a/src/core/QueueRunner.js +++ b/src/core/QueueRunner.js @@ -5,7 +5,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { return function() { if (!called) { called = true; - fn(); + fn.apply(null, arguments); } return null; }; @@ -59,15 +59,20 @@ getJasmineRequireObj().QueueRunner = function(j$) { var self = this, completedSynchronously = true, handleError = function handleError(error) { onException(error); - next(); + next(error); }, cleanup = once(function cleanup() { self.clearTimeout(timeoutId); self.globalErrors.popListener(handleError); }), - next = once(function next() { + next = once(function next(err) { cleanup(); + if (j$.isError_(err)) { + self.fail(err); + errored = true; + } + function runNext() { if (self.completeOnFirstError && errored) { self.skipToCleanup(iterativeIndex); diff --git a/src/core/base.js b/src/core/base.js index 5f0d54aa..799208c6 100644 --- a/src/core/base.js +++ b/src/core/base.js @@ -85,6 +85,17 @@ getJasmineRequireObj().base = function(j$, jasmineGlobal) { return j$.getType_(value) === '[object ' + typeName + ']'; }; + j$.isError_ = function(value) { + if (value instanceof Error) { + return true; + } + if (value && value.constructor && value.constructor.constructor && + (value instanceof (value.constructor.constructor('return this')()).Error)) { + return true; + } + return false; + }; + j$.getType_ = function(value) { return Object.prototype.toString.apply(value); }; diff --git a/src/core/matchers/toThrowError.js b/src/core/matchers/toThrowError.js index 2024d2a3..ca3bfc5e 100644 --- a/src/core/matchers/toThrowError.js +++ b/src/core/matchers/toThrowError.js @@ -32,7 +32,7 @@ getJasmineRequireObj().toThrowError = function(j$) { thrown = e; } - if (!isErrorObject(thrown)) { + if (!j$.isError_(thrown)) { return fail(function() { return 'Expected function to throw an Error, but it threw ' + j$.pp(thrown) + '.'; }); } @@ -144,18 +144,7 @@ getJasmineRequireObj().toThrowError = function(j$) { var Surrogate = function() {}; Surrogate.prototype = type.prototype; - return isErrorObject(new Surrogate()); - } - - function isErrorObject(thrown) { - if (thrown instanceof Error) { - return true; - } - if (thrown && thrown.constructor && thrown.constructor.constructor && - (thrown instanceof (thrown.constructor.constructor('return this')()).Error)) { - return true; - } - return false; + return j$.isError_(new Surrogate()); } }