test_runner: simplify test end time tracking

cjihrig · nodejs-github-bot · commit 6af404981030 · 2024-03-25T13:23:35.000Z
This commit simplifies the logic for tracking test end time. The end time is now only set in postRun(), which every test runs when it ends. PR-URL: #52182 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -526,7 +526,7 @@ class Test extends AsyncResource {
   };
 
   #cancel(error) {
-    if (this.endTime !== null) {
+    if (this.endTime !== null || this.error !== null) {
       return;
     }
 
@@ -564,17 +564,15 @@ class Test extends AsyncResource {
       return;
     }
 
-    this.endTime = hrtime();
     this.passed = false;
     this.error = err;
   }
 
   pass() {
-    if (this.endTime !== null) {
+    if (this.error !== null) {
       return;
     }
 
-    this.endTime = hrtime();
     this.passed = true;
   }
 
@@ -707,15 +705,8 @@ class Test extends AsyncResource {
       }
 
       this.pass();
-      try {
-        await afterEach();
-        await after();
-      } catch (err) {
-        // If one of the after hooks has thrown unset endTime so that the
-        // catch below can do its cancel/fail logic.
-        this.endTime = null;
-        throw err;
-      }
+      await afterEach();
+      await after();
     } catch (err) {
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
@@ -761,13 +752,10 @@ class Test extends AsyncResource {
   }
 
   postRun(pendingSubtestsError) {
-    this.startTime ??= hrtime();
-
-    // If the test was failed before it even started, then the end time will
-    // be earlier than the start time. Correct that here.
-    if (this.endTime < this.startTime) {
-      this.endTime = hrtime();
-    }
+    // If the test was cancelled before it started, then the start and end
+    // times need to be corrected.
+    this.endTime ??= hrtime();
+    this.startTime ??= this.endTime;
 
     // The test has run, so recursively cancel any outstanding subtests and
     // mark this test as failed if any subtests failed.
@@ -974,6 +962,7 @@ class TestHook extends Test {
         error.failureType = kHookFailure;
       }
 
+      this.endTime ??= hrtime();
       parent.reporter.fail(0, loc, parent.subtests.length + 1, loc.file, {
         __proto__: null,
         duration_ms: this.duration(),
diff --git a/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot b/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot
@@ -11,10 +11,10 @@
 
  describe hooks - no subtests (*ms)
  before throws
-   1 (*ms)
+   1
     'test did not finish before its parent and was cancelled'
 
-   2 (*ms)
+   2
     'test did not finish before its parent and was cancelled'
 
  before throws (*ms)
@@ -390,7 +390,7 @@
 
  - after() called
  run after when before throws
-   1 (*ms)
+   1
     'test did not finish before its parent and was cancelled'
 
  run after when before throws (*ms)
@@ -422,11 +422,11 @@
  failing tests:
 
 *
- 1 (*ms)
+ 1
   'test did not finish before its parent and was cancelled'
 
 *
- 2 (*ms)
+ 2
   'test did not finish before its parent and was cancelled'
 
 *
@@ -772,7 +772,7 @@
       *
 
 *
- 1 (*ms)
+ 1
   'test did not finish before its parent and was cancelled'
 
 *

Original file line number	Diff line number	Diff line change
`@@ -11,10 +11,10 @@`
`11`	`11`
`12`	`12`	`describe hooks - no subtests (*ms)`
`13`	`13`	`before throws`
`14`		`- 1 (*ms)`
	`14`	`+ 1`
`15`	`15`	`'test did not finish before its parent and was cancelled'`
`16`	`16`
`17`		`- 2 (*ms)`
	`17`	`+ 2`
`18`	`18`	`'test did not finish before its parent and was cancelled'`
`19`	`19`
`20`	`20`	`before throws (*ms)`
`@@ -390,7 +390,7 @@`
`390`	`390`
`391`	`391`	`- after() called`
`392`	`392`	`run after when before throws`
`393`		`- 1 (*ms)`
	`393`	`+ 1`
`394`	`394`	`'test did not finish before its parent and was cancelled'`
`395`	`395`
`396`	`396`	`run after when before throws (*ms)`
`@@ -422,11 +422,11 @@`
`422`	`422`	`failing tests:`
`423`	`423`
`424`	`424`	`*`
`425`		`- 1 (*ms)`
	`425`	`+ 1`
`426`	`426`	`'test did not finish before its parent and was cancelled'`
`427`	`427`
`428`	`428`	`*`
`429`		`- 2 (*ms)`
	`429`	`+ 2`
`430`	`430`	`'test did not finish before its parent and was cancelled'`
`431`	`431`
`432`	`432`	`*`
`@@ -772,7 +772,7 @@`
`772`	`772`	`*`
`773`	`773`
`774`	`774`	`*`
`775`		`- 1 (*ms)`
	`775`	`+ 1`
`776`	`776`	`'test did not finish before its parent and was cancelled'`
`777`	`777`
`778`	`778`	`*`