From f2e7bcc41eb1bf27acaa0addde34210cc9ddbcf7 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 21 Mar 2024 13:28:06 -0400 Subject: [PATCH 1/2] test_runner: simplify test start time tracking This commit simplifies the logic for tracking test start time. The start time is now set only when a test/suite begins running. If the test/suite never runs, a fallback is provided in postRun(). --- lib/internal/test_runner/test.js | 5 ++--- .../test-runner/output/hooks_spec_reporter.snapshot | 12 ++++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index b12febe824d2d7..d5a07cbd6f757a 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -507,7 +507,6 @@ class Test extends AsyncResource { } if (preventAddingSubtests) { - test.startTime = test.startTime || hrtime(); test.fail( new ERR_TEST_FAILURE( 'test could not be started because its parent finished', @@ -537,7 +536,6 @@ class Test extends AsyncResource { kCancelledByParent, ), ); - this.startTime = this.startTime || this.endTime; // If a test was canceled before it was started, e.g inside a hook this.cancelled = true; this.abortController.abort(); } @@ -763,12 +761,13 @@ 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(); } - this.startTime ??= this.endTime; // The test has run, so recursively cancel any outstanding subtests and // mark this test as failed if any subtests failed. diff --git a/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot b/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot index 4158335409aba1..a4f40e4321ff89 100644 --- 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 + 1 (*ms) 'test did not finish before its parent and was cancelled' - 2 + 2 (*ms) '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 + 1 (*ms) 'test did not finish before its parent and was cancelled' run after when before throws (*ms) @@ -422,11 +422,11 @@ failing tests: * - 1 + 1 (*ms) 'test did not finish before its parent and was cancelled' * - 2 + 2 (*ms) 'test did not finish before its parent and was cancelled' * @@ -772,7 +772,7 @@ * * - 1 + 1 (*ms) 'test did not finish before its parent and was cancelled' * From 74700655a7556b3065dbe08f7f48ca58417b6d57 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 21 Mar 2024 14:09:23 -0400 Subject: [PATCH 2/2] test_runner: simplify test end time tracking 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. --- lib/internal/test_runner/test.js | 29 ++++++------------- .../output/hooks_spec_reporter.snapshot | 12 ++++---- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index d5a07cbd6f757a..9f9c47edae7f62 100644 --- 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 index a4f40e4321ff89..4158335409aba1 100644 --- 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' *