More reliably report errors that occur late in the suite/spec lifecycle

Previously, an error that occurred after Jasmine started to report the suiteDone or specDone event for the current runable would not be reliably reported. Now such an error is reported on the nearest ancestor suite whose suiteDone event has not yet been reported.
2022-05-08 15:29:58 -07:00
parent 9ea8a2096f
commit bbb1b69b2e
8 changed files with 610 additions and 107 deletions
--- a/src/core/Env.js
+++ b/src/core/Env.js
@@ -342,16 +342,21 @@ getJasmineRequireObj().Env = function(j$) {
      }
    };

+    // TODO: Unify recordLateError with recordLateExpectation? The extra
+    // diagnostic info added by the latter is probably useful in most cases.
    function recordLateError(error) {
-      const result = expectationResultFactory({
-        error,
-        passed: false,
-        matcherName: '',
-        expected: '',
-        actual: ''
-      });
-      result.globalErrorType = 'lateError';
-      topSuite.result.failedExpectations.push(result);
+      const isExpectationResult =
+        error.matcherName !== undefined && error.passed !== undefined;
+      const result = isExpectationResult
+        ? error
+        : expectationResultFactory({
+            error,
+            passed: false,
+            matcherName: '',
+            expected: '',
+            actual: ''
+          });
+      routeLateFailure(result);
    }

    function recordLateExpectation(runable, runableType, result) {
@@ -382,6 +387,26 @@ getJasmineRequireObj().Env = function(j$) {
      topSuite.result.failedExpectations.push(delayedExpectationResult);
    }

+    function routeLateFailure(expectationResult) {
+      // Report the result on the nearest ancestor suite that hasn't already
+      // been reported done.
+      for (let r = currentRunnable(); r; r = r.parentSuite) {
+        if (!r.reportedDone) {
+          if (r === topSuite) {
+            expectationResult.globalErrorType = 'lateError';
+          }
+
+          r.result.failedExpectations.push(expectationResult);
+          return;
+        }
+      }
+
+      // If we get here, all results have been reported and there's nothing we
+      // can do except log the result and hope the user sees it.
+      console.error('Jasmine received a result after the suite finished:');
+      console.error(expectationResult);
+    }
+
    var asyncExpectationFactory = function(actual, spec, runableType) {
      return j$.Expectation.asyncFactory({
        matchersUtil: makeMatchersUtil(),
@@ -537,7 +562,7 @@ getJasmineRequireObj().Env = function(j$) {
      options.onException =
        options.onException ||
        function(e) {
-          (currentRunnable() || topSuite).onException(e);
+          (currentRunnable() || topSuite).handleException(e);
        };
      options.deprecated = self.deprecated;

@@ -722,10 +747,10 @@ getJasmineRequireObj().Env = function(j$) {

          if (suite.hadBeforeAllFailure) {
            reportChildrenOfBeforeAllFailure(suite).then(function() {
-              reporter.suiteDone(result, next);
+              reportSuiteDone(suite, result, next);
            });
          } else {
-            reporter.suiteDone(result, next);
+            reportSuiteDone(suite, result, next);
          }
        },
        orderChildren: function(node) {
@@ -815,6 +840,7 @@ getJasmineRequireObj().Env = function(j$) {
                  failedExpectations: topSuite.result.failedExpectations,
                  deprecationWarnings: topSuite.result.deprecationWarnings
                };
+                topSuite.reportedDone = true;
                reporter.jasmineDone(jasmineDoneInfo, function() {
                  done(jasmineDoneInfo);
                });
@@ -859,7 +885,7 @@ getJasmineRequireObj().Env = function(j$) {
            child.result.status = 'failed';

            await new Promise(function(resolve) {
-              reporter.specDone(child.result, resolve);
+              reportSpecDone(child, child.result, resolve);
            });
          }
        }
@@ -1073,7 +1099,7 @@ getJasmineRequireObj().Env = function(j$) {
      }

      if (declarationError) {
-        suite.onException(declarationError);
+        suite.handleException(declarationError);
      }

      currentDeclarationSuite = parentSuite;
@@ -1139,7 +1165,7 @@ getJasmineRequireObj().Env = function(j$) {
          hasFailures = true;
        }

-        reporter.specDone(result, next);
+        reportSpecDone(spec, result, next);
      }

      function specStarted(spec, next) {
@@ -1149,6 +1175,16 @@ getJasmineRequireObj().Env = function(j$) {
      }
    };

+    function reportSpecDone(spec, result, next) {
+      spec.reportedDone = true;
+      reporter.specDone(result, next);
+    }
+
+    function reportSuiteDone(suite, result, next) {
+      suite.reportedDone = true;
+      reporter.suiteDone(result, next);
+    }
+
    this.it_ = function(description, fn, timeout) {
      ensureIsNotNested('it');
      // it() sometimes doesn't have a fn argument, so only check the type if
--- a/src/core/Spec.js
+++ b/src/core/Spec.js
@@ -86,14 +86,20 @@ getJasmineRequireObj().Spec = function(j$) {
      properties: null,
      debugLogs: null
    };
+
+    this.reportedDone = false;
  }

  Spec.prototype.addExpectationResult = function(passed, data, isError) {
-    var expectationResult = this.expectationResultFactory(data);
+    const expectationResult = this.expectationResultFactory(data);
    if (passed) {
      this.result.passedExpectations.push(expectationResult);
    } else {
-      this.result.failedExpectations.push(expectationResult);
+      if (this.reportedDone) {
+        this.onLateError(expectationResult);
+      } else {
+        this.result.failedExpectations.push(expectationResult);
+      }

      if (this.throwOnExpectationFailure && !isError) {
        throw new j$.errors.ExpectationFailed();
@@ -147,7 +153,7 @@ getJasmineRequireObj().Spec = function(j$) {
      isLeaf: true,
      queueableFns: [...fns.befores, this.queueableFn, ...fns.afters],
      onException: function() {
-        self.onException.apply(self, arguments);
+        self.handleException.apply(self, arguments);
      },
      onMultipleDone: function() {
        // Issue a deprecation. Include the context ourselves and pass
@@ -197,9 +203,10 @@ getJasmineRequireObj().Spec = function(j$) {
      debugLogs: null
    };
    this.markedPending = this.markedExcluding;
+    this.reportedDone = false;
  };

-  Spec.prototype.onException = function onException(e) {
+  Spec.prototype.handleException = function handleException(e) {
    if (Spec.isPendingSpecException(e)) {
      this.pend(extractCustomPendingMessage(e));
      return;
--- a/src/core/Suite.js
+++ b/src/core/Suite.js
@@ -29,7 +29,7 @@ getJasmineRequireObj().Suite = function(j$) {
    this.throwOnExpectationFailure = !!attrs.throwOnExpectationFailure;
    this.autoCleanClosures =
      attrs.autoCleanClosures === undefined ? true : !!attrs.autoCleanClosures;
-    this.onLateError = attrs.onLateError;
+    this.onLateError = attrs.onLateError || function() {};

    this.beforeFns = [];
    this.afterFns = [];
@@ -163,6 +163,7 @@ getJasmineRequireObj().Suite = function(j$) {
    this.children.forEach(function(child) {
      child.reset();
    });
+    this.reportedDone = false;
  };

  Suite.prototype.addChild = function(child) {
@@ -204,7 +205,7 @@ getJasmineRequireObj().Suite = function(j$) {
    return j$.UserContext.fromExisting(this.sharedUserContext());
  };

-  Suite.prototype.onException = function() {
+  Suite.prototype.handleException = function() {
    if (arguments[0] instanceof j$.errors.ExpectationFailed) {
      return;
    }
@@ -222,7 +223,11 @@ getJasmineRequireObj().Suite = function(j$) {
      failedExpectation.globalErrorType = 'afterAll';
    }

-    this.result.failedExpectations.push(failedExpectation);
+    if (this.reportedDone) {
+      this.onLateError(failedExpectation);
+    } else {
+      this.result.failedExpectations.push(failedExpectation);
+    }
  };

  Suite.prototype.onMultipleDone = function() {
@@ -249,8 +254,15 @@ getJasmineRequireObj().Suite = function(j$) {

  Suite.prototype.addExpectationResult = function() {
    if (isFailure(arguments)) {
-      var data = arguments[1];
-      this.result.failedExpectations.push(this.expectationResultFactory(data));
+      const data = arguments[1];
+      const expectationResult = this.expectationResultFactory(data);
+
+      if (this.reportedDone) {
+        this.onLateError(expectationResult);
+      } else {
+        this.result.failedExpectations.push(expectationResult);
+      }
+
      if (this.throwOnExpectationFailure) {
        throw new j$.errors.ExpectationFailed();
      }
--- a/src/core/TreeProcessor.js
+++ b/src/core/TreeProcessor.js
@@ -42,7 +42,7 @@ getJasmineRequireObj().TreeProcessor = function() {
        queueableFns: childFns,
        userContext: tree.sharedUserContext(),
        onException: function() {
-          tree.onException.apply(tree, arguments);
+          tree.handleException.apply(tree, arguments);
        },
        onComplete: done,
        onMultipleDone: tree.onMultipleDone
@@ -218,7 +218,7 @@ getJasmineRequireObj().TreeProcessor = function() {
              queueableFns: [onStart].concat(wrapChildren(node, segmentNumber)),
              userContext: node.sharedUserContext(),
              onException: function() {
-                node.onException.apply(node, arguments);
+                node.handleException.apply(node, arguments);
              },
              onMultipleDone: node.onMultipleDone
                ? node.onMultipleDone.bind(node)