Skip to content

test_runner: recalculate run duration on watch restart - #57786

Merged
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
pmarchini:test_runner/fix/test-runner-watch-duration-update
Apr 16, 2025
Merged

test_runner: recalculate run duration on watch restart#57786
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
pmarchini:test_runner/fix/test-runner-watch-duration-update

Conversation

@pmarchini

Copy link
Copy Markdown
Member

Re-running a test using the --watch flag keeps the duration unchanged across the various reruns.
I've drafted a trivial implementation.

The test I introduced is flaky by design; I need to consider a better approach even though the number of digits is high and the likelihood of getting the same number in two consecutive runs is really low.

image

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 7, 2025
Comment threadlib/internal/test_runner/runner.js Outdated
@pmarchini
pmarchini marked this pull request as ready for review April 14, 2025 14:54
@codecov

codecovBot commented Apr 14, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (08a7df8) to head (121e360).
⚠️ Report is 1167 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/test_runner/runner.js0.00%4 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57786 +/- ##
==========================================
+ Coverage 90.22% 90.24% +0.02% 
==========================================
Files 630 630 Lines 185233 185678 +445 Branches 36325 36391 +66 ==========================================
+ Hits 167122 167567 +445 + Misses 11020 10996 -24 - Partials 7091 7115 +24 
Files with missing linesCoverage Δ
lib/internal/test_runner/test.js97.14% <100.00%> (-0.18%)⬇️
lib/internal/test_runner/runner.js88.83% <0.00%> (-0.80%)⬇️

... and 59 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pmarchinipmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2025
@pmarchinipmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pmarchinipmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 16, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/57786
✔ Done loading data for nodejs/node/pull/57786
----------------------------------- PR info ------------------------------------
Title test_runner: recalculate run duration on watch restart (#57786)
Author Pietro Marchini <pietro.marchini94@gmail.com> (@pmarchini)
Branch pmarchini:test_runner/fix/test-runner-watch-duration-update -> nodejs:main
Labels author ready, needs-ci, test_runner
Commits 4
- test_runner: recalculate run duration on watch restart
- test_runner: rename method to clearExecutionTime for clarity
- fixup! test_runner: rename method to clearExecutionTime for clarity
- test: lint tests
Committers 1
- Pietro Marchini <pietro.marchini94@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
--------------------------------------------------------------------------------
ℹ This PR was created on Mon, 07 Apr 2025 20:48:01 GMT
✔ Approvals: 2
✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/57786#pullrequestreview-2767394183
✔ - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/57786#pullrequestreview-2769240500
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2025-04-16T13:07:46Z: https://ci.nodejs.org/job/node-test-pull-request/66304/
- Querying data for job/node-test-pull-request/66304/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
* branch main -> FETCH_HEAD
✔ origin/main is now up-to-date
- Downloading patch for 57786
From https://github.com/nodejs/node
* branch refs/pull/57786/merge -> FETCH_HEAD
✔ Fetched commits as 5077ea41491d..121e36088b32
--------------------------------------------------------------------------------
Auto-merging lib/internal/test_runner/runner.js
Auto-merging lib/internal/test_runner/test.js
[main 6e8c7c9d5f] test_runner: recalculate run duration on watch restart
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 7 22:45:29 2025 +0200
3 files changed, 25 insertions(+), 1 deletion(-)
Auto-merging lib/internal/test_runner/runner.js
Auto-merging lib/internal/test_runner/test.js
[main 178cabf5a1] test_runner: rename method to clearExecutionTime for clarity
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 14 12:16:44 2025 +0200
2 files changed, 2 insertions(+), 2 deletions(-)
Auto-merging lib/internal/test_runner/runner.js
[main 04b2484f55] fixup! test_runner: rename method to clearExecutionTime for clarity
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 14 12:19:13 2025 +0200
1 file changed, 1 insertion(+), 1 deletion(-)
[main 2dea2d5d54] test: lint tests
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 14 17:03:56 2025 +0200
1 file changed, 4 insertions(+), 4 deletions(-)
✔ Patches applied
There are 4 commits in the PR. Attempting autorebase.
Rebasing (2/7)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: recalculate run duration on watch restart

PR-URL: #57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

[detached HEAD 58bcd84338] test_runner: recalculate run duration on watch restart
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 7 22:45:29 2025 +0200
3 files changed, 25 insertions(+), 1 deletion(-)
Rebasing (3/7)
Rebasing (4/7)
Rebasing (5/7)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: rename method to clearExecutionTime for clarity

PR-URL: #57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

[detached HEAD cc36f9885b] test_runner: rename method to clearExecutionTime for clarity
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 14 12:16:44 2025 +0200
2 files changed, 3 insertions(+), 3 deletions(-)
Rebasing (6/7)
Rebasing (7/7)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: lint tests

PR-URL: #57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

[detached HEAD 1044779fd7] test: lint tests
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Mon Apr 14 17:03:56 2025 +0200
1 file changed, 4 insertions(+), 4 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/14494776822

@pmarchinipmarchini added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 16, 2025
@pmarchinipmarchini added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 16, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 8e4e4df into nodejs:mainApr 16, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 8e4e4df

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57786
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@richardlaurichardlau added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Sep 19, 2025
@richardlau

Copy link
Copy Markdown
Member

This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@pmarchini@nodejs-github-bot@richardlau@benjamingr@cjihrig@marco-ippolito@MoLow@jakecastelli