From be23836c9d3ee5aa0b57f2efc4243b7aaebcf4a3 Mon Sep 17 00:00:00 2001 From: Steve Gravrock Date: Wed, 8 Sep 2021 20:44:27 -0700 Subject: [PATCH] Deprecate multiple calls to done callbacks --- lib/jasmine-core/jasmine.js | 159 ++++++++++++++++++++----- spec/core/QueueRunnerSpec.js | 37 +++++- spec/core/SpecSpec.js | 27 +++++ spec/core/SuiteSpec.js | 40 +++++++ spec/core/TreeProcessorSpec.js | 10 +- spec/core/integration/EnvSpec.js | 195 ++++++++++++++++++++++++++++++- src/core/Env.js | 4 +- src/core/QueueRunner.js | 88 +++++++++----- src/core/ReportDispatcher.js | 12 +- src/core/Spec.js | 19 ++- src/core/Suite.js | 26 +++++ src/core/TreeProcessor.js | 10 +- 12 files changed, 553 insertions(+), 74 deletions(-) diff --git a/lib/jasmine-core/jasmine.js b/lib/jasmine-core/jasmine.js index 827b137f..02037164 100644 --- a/lib/jasmine-core/jasmine.js +++ b/lib/jasmine-core/jasmine.js @@ -772,6 +772,7 @@ getJasmineRequireObj().Spec = function(j$) { }; this.expectationResultFactory = attrs.expectationResultFactory || function() {}; + this.deprecated = attrs.deprecated || function() {}; this.queueRunnerFactory = attrs.queueRunnerFactory || function() {}; this.catchingExceptions = attrs.catchingExceptions || @@ -866,6 +867,21 @@ getJasmineRequireObj().Spec = function(j$) { onException: function() { self.onException.apply(self, arguments); }, + onMultipleDone: function() { + // Issue a deprecation. Include the context ourselves and pass + // ignoreRunnable: true, since getting here always means that we've already + // moved on and the current runnable isn't the one that caused the problem. + self.deprecated( + "An asynchronous function called its 'done' " + + 'callback more than once. This is a bug in the spec, beforeAll, ' + + 'beforeEach, afterAll, or afterEach function in question. This will ' + + 'be treated as an error in a future version.\n' + + '(in spec: ' + + self.getFullName() + + ')', + { ignoreRunnable: true } + ); + }, onComplete: function() { if (self.result.status === 'failed') { onComplete(new j$.StopExecutionError('spec failed')); @@ -873,7 +889,8 @@ getJasmineRequireObj().Spec = function(j$) { onComplete(); } }, - userContext: this.userContext() + userContext: this.userContext(), + runnableName: this.getFullName.bind(this) }; if (this.markedPending || excluded === true) { @@ -1929,7 +1946,8 @@ getJasmineRequireObj().Env = function(j$) { */ 'specDone' ], - queueRunnerFactory + queueRunnerFactory, + self.deprecated ); /** @@ -2344,6 +2362,7 @@ getJasmineRequireObj().Env = function(j$) { beforeAndAfterFns: beforeAndAfterFns(suite), expectationFactory: expectationFactory, asyncExpectationFactory: specAsyncExpectationFactory, + deprecated: self.deprecated, resultCallback: specResultCallback, getSpecName: function(spec) { return getSpecName(spec, suite); @@ -8201,10 +8220,14 @@ getJasmineRequireObj().QueueRunner = function(j$) { StopExecutionError.prototype = new Error(); j$.StopExecutionError = StopExecutionError; - function once(fn) { + function once(fn, onTwice) { var called = false; return function(arg) { - if (!called) { + if (called) { + if (onTwice) { + onTwice(); + } + } else { called = true; // Direct call using single parameter, because cleanup/next does not need more fn(arg); @@ -8213,6 +8236,16 @@ getJasmineRequireObj().QueueRunner = function(j$) { }; } + function fallbackOnMultipleDone() { + console.error( + new Error( + "An asynchronous function called its 'done' " + + 'callback more than once, in a QueueRunner without a onMultipleDone ' + + 'handler.' + ) + ); + } + function emptyFn() {} function QueueRunner(attrs) { @@ -8227,6 +8260,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { fn(); }; this.onException = attrs.onException || emptyFn; + this.onMultipleDone = attrs.onMultipleDone || fallbackOnMultipleDone; this.userContext = attrs.userContext || new j$.UserContext(); this.timeout = attrs.timeout || { setTimeout: setTimeout, @@ -8284,6 +8318,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { var self = this, completedSynchronously = true, handleError = function handleError(error) { + // TODO probably shouldn't next() right away here. + // That makes debugging async failures much more confusing. onException(error); next(error); }, @@ -8293,37 +8329,52 @@ getJasmineRequireObj().QueueRunner = function(j$) { } self.globalErrors.popListener(handleError); }), - next = once(function next(err) { - cleanup(); + next = once( + function next(err) { + cleanup(); - if (j$.isError_(err)) { - if (!(err instanceof StopExecutionError) && !err.jasmineMessage) { - self.fail(err); + if (j$.isError_(err)) { + if (!(err instanceof StopExecutionError) && !err.jasmineMessage) { + self.fail(err); + } + self.errored = errored = true; + } else if (typeof err !== 'undefined' && !self.errored) { + self.deprecated( + 'Any argument passed to a done callback will be treated as an ' + + 'error in a future release. Call the done callback without ' + + "arguments if you don't want to trigger a spec failure." + ); } - self.errored = errored = true; - } else if (typeof err !== 'undefined' && !self.errored) { - self.deprecated( - 'Any argument passed to a done callback will be treated as an ' + - 'error in a future release. Call the done callback without ' + - "arguments if you don't want to trigger a spec failure." - ); - } - function runNext() { - if (self.completeOnFirstError && errored) { - self.skipToCleanup(iterativeIndex); + function runNext() { + if (self.completeOnFirstError && errored) { + self.skipToCleanup(iterativeIndex); + } else { + self.run(iterativeIndex + 1); + } + } + + if (completedSynchronously) { + self.setTimeout(runNext); } else { - self.run(iterativeIndex + 1); + runNext(); + } + }, + function() { + try { + if (!timedOut) { + self.onMultipleDone(); + } + } catch (error) { + // Any error we catch here is probably due to a bug in Jasmine, + // and it's not likely to end up anywhere useful if we let it + // propagate. Log it so it can at least show up when debugging. + console.error(error); } } - - if (completedSynchronously) { - self.setTimeout(runNext); - } else { - runNext(); - } - }), + ), errored = false, + timedOut = false, queueableFn = self.queueableFns[iterativeIndex], timeoutId, maybeThenable; @@ -8339,6 +8390,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { if (queueableFn.timeout !== undefined) { var timeoutInterval = queueableFn.timeout || j$.DEFAULT_TIMEOUT_INTERVAL; timeoutId = self.setTimeout(function() { + timedOut = true; var error = new Error( 'Timeout - Async function did not complete within ' + timeoutInterval + @@ -8347,6 +8399,9 @@ getJasmineRequireObj().QueueRunner = function(j$) { ? '(custom timeout)' : '(set by jasmine.DEFAULT_TIMEOUT_INTERVAL)') ); + // TODO Need to decide what to do about a successful completion after a + // timeout. That should probably not be a deprecation, and maybe not + // an error in 4.0. (But a diagnostic of some sort might be helpful.) onException(error); next(); }, timeoutInterval); @@ -8452,7 +8507,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { }; getJasmineRequireObj().ReportDispatcher = function(j$) { - function ReportDispatcher(methods, queueRunnerFactory) { + function ReportDispatcher(methods, queueRunnerFactory, deprecated) { var dispatchedMethods = methods || []; for (var i = 0; i < dispatchedMethods.length; i++) { @@ -8496,7 +8551,15 @@ getJasmineRequireObj().ReportDispatcher = function(j$) { queueRunnerFactory({ queueableFns: fns, onComplete: onComplete, - isReporter: true + isReporter: true, + onMultipleDone: function() { + deprecated( + "An asynchronous reporter callback called its 'done' callback " + + 'more than once. This is a bug in the reporter callback in ' + + 'question. This will be treated as an error in a future version.', + { ignoreRunnable: true } + ); + } }); } @@ -10042,6 +10105,32 @@ getJasmineRequireObj().Suite = function(j$) { this.result.failedExpectations.push(failedExpectation); }; + Suite.prototype.onMultipleDone = function() { + var msg; + + // Issue a deprecation. Include the context ourselves and pass + // ignoreRunnable: true, since getting here always means that we've already + // moved on and the current runnable isn't the one that caused the problem. + if (this.parentSuite) { + msg = + "An asynchronous function called its 'done' callback more than " + + 'once. This is a bug in the spec, beforeAll, beforeEach, afterAll, ' + + 'or afterEach function in question. This will be treated as an error ' + + 'in a future version.\n' + + '(in suite: ' + + this.getFullName() + + ')'; + } else { + msg = + 'A top-level beforeAll or afterAll function called its ' + + "'done' callback more than once. This is a bug in the beforeAll " + + 'or afterAll function in question. This will be treated as an ' + + 'error in a future version.'; + } + + this.env.deprecated(msg, { ignoreRunnable: true }); + }; + Suite.prototype.addExpectationResult = function() { if (isFailure(arguments)) { var data = arguments[1]; @@ -10144,7 +10233,10 @@ getJasmineRequireObj().TreeProcessor = function() { onException: function() { tree.onException.apply(tree, arguments); }, - onComplete: done + onComplete: done, + onMultipleDone: tree.onMultipleDone + ? tree.onMultipleDone.bind(tree) + : null }); }; @@ -10316,7 +10408,10 @@ getJasmineRequireObj().TreeProcessor = function() { userContext: node.sharedUserContext(), onException: function() { node.onException.apply(node, arguments); - } + }, + onMultipleDone: node.onMultipleDone + ? node.onMultipleDone.bind(node) + : null }); } }; diff --git a/spec/core/QueueRunnerSpec.js b/spec/core/QueueRunnerSpec.js index d7704569..d0574296 100644 --- a/spec/core/QueueRunnerSpec.js +++ b/spec/core/QueueRunnerSpec.js @@ -305,6 +305,32 @@ describe('QueueRunner', function() { expect(onComplete).toHaveBeenCalled(); }); + it('does not call onMultipleDone if an asynchrnous function completes after timing out', function() { + var timeout = 3, + queueableFn = { + fn: function(done) { + queueableFnDone = done; + }, + type: 'queueable', + timeout: timeout + }, + onComplete = jasmine.createSpy('onComplete'), + onMultipleDone = jasmine.createSpy('onMultipleDone'), + queueRunner = new jasmineUnderTest.QueueRunner({ + queueableFns: [queueableFn], + onComplete: onComplete, + onMultipleDone: onMultipleDone + }), + queueableFnDone; + + queueRunner.execute(); + jasmine.clock().tick(timeout); + queueableFnDone(); + + expect(onComplete).toHaveBeenCalled(); + expect(onMultipleDone).not.toHaveBeenCalled(); + }); + it('by default does not set a timeout for asynchronous functions', function() { var beforeFn = { fn: function(done) {} }, queueableFn = { fn: jasmine.createSpy('fn') }, @@ -380,13 +406,16 @@ describe('QueueRunner', function() { } }, nextQueueableFn = { fn: jasmine.createSpy('nextFn') }, + onMultipleDone = jasmine.createSpy('onMultipleDone'), queueRunner = new jasmineUnderTest.QueueRunner({ - queueableFns: [queueableFn, nextQueueableFn] + queueableFns: [queueableFn, nextQueueableFn], + onMultipleDone: onMultipleDone }); queueRunner.execute(); jasmine.clock().tick(1); expect(nextQueueableFn.fn.calls.count()).toEqual(1); + expect(onMultipleDone).toHaveBeenCalled(); }); it('does not move to the next spec if done is called after an exception has ended the spec', function() { @@ -397,13 +426,17 @@ describe('QueueRunner', function() { } }, nextQueueableFn = { fn: jasmine.createSpy('nextFn') }, + deprecated = jasmine.createSpy('deprecated'), queueRunner = new jasmineUnderTest.QueueRunner({ + deprecated: deprecated, queueableFns: [queueableFn, nextQueueableFn] }); - queueRunner.execute(); jasmine.clock().tick(1); expect(nextQueueableFn.fn.calls.count()).toEqual(1); + // Don't issue a deprecation. The error already tells the user that + // something went wrong. + expect(deprecated).not.toHaveBeenCalled(); }); it('should return a null when you call done', function() { diff --git a/spec/core/SpecSpec.js b/spec/core/SpecSpec.js index fa530049..33cf4fd8 100644 --- a/spec/core/SpecSpec.js +++ b/spec/core/SpecSpec.js @@ -516,4 +516,31 @@ describe('Spec', function() { args.cleanupFns[0].fn(); expect(resultCallback.calls.first().args[0].failedExpectations).toEqual([]); }); + + it('passes an onMultipleDone that logs a deprecation', function() { + var queueRunnerFactory = jasmine.createSpy('queueRunnerFactory'), + deprecated = jasmine.createSpy('depredated'), + spec = new jasmineUnderTest.Spec({ + deprecated: deprecated, + queueableFn: { fn: function() {} }, + queueRunnerFactory: queueRunnerFactory, + getSpecName: function() { + return 'a spec'; + } + }); + + spec.execute(); + + expect(queueRunnerFactory).toHaveBeenCalled(); + queueRunnerFactory.calls.argsFor(0)[0].onMultipleDone(); + + expect(deprecated).toHaveBeenCalledWith( + "An asynchronous function called its 'done' " + + 'callback more than once. This is a bug in the spec, beforeAll, ' + + 'beforeEach, afterAll, or afterEach function in question. This will ' + + 'be treated as an error in a future version.\n' + + '(in spec: a spec)', + { ignoreRunnable: true } + ); + }); }); diff --git a/spec/core/SuiteSpec.js b/spec/core/SuiteSpec.js index 2a1cd757..60b96285 100644 --- a/spec/core/SuiteSpec.js +++ b/spec/core/SuiteSpec.js @@ -142,4 +142,44 @@ describe('Suite', function() { ); }); }); + + describe('#onMultipleDone', function() { + it('logs a special deprecation when it is the top suite', function() { + var env = jasmine.createSpyObj('env', ['deprecated']); + var suite = new jasmineUnderTest.Suite({ env: env, parentSuite: null }); + + suite.onMultipleDone(); + + expect(env.deprecated).toHaveBeenCalledWith( + 'A top-level beforeAll or afterAll function called its ' + + "'done' callback more than once. This is a bug in the beforeAll " + + 'or afterAll function in question. This will be treated as an ' + + 'error in a future version.', + { ignoreRunnable: true } + ); + }); + + it('logs a deprecation including the suite name when it is a normal suite', function() { + var env = jasmine.createSpyObj('env', ['deprecated']); + var suite = new jasmineUnderTest.Suite({ + env: env, + description: 'the suite', + parentSuite: { + description: 'the parent suite', + parentSuite: {} + } + }); + + suite.onMultipleDone(); + + expect(env.deprecated).toHaveBeenCalledWith( + "An asynchronous function called its 'done' callback more than " + + 'once. This is a bug in the spec, beforeAll, beforeEach, afterAll, ' + + 'or afterEach function in question. This will be treated as an error ' + + 'in a future version.\n' + + '(in suite: the parent suite the suite)', + { ignoreRunnable: true } + ); + }); + }); }); diff --git a/spec/core/TreeProcessorSpec.js b/spec/core/TreeProcessorSpec.js index 5c31f100..b494d1ac 100644 --- a/spec/core/TreeProcessorSpec.js +++ b/spec/core/TreeProcessorSpec.js @@ -291,7 +291,8 @@ describe('TreeProcessor', function() { onComplete: treeComplete, onException: jasmine.any(Function), userContext: { root: 'context' }, - queueableFns: [{ fn: jasmine.any(Function) }] + queueableFns: [{ fn: jasmine.any(Function) }], + onMultipleDone: null }); queueRunner.calls.mostRecent().args[0].queueableFns[0].fn('foo'); @@ -321,16 +322,19 @@ describe('TreeProcessor', function() { onComplete: treeComplete, onException: jasmine.any(Function), userContext: { root: 'context' }, - queueableFns: [{ fn: jasmine.any(Function) }] + queueableFns: [{ fn: jasmine.any(Function) }], + onMultipleDone: null }); queueRunner.calls.mostRecent().args[0].queueableFns[0].fn(nodeDone); expect(queueRunner).toHaveBeenCalledWith({ onComplete: jasmine.any(Function), + onMultipleDone: null, queueableFns: [{ fn: jasmine.any(Function) }], userContext: { node: 'context' }, - onException: jasmine.any(Function) + onException: jasmine.any(Function), + onMultipleDone: null }); queueRunner.calls.mostRecent().args[0].queueableFns[0].fn('foo'); diff --git a/spec/core/integration/EnvSpec.js b/spec/core/integration/EnvSpec.js index a4aaa390..484e9e8d 100644 --- a/spec/core/integration/EnvSpec.js +++ b/spec/core/integration/EnvSpec.js @@ -513,6 +513,191 @@ describe('Env integration', function() { env.execute(null, assertions); }); + it('deprecates multiple calls to done in the top suite', function(done) { + var reporter = jasmine.createSpyObj('fakeReporter', ['jasmineDone']); + var message = + 'A top-level beforeAll or afterAll function called its ' + + "'done' callback more than once. This is a bug in the beforeAll " + + 'or afterAll function in question. This will be treated as an ' + + 'error in a future version.'; + + spyOn(console, 'error'); + env.addReporter(reporter); + env.configure({ verboseDeprecations: true }); + env.beforeAll(function(innerDone) { + innerDone(); + innerDone(); + }); + env.it('a spec, so the beforeAll runs', function() {}); + env.afterAll(function(innerDone) { + innerDone(); + innerDone(); + }); + + env.execute(null, function() { + var warnings; + expect(reporter.jasmineDone).toHaveBeenCalled(); + warnings = reporter.jasmineDone.calls.argsFor(0)[0].deprecationWarnings; + expect(warnings.length).toEqual(2); + expect(warnings[0]) + .withContext('top beforeAll') + .toEqual(jasmine.objectContaining({ message: message })); + expect(warnings[1]) + .withContext('top afterAll') + .toEqual(jasmine.objectContaining({ message: message })); + done(); + }); + }); + + it('deprecates multiple calls to done in a non-top suite', function(done) { + var reporter = jasmine.createSpyObj('fakeReporter', ['jasmineDone']); + var message = + "An asynchronous function called its 'done' " + + 'callback more than once. This is a bug in the spec, beforeAll, ' + + 'beforeEach, afterAll, or afterEach function in question. This will ' + + 'be treated as an error in a future version.'; + + spyOn(console, 'error'); + env.addReporter(reporter); + env.configure({ verboseDeprecations: true }); + env.describe('a suite', function() { + env.beforeAll(function(innerDone) { + innerDone(); + innerDone(); + }); + env.it('a spec, so that before/afters run', function() {}); + env.afterAll(function(innerDone) { + innerDone(); + innerDone(); + }); + }); + + env.execute(null, function() { + var warnings; + expect(reporter.jasmineDone).toHaveBeenCalled(); + warnings = reporter.jasmineDone.calls.argsFor(0)[0].deprecationWarnings; + expect(warnings.length).toEqual(2); + expect(warnings[0]) + .withContext('suite beforeAll') + .toEqual( + jasmine.objectContaining({ + message: message + '\n(in suite: a suite)' + }) + ); + expect(warnings[1]) + .withContext('suite afterAll') + .toEqual( + jasmine.objectContaining({ + message: message + '\n(in suite: a suite)' + }) + ); + done(); + }); + }); + + it('deprecates multiple calls to done in a spec', function(done) { + var reporter = jasmine.createSpyObj('fakeReporter', ['jasmineDone']); + var message = + "An asynchronous function called its 'done' " + + 'callback more than once. This is a bug in the spec, beforeAll, ' + + 'beforeEach, afterAll, or afterEach function in question. This will ' + + 'be treated as an error in a future version.\n' + + '(in spec: a suite a spec)'; + + spyOn(console, 'error'); + env.addReporter(reporter); + env.configure({ verboseDeprecations: true }); + env.describe('a suite', function() { + env.beforeEach(function(innerDone) { + innerDone(); + innerDone(); + }); + env.it('a spec', function(innerDone) { + innerDone(); + innerDone(); + }); + env.afterEach(function(innerDone) { + innerDone(); + innerDone(); + }); + }); + + env.execute(null, function() { + var warnings; + expect(reporter.jasmineDone).toHaveBeenCalled(); + warnings = reporter.jasmineDone.calls.argsFor(0)[0].deprecationWarnings; + expect(warnings.length).toEqual(3); + expect(warnings[0]) + .withContext('warning caused by beforeEach') + .toEqual(jasmine.objectContaining({ message: message })); + expect(warnings[1]) + .withContext('warning caused by it') + .toEqual(jasmine.objectContaining({ message: message })); + expect(warnings[2]) + .withContext('warning caused by afterEach') + .toEqual(jasmine.objectContaining({ message: message })); + done(); + }); + }); + + it('deprecates multiple calls to done in reporters', function(done) { + var message = + "An asynchronous reporter callback called its 'done' callback more " + + 'than once. This is a bug in the reporter callback in question. This ' + + 'will be treated as an error in a future version.\nNote: This message ' + + 'will be shown only once. Set config.verboseDeprecations to true to ' + + 'see every occurrence.'; + var reporter = jasmine.createSpyObj('fakeReport', ['jasmineDone']); + reporter.specDone = function(result, done) { + done(); + done(); + }; + env.addReporter(reporter); + + env.it('a spec', function() {}); + + spyOn(console, 'error'); + env.execute(null, function() { + expect(reporter.jasmineDone).toHaveBeenCalled(); + warnings = reporter.jasmineDone.calls.argsFor(0)[0].deprecationWarnings; + expect(warnings.length).toEqual(1); + expect(warnings[0]).toEqual( + jasmine.objectContaining({ message: message }) + ); + done(); + }); + }); + + it('does not deprecate a call to done that comes after a timeout', function(done) { + var reporter = jasmine.createSpyObj('fakeReporter', ['jasmineDone']), + firstSpecDone; + + reporter.specDone = function(result, reporterDone) { + setTimeout(function() { + firstSpecDone(); + reporterDone(); + }); + }; + env.addReporter(reporter); + + env.it( + 'a spec', + function(innerDone) { + firstSpecDone = innerDone; + }, + 1 + ); + + env.execute(null, function() { + expect(reporter.jasmineDone).toHaveBeenCalledWith( + jasmine.objectContaining({ + deprecationWarnings: [] + }) + ); + done(); + }); + }); + describe('suiteDone reporting', function() { it('reports when an afterAll fails an expectation', function(done) { var reporter = jasmine.createSpyObj('fakeReport', ['suiteDone']); @@ -1197,10 +1382,6 @@ describe('Env integration', function() { }); it('should wait a custom interval before reporting async functions that fail to complete', function(done) { - if (jasmine.getEnv().skipBrowserFlake) { - jasmine.getEnv().skipBrowserFlake(); - } - createMockedEnv(); var reporter = jasmine.createSpyObj('fakeReport', [ 'jasmineDone', @@ -2519,7 +2700,7 @@ describe('Env integration', function() { return setTimeout(fn, delay); }, clearTimeout: function(fn, delay) { - clearTimeout(fn, delay); + return clearTimeout(fn, delay); } }; spyOn(jasmineUnderTest, 'getGlobal').and.returnValue(global); @@ -2774,6 +2955,10 @@ describe('Env integration', function() { }); it('provides custom equality testers to async matchers', function(done) { + if (jasmine.getEnv().skipBrowserFlake) { + jasmine.getEnv().skipBrowserFlake(); + } + jasmine.getEnv().requirePromises(); var specDone = jasmine.createSpy('specDone'); diff --git a/src/core/Env.js b/src/core/Env.js index 5f34ac2a..6a28a34e 100644 --- a/src/core/Env.js +++ b/src/core/Env.js @@ -891,7 +891,8 @@ getJasmineRequireObj().Env = function(j$) { */ 'specDone' ], - queueRunnerFactory + queueRunnerFactory, + self.deprecated ); /** @@ -1306,6 +1307,7 @@ getJasmineRequireObj().Env = function(j$) { beforeAndAfterFns: beforeAndAfterFns(suite), expectationFactory: expectationFactory, asyncExpectationFactory: specAsyncExpectationFactory, + deprecated: self.deprecated, resultCallback: specResultCallback, getSpecName: function(spec) { return getSpecName(spec, suite); diff --git a/src/core/QueueRunner.js b/src/core/QueueRunner.js index b978b364..29b22660 100644 --- a/src/core/QueueRunner.js +++ b/src/core/QueueRunner.js @@ -5,10 +5,14 @@ getJasmineRequireObj().QueueRunner = function(j$) { StopExecutionError.prototype = new Error(); j$.StopExecutionError = StopExecutionError; - function once(fn) { + function once(fn, onTwice) { var called = false; return function(arg) { - if (!called) { + if (called) { + if (onTwice) { + onTwice(); + } + } else { called = true; // Direct call using single parameter, because cleanup/next does not need more fn(arg); @@ -17,6 +21,16 @@ getJasmineRequireObj().QueueRunner = function(j$) { }; } + function fallbackOnMultipleDone() { + console.error( + new Error( + "An asynchronous function called its 'done' " + + 'callback more than once, in a QueueRunner without a onMultipleDone ' + + 'handler.' + ) + ); + } + function emptyFn() {} function QueueRunner(attrs) { @@ -31,6 +45,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { fn(); }; this.onException = attrs.onException || emptyFn; + this.onMultipleDone = attrs.onMultipleDone || fallbackOnMultipleDone; this.userContext = attrs.userContext || new j$.UserContext(); this.timeout = attrs.timeout || { setTimeout: setTimeout, @@ -88,6 +103,8 @@ getJasmineRequireObj().QueueRunner = function(j$) { var self = this, completedSynchronously = true, handleError = function handleError(error) { + // TODO probably shouldn't next() right away here. + // That makes debugging async failures much more confusing. onException(error); next(error); }, @@ -97,37 +114,52 @@ getJasmineRequireObj().QueueRunner = function(j$) { } self.globalErrors.popListener(handleError); }), - next = once(function next(err) { - cleanup(); + next = once( + function next(err) { + cleanup(); - if (j$.isError_(err)) { - if (!(err instanceof StopExecutionError) && !err.jasmineMessage) { - self.fail(err); + if (j$.isError_(err)) { + if (!(err instanceof StopExecutionError) && !err.jasmineMessage) { + self.fail(err); + } + self.errored = errored = true; + } else if (typeof err !== 'undefined' && !self.errored) { + self.deprecated( + 'Any argument passed to a done callback will be treated as an ' + + 'error in a future release. Call the done callback without ' + + "arguments if you don't want to trigger a spec failure." + ); } - self.errored = errored = true; - } else if (typeof err !== 'undefined' && !self.errored) { - self.deprecated( - 'Any argument passed to a done callback will be treated as an ' + - 'error in a future release. Call the done callback without ' + - "arguments if you don't want to trigger a spec failure." - ); - } - function runNext() { - if (self.completeOnFirstError && errored) { - self.skipToCleanup(iterativeIndex); + function runNext() { + if (self.completeOnFirstError && errored) { + self.skipToCleanup(iterativeIndex); + } else { + self.run(iterativeIndex + 1); + } + } + + if (completedSynchronously) { + self.setTimeout(runNext); } else { - self.run(iterativeIndex + 1); + runNext(); + } + }, + function() { + try { + if (!timedOut) { + self.onMultipleDone(); + } + } catch (error) { + // Any error we catch here is probably due to a bug in Jasmine, + // and it's not likely to end up anywhere useful if we let it + // propagate. Log it so it can at least show up when debugging. + console.error(error); } } - - if (completedSynchronously) { - self.setTimeout(runNext); - } else { - runNext(); - } - }), + ), errored = false, + timedOut = false, queueableFn = self.queueableFns[iterativeIndex], timeoutId, maybeThenable; @@ -143,6 +175,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { if (queueableFn.timeout !== undefined) { var timeoutInterval = queueableFn.timeout || j$.DEFAULT_TIMEOUT_INTERVAL; timeoutId = self.setTimeout(function() { + timedOut = true; var error = new Error( 'Timeout - Async function did not complete within ' + timeoutInterval + @@ -151,6 +184,9 @@ getJasmineRequireObj().QueueRunner = function(j$) { ? '(custom timeout)' : '(set by jasmine.DEFAULT_TIMEOUT_INTERVAL)') ); + // TODO Need to decide what to do about a successful completion after a + // timeout. That should probably not be a deprecation, and maybe not + // an error in 4.0. (But a diagnostic of some sort might be helpful.) onException(error); next(); }, timeoutInterval); diff --git a/src/core/ReportDispatcher.js b/src/core/ReportDispatcher.js index d254fb59..6c3759ba 100644 --- a/src/core/ReportDispatcher.js +++ b/src/core/ReportDispatcher.js @@ -1,5 +1,5 @@ getJasmineRequireObj().ReportDispatcher = function(j$) { - function ReportDispatcher(methods, queueRunnerFactory) { + function ReportDispatcher(methods, queueRunnerFactory, deprecated) { var dispatchedMethods = methods || []; for (var i = 0; i < dispatchedMethods.length; i++) { @@ -43,7 +43,15 @@ getJasmineRequireObj().ReportDispatcher = function(j$) { queueRunnerFactory({ queueableFns: fns, onComplete: onComplete, - isReporter: true + isReporter: true, + onMultipleDone: function() { + deprecated( + "An asynchronous reporter callback called its 'done' callback " + + 'more than once. This is a bug in the reporter callback in ' + + 'question. This will be treated as an error in a future version.', + { ignoreRunnable: true } + ); + } }); } diff --git a/src/core/Spec.js b/src/core/Spec.js index 5be0040d..60946906 100644 --- a/src/core/Spec.js +++ b/src/core/Spec.js @@ -40,6 +40,7 @@ getJasmineRequireObj().Spec = function(j$) { }; this.expectationResultFactory = attrs.expectationResultFactory || function() {}; + this.deprecated = attrs.deprecated || function() {}; this.queueRunnerFactory = attrs.queueRunnerFactory || function() {}; this.catchingExceptions = attrs.catchingExceptions || @@ -134,6 +135,21 @@ getJasmineRequireObj().Spec = function(j$) { onException: function() { self.onException.apply(self, arguments); }, + onMultipleDone: function() { + // Issue a deprecation. Include the context ourselves and pass + // ignoreRunnable: true, since getting here always means that we've already + // moved on and the current runnable isn't the one that caused the problem. + self.deprecated( + "An asynchronous function called its 'done' " + + 'callback more than once. This is a bug in the spec, beforeAll, ' + + 'beforeEach, afterAll, or afterEach function in question. This will ' + + 'be treated as an error in a future version.\n' + + '(in spec: ' + + self.getFullName() + + ')', + { ignoreRunnable: true } + ); + }, onComplete: function() { if (self.result.status === 'failed') { onComplete(new j$.StopExecutionError('spec failed')); @@ -141,7 +157,8 @@ getJasmineRequireObj().Spec = function(j$) { onComplete(); } }, - userContext: this.userContext() + userContext: this.userContext(), + runnableName: this.getFullName.bind(this) }; if (this.markedPending || excluded === true) { diff --git a/src/core/Suite.js b/src/core/Suite.js index c3e475bd..ce873ad3 100644 --- a/src/core/Suite.js +++ b/src/core/Suite.js @@ -201,6 +201,32 @@ getJasmineRequireObj().Suite = function(j$) { this.result.failedExpectations.push(failedExpectation); }; + Suite.prototype.onMultipleDone = function() { + var msg; + + // Issue a deprecation. Include the context ourselves and pass + // ignoreRunnable: true, since getting here always means that we've already + // moved on and the current runnable isn't the one that caused the problem. + if (this.parentSuite) { + msg = + "An asynchronous function called its 'done' callback more than " + + 'once. This is a bug in the spec, beforeAll, beforeEach, afterAll, ' + + 'or afterEach function in question. This will be treated as an error ' + + 'in a future version.\n' + + '(in suite: ' + + this.getFullName() + + ')'; + } else { + msg = + 'A top-level beforeAll or afterAll function called its ' + + "'done' callback more than once. This is a bug in the beforeAll " + + 'or afterAll function in question. This will be treated as an ' + + 'error in a future version.'; + } + + this.env.deprecated(msg, { ignoreRunnable: true }); + }; + Suite.prototype.addExpectationResult = function() { if (isFailure(arguments)) { var data = arguments[1]; diff --git a/src/core/TreeProcessor.js b/src/core/TreeProcessor.js index 14798d77..f93fc044 100644 --- a/src/core/TreeProcessor.js +++ b/src/core/TreeProcessor.js @@ -44,7 +44,10 @@ getJasmineRequireObj().TreeProcessor = function() { onException: function() { tree.onException.apply(tree, arguments); }, - onComplete: done + onComplete: done, + onMultipleDone: tree.onMultipleDone + ? tree.onMultipleDone.bind(tree) + : null }); }; @@ -216,7 +219,10 @@ getJasmineRequireObj().TreeProcessor = function() { userContext: node.sharedUserContext(), onException: function() { node.onException.apply(node, arguments); - } + }, + onMultipleDone: node.onMultipleDone + ? node.onMultipleDone.bind(node) + : null }); } };