Skip afterEach fns in nested suites when a beforeEach fn errors

This matches the behavior of beforeAll errors.

* #1533

lib/jasmine-core/jasmine.js

@@ -3397,22 +3397,47 @@ getJasmineRequireObj().Clock = function() {
 
 getJasmineRequireObj().CompleteOnFirstErrorSkipPolicy = function(j$) {
   function CompleteOnFirstErrorSkipPolicy(queueableFns, firstCleanupIx) {
+    this.queueableFns_ = queueableFns;
     this.firstCleanupIx_ = firstCleanupIx;
-    this.skipping_ = false;
+    this.erroredFnIx_ = null;
   }
 
   CompleteOnFirstErrorSkipPolicy.prototype.skipTo = function(lastRanFnIx) {
-    if (this.skipping_ && lastRanFnIx < this.firstCleanupIx_) {
-      return this.firstCleanupIx_;
-    } else {
-      return lastRanFnIx + 1;
-    }
+    for (
+      i = lastRanFnIx + 1;
+      i < this.queueableFns_.length && this.shouldSkip_(i);
+      i++
+    ) {}
+    return i;
   };
 
   CompleteOnFirstErrorSkipPolicy.prototype.fnErrored = function(fnIx) {
-    this.skipping_ = true;
+    this.erroredFnIx_ = fnIx;
   };
 
+  CompleteOnFirstErrorSkipPolicy.prototype.shouldSkip_ = function(fnIx) {
+    if (this.erroredFnIx_ === null) {
+      return false;
+    }
+
+    const candidateSuite = this.queueableFns_[fnIx].suite;
+    const errorSuite = this.queueableFns_[this.erroredFnIx_].suite;
+    return (
+      fnIx < this.firstCleanupIx_ ||
+      (candidateSuite && isDescendent(candidateSuite, errorSuite))
+    );
+  };
+
+  function isDescendent(candidate, ancestor) {
+    if (!candidate.parentSuite) {
+      return false;
+    } else if (candidate.parentSuite === ancestor) {
+      return true;
+    } else {
+      return isDescendent(candidate.parentSuite, ancestor);
+    }
+  }
+
   return CompleteOnFirstErrorSkipPolicy;
 };
 
@@ -9501,7 +9526,7 @@ getJasmineRequireObj().Suite = function(j$) {
   };
 
   Suite.prototype.beforeEach = function(fn) {
-    this.beforeFns.unshift(fn);
+    this.beforeFns.unshift({ ...fn, suite: this });
   };
 
   Suite.prototype.beforeAll = function(fn) {
@@ -9509,7 +9534,7 @@ getJasmineRequireObj().Suite = function(j$) {
   };
 
   Suite.prototype.afterEach = function(fn) {
-    this.afterFns.unshift(fn);
+    this.afterFns.unshift({ ...fn, suite: this });
   };
 
   Suite.prototype.afterAll = function(fn) {

spec/core/CompleteOnFirstErrorSkipPolicySpec.js

@@ -11,26 +11,89 @@ describe('CompleteOnFirstErrorSkipPolicy', function() {
     });
 
     describe('After something has errored', function() {
-      it('returns the first cleanup fn when called with a non cleanup fn', function() {
+      it('skips non cleanup fns', function() {
+        const fns = arrayOfArbitraryFns(4);
         const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy(
-          arrayOfArbitraryFns(4),
+          fns,
           2
         );
 
         policy.fnErrored(0);
         expect(policy.skipTo(0)).toEqual(2);
-        expect(policy.skipTo(1)).toEqual(2);
+        expect(policy.skipTo(2)).toEqual(3);
+        expect(policy.skipTo(3)).toEqual(4);
       });
 
-      it('returns the next index when called with a cleanup fn', function() {
+      describe('When the error was in a beforeEach fn', function() {
+        it('runs cleanup fns defined by the current and containing suites', function() {
+          const parentSuite = { description: 'parentSuite' };
+          const suite = { description: 'suite', parentSuite };
+          const fns = [
+            {
+              suite: suite
+            },
+            {
+              fn: () => {}
+            },
+            {
+              fn: () => {},
+              suite: suite
+            },
+            {
+              fn: () => {},
+              suite: parentSuite
+            }
+          ];
+          const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy(
+            fns,
+            2
+          );
+
+          policy.fnErrored(0);
+          expect(policy.skipTo(0)).toEqual(2);
+          expect(policy.skipTo(2)).toEqual(3);
+        });
+
+        it('skips cleanup fns defined by nested suites', function() {
+          const parentSuite = { description: 'parentSuite' };
+          const suite = { description: 'suite', parentSuite };
+          const fns = [
+            {
+              fn: () => {},
+              type: 'beforeEach',
+              suite: parentSuite
+            },
+            {
+              fn: () => {}
+            },
+            {
+              fn: () => {},
+              suite: suite
+            },
+            {
+              fn: () => {},
+              suite: parentSuite
+            }
+          ];
+          const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy(
+            fns,
+            2
+          );
+
+          policy.fnErrored(0);
+          expect(policy.skipTo(0)).toEqual(3);
+        });
+      });
+
+      it('does not skip cleanup fns that have no suite, such as the spec complete fn', function() {
+        const fns = [{ fn: () => {} }, { fn: () => {} }];
         const policy = new jasmineUnderTest.CompleteOnFirstErrorSkipPolicy(
-          arrayOfArbitraryFns(4),
+          fns,
           1
         );
 
         policy.fnErrored(0);
-        expect(policy.skipTo(1)).toEqual(2);
-        expect(policy.skipTo(2)).toEqual(3);
+        expect(policy.skipTo(0)).toEqual(1);
       });
     });
   });

spec/core/SuiteSpec.js

@@ -48,13 +48,16 @@ describe('Suite', function() {
         env: env,
         description: 'I am a suite'
       }),
-      outerBefore = jasmine.createSpy('outerBeforeEach'),
-      innerBefore = jasmine.createSpy('insideBeforeEach');
+      outerBefore = { fn: 'outerBeforeEach' },
+      innerBefore = { fn: 'insideBeforeEach' };
 
     suite.beforeEach(outerBefore);
     suite.beforeEach(innerBefore);
 
-    expect(suite.beforeFns).toEqual([innerBefore, outerBefore]);
+    expect(suite.beforeFns).toEqual([
+      { fn: innerBefore.fn, suite },
+      { fn: outerBefore.fn, suite }
+    ]);
   });
 
   it('adds after functions in order of needed execution', function() {
@@ -62,13 +65,16 @@ describe('Suite', function() {
         env: env,
         description: 'I am a suite'
       }),
-      outerAfter = jasmine.createSpy('outerAfterEach'),
-      innerAfter = jasmine.createSpy('insideAfterEach');
+      outerAfter = { fn: 'outerAfterEach' },
+      innerAfter = { fn: 'insideAfterEach' };
 
     suite.afterEach(outerAfter);
     suite.afterEach(innerAfter);
 
-    expect(suite.afterFns).toEqual([innerAfter, outerAfter]);
+    expect(suite.afterFns).toEqual([
+      { fn: innerAfter.fn, suite },
+      { fn: outerAfter.fn, suite }
+    ]);
   });
 
   it('has a status of failed if any expectations have failed', function() {

spec/core/integration/SpecRunningSpec.js

@@ -824,11 +824,7 @@ describe('spec running', function() {
       });
 
       env.execute(null, function() {
-        expect(actions).toEqual([
-          'outer beforeEach',
-          'inner afterEach',
-          'outer afterEach'
-        ]);
+        expect(actions).toEqual(['outer beforeEach', 'outer afterEach']);
         done();
       });
     });
@@ -865,11 +861,7 @@ describe('spec running', function() {
 
       await env.execute();
 
-      expect(actions).toEqual([
-        'outer beforeEach',
-        'inner afterEach',
-        'outer afterEach'
-      ]);
+      expect(actions).toEqual(['outer beforeEach', 'outer afterEach']);
     });
 
     it('skips to cleanup functions after an expectation failure', async function() {
@@ -904,11 +896,7 @@ describe('spec running', function() {
 
       await env.execute();
 
-      expect(actions).toEqual([
-        'outer beforeEach',
-        'inner afterEach',
-        'outer afterEach'
-      ]);
+      expect(actions).toEqual(['outer beforeEach', 'outer afterEach']);
     });
 
     it('skips to cleanup functions after done.fail is called', function(done) {
@@ -1013,11 +1001,7 @@ describe('spec running', function() {
 
       await env.execute();
 
-      expect(actions).toEqual([
-        'outer beforeEach',
-        'inner afterEach',
-        'outer afterEach'
-      ]);
+      expect(actions).toEqual(['outer beforeEach', 'outer afterEach']);
     });
 
     it('skips to cleanup functions after a rejected promise', async function() {
@@ -1050,11 +1034,7 @@ describe('spec running', function() {
 
       await env.execute();
 
-      expect(actions).toEqual([
-        'outer beforeEach',
-        'inner afterEach',
-        'outer afterEach'
-      ]);
+      expect(actions).toEqual(['outer beforeEach', 'outer afterEach']);
     });
 
     it('does not skip anything after an expectation failure', async function() {
@@ -1141,6 +1121,29 @@ describe('spec running', function() {
       expect(actions).toEqual(['beforeEach', 'afterEach']);
     });
 
+    it('skips cleanup functions that are defined in child suites when a beforeEach errors', async function() {
+      const parentAfterEachFn = jasmine.createSpy('parentAfterEachFn');
+      const childAfterEachFn = jasmine.createSpy('childAfterEachFn');
+
+      env.describe('parent suite', function() {
+        env.beforeEach(function() {
+          throw new Error('nope');
+        });
+
+        env.afterEach(parentAfterEachFn);
+
+        env.describe('child suite', function() {
+          env.it('a spec', function() {});
+          env.afterEach(childAfterEachFn);
+        });
+      });
+
+      await env.execute();
+
+      expect(parentAfterEachFn).toHaveBeenCalled();
+      expect(childAfterEachFn).not.toHaveBeenCalled();
+    });
+
     it('runs all reporter callbacks even if one fails', async function() {
       var laterReporter = jasmine.createSpyObj('laterReporter', ['specDone']);
 

src/core/CompleteOnFirstErrorSkipPolicy.js

@@ -1,20 +1,45 @@
 getJasmineRequireObj().CompleteOnFirstErrorSkipPolicy = function(j$) {
   function CompleteOnFirstErrorSkipPolicy(queueableFns, firstCleanupIx) {
+    this.queueableFns_ = queueableFns;
     this.firstCleanupIx_ = firstCleanupIx;
-    this.skipping_ = false;
+    this.erroredFnIx_ = null;
   }
 
   CompleteOnFirstErrorSkipPolicy.prototype.skipTo = function(lastRanFnIx) {
-    if (this.skipping_ && lastRanFnIx < this.firstCleanupIx_) {
-      return this.firstCleanupIx_;
-    } else {
-      return lastRanFnIx + 1;
-    }
+    for (
+      i = lastRanFnIx + 1;
+      i < this.queueableFns_.length && this.shouldSkip_(i);
+      i++
+    ) {}
+    return i;
   };
 
   CompleteOnFirstErrorSkipPolicy.prototype.fnErrored = function(fnIx) {
-    this.skipping_ = true;
+    this.erroredFnIx_ = fnIx;
   };
 
+  CompleteOnFirstErrorSkipPolicy.prototype.shouldSkip_ = function(fnIx) {
+    if (this.erroredFnIx_ === null) {
+      return false;
+    }
+
+    const candidateSuite = this.queueableFns_[fnIx].suite;
+    const errorSuite = this.queueableFns_[this.erroredFnIx_].suite;
+    return (
+      fnIx < this.firstCleanupIx_ ||
+      (candidateSuite && isDescendent(candidateSuite, errorSuite))
+    );
+  };
+
+  function isDescendent(candidate, ancestor) {
+    if (!candidate.parentSuite) {
+      return false;
+    } else if (candidate.parentSuite === ancestor) {
+      return true;
+    } else {
+      return isDescendent(candidate.parentSuite, ancestor);
+    }
+  }
+
   return CompleteOnFirstErrorSkipPolicy;
 };

src/core/Suite.js

@@ -105,7 +105,7 @@ getJasmineRequireObj().Suite = function(j$) {
   };
 
   Suite.prototype.beforeEach = function(fn) {
-    this.beforeFns.unshift(fn);
+    this.beforeFns.unshift({ ...fn, suite: this });
   };
 
   Suite.prototype.beforeAll = function(fn) {
@@ -113,7 +113,7 @@ getJasmineRequireObj().Suite = function(j$) {
   };
 
   Suite.prototype.afterEach = function(fn) {
-    this.afterFns.unshift(fn);
+    this.afterFns.unshift({ ...fn, suite: this });
   };
 
   Suite.prototype.afterAll = function(fn) {