Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: simplify test time tracking #52182

Closed
wants to merge 2 commits into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 21, 2024

This PR simplifies some of the logic that has accumulated over time for tracking test durations.

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().

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.

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().
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 21, 2024
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Mar 24, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get why these changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec reporter only shows times if they are non-zero. Those values were coming through as zeros before. I have now updated the PR as shown below. That seems to keep the snapshots the same, which can also save a second hrtime() call. Please take another look.

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index f61aada409..9f9c47edae 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -754,8 +754,8 @@ class Test extends AsyncResource {
   postRun(pendingSubtestsError) {
     // If the test was cancelled before it started, then the start and end
     // times need to be corrected.
-    this.startTime ??= hrtime();
     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.

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.
@nodejs-github-bot
Copy link
Collaborator

@cjihrig cjihrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Mar 25, 2024
@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in a8b21fd...6af4049

nodejs-github-bot pushed a commit that referenced this pull request Mar 25, 2024
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().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 25, 2024
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 <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@cjihrig cjihrig deleted the test-times branch March 25, 2024 13:27
anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
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().

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
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: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
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().

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
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: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
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().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
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 <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
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().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
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 <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants