From 5eaeeb0b6c8c357667388df89816dd967057d6ca Mon Sep 17 00:00:00 2001 From: Steve Gravrock Date: Wed, 29 Sep 2021 12:14:07 -0700 Subject: [PATCH] Moved fn skipping policy out of QueueRunner This should allow us to more easily support complex skipping strategies like skipping nested cleanup fns when the corresponding befores were skipped, or skipping specs and suites when a beforeAll fails. * #1533 --- lib/jasmine-core/jasmine.js | 85 +++++++++++++------ .../CompleteOnFirstErrorSkipPolicySpec.js | 45 ++++++++++ spec/core/QueueRunnerSpec.js | 72 ++++++++++++++-- src/core/CompleteOnFirstErrorSkipPolicy.js | 19 +++++ src/core/Env.js | 16 ++-- src/core/NeverSkipPolicy.js | 9 ++ src/core/QueueRunner.js | 35 ++++---- src/core/requireCore.js | 4 + 8 files changed, 228 insertions(+), 57 deletions(-) create mode 100644 spec/core/CompleteOnFirstErrorSkipPolicySpec.js create mode 100644 src/core/CompleteOnFirstErrorSkipPolicy.js create mode 100644 src/core/NeverSkipPolicy.js diff --git a/lib/jasmine-core/jasmine.js b/lib/jasmine-core/jasmine.js index b2fcceda..1fb3973c 100644 --- a/lib/jasmine-core/jasmine.js +++ b/lib/jasmine-core/jasmine.js @@ -82,6 +82,10 @@ var getJasmineRequireObj = (function(jasmineGlobal) { j$.MapContaining = jRequire.MapContaining(j$); j$.SetContaining = jRequire.SetContaining(j$); j$.QueueRunner = jRequire.QueueRunner(j$); + j$.NeverSkipPolicy = jRequire.NeverSkipPolicy(j$); + j$.CompleteOnFirstErrorSkipPolicy = jRequire.CompleteOnFirstErrorSkipPolicy( + j$ + ); j$.ReportDispatcher = jRequire.ReportDispatcher(j$); j$.Spec = jRequire.Spec(j$); j$.Spy = jRequire.Spy(j$); @@ -1608,12 +1612,17 @@ getJasmineRequireObj().Env = function(j$) { }; var queueRunnerFactory = function(options, args) { - var failFast = false; - if (options.isLeaf) { - failFast = true; - } else if (!options.isReporter) { - failFast = config.stopOnSpecFailure; + if ( + // A spec + options.isLeaf || + // A suite, and config.stopOnSpecFailure is set + (!options.isLeaf && !options.isReporter && config.stopOnSpecFailure) + ) { + options.SkipPolicy = j$.CompleteOnFirstErrorSkipPolicy; + } else { + options.SkipPolicy = j$.NeverSkipPolicy; } + options.clearStack = options.clearStack || clearStack; options.timeout = { setTimeout: realSetTimeout, @@ -1621,7 +1630,6 @@ getJasmineRequireObj().Env = function(j$) { }; options.fail = self.fail; options.globalErrors = globalErrors; - options.completeOnFirstError = failFast; options.onException = options.onException || function(e) { @@ -3380,6 +3388,26 @@ getJasmineRequireObj().Clock = function() { return Clock; }; +getJasmineRequireObj().CompleteOnFirstErrorSkipPolicy = function(j$) { + function CompleteOnFirstErrorSkipPolicy(queueableFns, firstCleanupIx) { + this.queueableFns_ = queueableFns; + this.firstCleanupIx_ = firstCleanupIx; + } + + CompleteOnFirstErrorSkipPolicy.prototype.skipTo = function( + lastRanFnIx, + errored + ) { + if (errored && lastRanFnIx < this.firstCleanupIx_) { + return this.firstCleanupIx_; + } else { + return lastRanFnIx + 1; + } + }; + + return CompleteOnFirstErrorSkipPolicy; +}; + getJasmineRequireObj().DelayedFunctionScheduler = function(j$) { function DelayedFunctionScheduler() { var self = this; @@ -7257,6 +7285,16 @@ getJasmineRequireObj().MockDate = function(j$) { return MockDate; }; +getJasmineRequireObj().NeverSkipPolicy = function(j$) { + function NeverSkipPolicy(queueableFns, firstCleanupIx) {} + + NeverSkipPolicy.prototype.skipTo = function(lastRanFnIx, errored) { + return lastRanFnIx + 1; + }; + + return NeverSkipPolicy; +}; + getJasmineRequireObj().makePrettyPrinter = function(j$) { function SinglePrettyPrintRun(customObjectFormatters, pp) { this.customObjectFormatters_ = customObjectFormatters; @@ -7721,7 +7759,9 @@ getJasmineRequireObj().QueueRunner = function(j$) { pushListener: emptyFn, popListener: emptyFn }; - this.completeOnFirstError = !!attrs.completeOnFirstError; + + const SkipPolicy = attrs.SkipPolicy || j$.NeverSkipPolicy; + this.skipPolicy_ = new SkipPolicy(this.queueableFns, this.firstCleanupIx); this.errored = false; if (typeof this.onComplete !== 'function') { @@ -7742,14 +7782,6 @@ getJasmineRequireObj().QueueRunner = function(j$) { this.run(0); }; - QueueRunner.prototype.skipToCleanup = function(lastRanIndex) { - if (lastRanIndex < this.firstCleanupIx) { - this.run(this.firstCleanupIx); - } else { - this.run(lastRanIndex + 1); - } - }; - QueueRunner.prototype.clearTimeout = function(timeoutId) { Function.prototype.apply.apply(this.timeout.clearTimeout, [ j$.getGlobal(), @@ -7790,11 +7822,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { } function runNext() { - if (self.completeOnFirstError && errored) { - self.skipToCleanup(iterativeIndex); - } else { - self.run(iterativeIndex + 1); - } + self.run(self.nextFnIx_(iterativeIndex)); } if (completedSynchronously) { @@ -7892,7 +7920,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { for ( iterativeIndex = recursiveIndex; iterativeIndex < length; - iterativeIndex++ + iterativeIndex = this.nextFnIx_(iterativeIndex) ) { var result = this.attempt(iterativeIndex); @@ -7901,11 +7929,6 @@ getJasmineRequireObj().QueueRunner = function(j$) { } self.errored = self.errored || result.errored; - - if (this.completeOnFirstError && result.errored) { - this.skipToCleanup(iterativeIndex); - return; - } } this.clearStack(function() { @@ -7919,6 +7942,16 @@ getJasmineRequireObj().QueueRunner = function(j$) { }); }; + QueueRunner.prototype.nextFnIx_ = function(currentFnIx) { + const result = this.skipPolicy_.skipTo(currentFnIx, this.errored); + + if (result === currentFnIx) { + throw new Error("Can't skip to the same queueable fn that just finished"); + } + + return result; + }; + QueueRunner.prototype.diagnoseConflictingAsync_ = function(fn, retval) { var msg; diff --git a/spec/core/CompleteOnFirstErrorSkipPolicySpec.js b/spec/core/CompleteOnFirstErrorSkipPolicySpec.js new file mode 100644 index 00000000..5c746fb7 --- /dev/null +++ b/spec/core/CompleteOnFirstErrorSkipPolicySpec.js @@ -0,0 +1,45 @@ +describe('CompleteOnFirstErrorSkipPolicy', function() { + describe('#skipTo', function() { + describe('When errored is false', function() { + it('returns the next index', function() { + const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy( + arrayOfArbitraryFns(4), + 4 + ); + expect(policy.skipTo(1, false)).toEqual(2); + }); + }); + + describe('When errored is true', function() { + it('returns the first cleanup fn when called with a non cleanup fn', function() { + const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy( + arrayOfArbitraryFns(4), + 2 + ); + + expect(policy.skipTo(0, true)).toEqual(2); + expect(policy.skipTo(1, true)).toEqual(2); + }); + + it('returns the next index when called with a cleanup fn', function() { + const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy( + arrayOfArbitraryFns(4), + 1 + ); + + expect(policy.skipTo(1, true)).toEqual(2); + expect(policy.skipTo(2, true)).toEqual(3); + }); + }); + }); +}); + +function arrayOfArbitraryFns(n) { + const result = []; + + for (let i = 0; i < n; i++) { + result.push({ fn: () => {} }); + } + + return result; +} diff --git a/spec/core/QueueRunnerSpec.js b/spec/core/QueueRunnerSpec.js index d9d68d20..180736f2 100644 --- a/spec/core/QueueRunnerSpec.js +++ b/spec/core/QueueRunnerSpec.js @@ -649,6 +649,68 @@ describe('QueueRunner', function() { expect(nextQueueableFn.fn).toHaveBeenCalled(); }); + describe('When configured with a skip policy', function() { + it('instantiates the skip policy', function() { + const SkipPolicy = jasmine.createSpy('SkipPolicy ctor'); + const queueableFns = [{ fn: () => {} }, { fn: () => {} }]; + const cleanupFns = [{ fn: () => {} }]; + + new jasmineUnderTest.QueueRunner({ + queueableFns, + cleanupFns, + SkipPolicy + }); + + expect(SkipPolicy).toHaveBeenCalledWith( + [...queueableFns, ...cleanupFns], + 2 + ); + }); + + it('uses the skip policy to determine which fn to run next', function() { + const queueableFns = [ + { fn: jasmine.createSpy('fn0') }, + { fn: jasmine.createSpy('fn1') }, + { fn: jasmine.createSpy('fn2').and.throwError(new Error('nope')) }, + { fn: jasmine.createSpy('fn3') } + ]; + const skipPolicy = jasmine.createSpyObj('skipPolicy', ['skipTo']); + skipPolicy.skipTo.and.callFake(function(lastRanIx) { + return lastRanIx === 0 ? 2 : lastRanIx + 1; + }); + const queueRunner = new jasmineUnderTest.QueueRunner({ + queueableFns, + SkipPolicy: function() { + return skipPolicy; + } + }); + + queueRunner.execute(); + + expect(skipPolicy.skipTo).toHaveBeenCalledWith(0, false); + expect(skipPolicy.skipTo).toHaveBeenCalledWith(2, true); + expect(queueableFns[0].fn).toHaveBeenCalled(); + expect(queueableFns[1].fn).not.toHaveBeenCalled(); + expect(queueableFns[2].fn).toHaveBeenCalled(); + expect(queueableFns[3].fn).toHaveBeenCalled(); + }); + + it('throws if the skip policy returns the current fn', function() { + const skipPolicy = { skipTo: i => i }; + const queueableFns = [{ fn: () => {} }]; + const queueRunner = new jasmineUnderTest.QueueRunner({ + queueableFns, + SkipPolicy: function() { + return skipPolicy; + } + }); + + expect(function() { + queueRunner.execute(); + }).toThrowError("Can't skip to the same queueable fn that just finished"); + }); + }); + describe('When configured to complete on first error', function() { it('skips to cleanup functions on the first exception', function() { var queueableFn = { @@ -663,7 +725,7 @@ describe('QueueRunner', function() { queueableFns: [queueableFn, nextQueueableFn], cleanupFns: [cleanupFn], onComplete: onComplete, - completeOnFirstError: true + SkipPolicy: jasmineUnderTest.CompleteOnFirstErrorSkipPolicy }); queueRunner.execute(); @@ -685,7 +747,7 @@ describe('QueueRunner', function() { queueRunner = new jasmineUnderTest.QueueRunner({ queueableFns: [queueableFn], cleanupFns: [cleanupFn1, cleanupFn2], - completeOnFirstError: true + SkipPolicy: jasmineUnderTest.CompleteOnFirstErrorSkipPolicy }); queueRunner.execute(); @@ -721,7 +783,7 @@ describe('QueueRunner', function() { }, queueableFns: [queueableFn, nextQueueableFn], cleanupFns: [cleanupFn], - completeOnFirstError: true + SkipPolicy: jasmineUnderTest.CompleteOnFirstErrorSkipPolicy }), queueableFnDone; @@ -744,7 +806,7 @@ describe('QueueRunner', function() { queueRunner = new jasmineUnderTest.QueueRunner({ queueableFns: [queueableFn, nextQueueableFn], cleanupFns: [cleanupFn], - completeOnFirstError: true + SkipPolicy: jasmineUnderTest.CompleteOnFirstErrorSkipPolicy }); queueRunner.execute(); @@ -764,7 +826,7 @@ describe('QueueRunner', function() { queueRunner = new jasmineUnderTest.QueueRunner({ queueableFns: [queueableFn, nextQueueableFn], cleanupFns: [cleanupFn], - completeOnFirstError: true + SkipPolicy: jasmineUnderTest.CompleteOnFirstErrorSkipPolicy }); queueRunner.execute(); diff --git a/src/core/CompleteOnFirstErrorSkipPolicy.js b/src/core/CompleteOnFirstErrorSkipPolicy.js new file mode 100644 index 00000000..89a6070f --- /dev/null +++ b/src/core/CompleteOnFirstErrorSkipPolicy.js @@ -0,0 +1,19 @@ +getJasmineRequireObj().CompleteOnFirstErrorSkipPolicy = function(j$) { + function CompleteOnFirstErrorSkipPolicy(queueableFns, firstCleanupIx) { + this.queueableFns_ = queueableFns; + this.firstCleanupIx_ = firstCleanupIx; + } + + CompleteOnFirstErrorSkipPolicy.prototype.skipTo = function( + lastRanFnIx, + errored + ) { + if (errored && lastRanFnIx < this.firstCleanupIx_) { + return this.firstCleanupIx_; + } else { + return lastRanFnIx + 1; + } + }; + + return CompleteOnFirstErrorSkipPolicy; +}; diff --git a/src/core/Env.js b/src/core/Env.js index c6c53950..15f06f6a 100644 --- a/src/core/Env.js +++ b/src/core/Env.js @@ -502,12 +502,17 @@ getJasmineRequireObj().Env = function(j$) { }; var queueRunnerFactory = function(options, args) { - var failFast = false; - if (options.isLeaf) { - failFast = true; - } else if (!options.isReporter) { - failFast = config.stopOnSpecFailure; + if ( + // A spec + options.isLeaf || + // A suite, and config.stopOnSpecFailure is set + (!options.isLeaf && !options.isReporter && config.stopOnSpecFailure) + ) { + options.SkipPolicy = j$.CompleteOnFirstErrorSkipPolicy; + } else { + options.SkipPolicy = j$.NeverSkipPolicy; } + options.clearStack = options.clearStack || clearStack; options.timeout = { setTimeout: realSetTimeout, @@ -515,7 +520,6 @@ getJasmineRequireObj().Env = function(j$) { }; options.fail = self.fail; options.globalErrors = globalErrors; - options.completeOnFirstError = failFast; options.onException = options.onException || function(e) { diff --git a/src/core/NeverSkipPolicy.js b/src/core/NeverSkipPolicy.js new file mode 100644 index 00000000..c256bd3c --- /dev/null +++ b/src/core/NeverSkipPolicy.js @@ -0,0 +1,9 @@ +getJasmineRequireObj().NeverSkipPolicy = function(j$) { + function NeverSkipPolicy(queueableFns, firstCleanupIx) {} + + NeverSkipPolicy.prototype.skipTo = function(lastRanFnIx, errored) { + return lastRanFnIx + 1; + }; + + return NeverSkipPolicy; +}; diff --git a/src/core/QueueRunner.js b/src/core/QueueRunner.js index 8d485424..03089d0c 100644 --- a/src/core/QueueRunner.js +++ b/src/core/QueueRunner.js @@ -56,7 +56,9 @@ getJasmineRequireObj().QueueRunner = function(j$) { pushListener: emptyFn, popListener: emptyFn }; - this.completeOnFirstError = !!attrs.completeOnFirstError; + + const SkipPolicy = attrs.SkipPolicy || j$.NeverSkipPolicy; + this.skipPolicy_ = new SkipPolicy(this.queueableFns, this.firstCleanupIx); this.errored = false; if (typeof this.onComplete !== 'function') { @@ -77,14 +79,6 @@ getJasmineRequireObj().QueueRunner = function(j$) { this.run(0); }; - QueueRunner.prototype.skipToCleanup = function(lastRanIndex) { - if (lastRanIndex < this.firstCleanupIx) { - this.run(this.firstCleanupIx); - } else { - this.run(lastRanIndex + 1); - } - }; - QueueRunner.prototype.clearTimeout = function(timeoutId) { Function.prototype.apply.apply(this.timeout.clearTimeout, [ j$.getGlobal(), @@ -125,11 +119,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { } function runNext() { - if (self.completeOnFirstError && errored) { - self.skipToCleanup(iterativeIndex); - } else { - self.run(iterativeIndex + 1); - } + self.run(self.nextFnIx_(iterativeIndex)); } if (completedSynchronously) { @@ -227,7 +217,7 @@ getJasmineRequireObj().QueueRunner = function(j$) { for ( iterativeIndex = recursiveIndex; iterativeIndex < length; - iterativeIndex++ + iterativeIndex = this.nextFnIx_(iterativeIndex) ) { var result = this.attempt(iterativeIndex); @@ -236,11 +226,6 @@ getJasmineRequireObj().QueueRunner = function(j$) { } self.errored = self.errored || result.errored; - - if (this.completeOnFirstError && result.errored) { - this.skipToCleanup(iterativeIndex); - return; - } } this.clearStack(function() { @@ -254,6 +239,16 @@ getJasmineRequireObj().QueueRunner = function(j$) { }); }; + QueueRunner.prototype.nextFnIx_ = function(currentFnIx) { + const result = this.skipPolicy_.skipTo(currentFnIx, this.errored); + + if (result === currentFnIx) { + throw new Error("Can't skip to the same queueable fn that just finished"); + } + + return result; + }; + QueueRunner.prototype.diagnoseConflictingAsync_ = function(fn, retval) { var msg; diff --git a/src/core/requireCore.js b/src/core/requireCore.js index a185bd47..08c1e006 100644 --- a/src/core/requireCore.js +++ b/src/core/requireCore.js @@ -60,6 +60,10 @@ var getJasmineRequireObj = (function(jasmineGlobal) { j$.MapContaining = jRequire.MapContaining(j$); j$.SetContaining = jRequire.SetContaining(j$); j$.QueueRunner = jRequire.QueueRunner(j$); + j$.NeverSkipPolicy = jRequire.NeverSkipPolicy(j$); + j$.CompleteOnFirstErrorSkipPolicy = jRequire.CompleteOnFirstErrorSkipPolicy( + j$ + ); j$.ReportDispatcher = jRequire.ReportDispatcher(j$); j$.Spec = jRequire.Spec(j$); j$.Spy = jRequire.Spy(j$);