Skip to content

benchmark: add test-only and mock timers cases - #64097

Open
luanmuniz wants to merge 4 commits into
nodejs:mainfrom
luanmuniz:benchmark-test-runner-only-mock-timers
Open

benchmark: add test-only and mock timers cases#64097
luanmuniz wants to merge 4 commits into
nodejs:mainfrom
luanmuniz:benchmark-test-runner-only-mock-timers

Conversation

@luanmuniz

@luanmuniz luanmuniz commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Adds another small set of node:test benchmarks for areas listed in #55723.
A continuation of the work I started in #63754
This is a different PR because it brings a different set of attention points, and to keep both PR easy to review.

This PR covers:

  • only mode with --test-only
  • mock.timers.enable()
  • mocked setTimeout
  • mocked setInterval
  • mocked setImmediate
  • mocked scheduler.wait
  • mocked AbortSignal.timeout
  • mocked Date.now()
  • mock.timers.setTime()
  • mock.timers.runAll()

Notes

  1. enable-* test the enable() + reset() instead of just enable. This happens because an error is thrown if enable is called multiple times without reset.
  2. The scheduler.wait mode includes the final Promise.all() inside the measured section. My evaluation was that it was more appropriate to do so than not, since the scheduled waits are promise-based, but I am happy to adjust the structure if reviewers prefer a narrower measure.

Issue found while writing this

TLDR: I found an issue (that doesn't affect this PR) with using setImmediate and runAll together while writing the benchmark; guidance welcome;

While writing the benchmark, I found that setImmediate doesn't work with runAll(). The mocked setImmediate is scheduled with a special -1 time so it runs before zero-delay timers. runAll() computes longestTimer.runAt - this.#now in mock_timers.js:816, which becomes -1, and tick() rejects negative time in mock_timers.js:556.

I would love to open a new PR to fix this issue, but this is beyond my context of how the team working on this operates and what they think would be an appropriate fix. If the person reviewing the PR could provide some guidance on the best way to handle this issue, it would be much appreciated.

This issue doesn't impact this PR, all benchmarks still work with the current code (With tick(0) instead of runAll() for setImmediate).

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. test_runner Issues and PRs related to the test runner subsystem. labels Jun 23, 2026
Comment thread benchmark/test_runner/test-only.js Outdated

for (let i = 0; i < selected; i++) {
test(`selected-${i}`, { only: true }, () => {
avoidV8Optimization = i;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unnecessary; it's a clojure. Adding an assert.ok is probably more useful.

@luanmuniz luanmuniz Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @RafaelGSS, thanks for taking a look.

Just to make sure I understand the suggestion: are you suggesting replacing the avoidV8Optimization assignment with an assertion inside the benchmarked section?

I added the avoidV8Optimization bit to avoid making the benchmarked callback completely empty, as detailed in the previous PR: #63754 (comment)

My concern with adding assert.ok() inside the callback is that the assertion itself would become part of what is being measured. Adding extra work inside the benchmark was also something @avivkeller raised in the previous PR: #63754 (comment)

I’m happy to adjust it either way. Please let me know how to proceed.

Thank you for the guidance.

Obs: I see the comments about calbrate-n on the first PR and i will push that change very soon here too

@luanmuniz luanmuniz Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@RafaelGSS For consistency with the other PR, i've removed the avoidV8Optimization here too. If the current implementation is acceptable, could the thread be resolved and CI started?

@luanmuniz
luanmuniz force-pushed the benchmark-test-runner-only-mock-timers branch from 723caba to 8e49e7c Compare June 25, 2026 11:53
@luanmuniz

Copy link
Copy Markdown
Contributor Author

Hi there, sorry for the force-push. Like the other branch, I had a local git history issue while cleaning up the branch, and I force-pushed to get the PR back into the intended state. Apologies for the noise. I’ll be more careful with the branch history going forward.

I've used calibrate-n, and there are the results in case you want to take a look:
calibrate-mock-timers.txt
calibrate-test-only.txt

The numbers are already adjusted.

cc @RafaelGSS

@luanmuniz
luanmuniz requested a review from RafaelGSS June 25, 2026 11:58
@luanmuniz
luanmuniz force-pushed the benchmark-test-runner-only-mock-timers branch from 8e49e7c to e5623a8 Compare June 26, 2026 21:59
@luanmuniz

luanmuniz commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Ping @RafaelGSS
Friendly ping on this one.

I believe the review feedback has been addressed, but please let me know if there is anything else you’d like me to change before this can move forward.

Happy to adjust further if needed.

@luanmuniz

Copy link
Copy Markdown
Contributor Author

@RafaelGSS Sorry to keep pinging, im waiting the existing PR's to be closed to continue working on the other benchmarks, i don't want to have more PR's open at the same time. Let me know if there is anything i can do to help you move this and the other tickets i have open. Thank you!

@luanmuniz

Copy link
Copy Markdown
Contributor Author

@RafaelGSS @avivkeller @nodejs/benchmarking could someone help resolve the remaining avoidV8Optimization thread?

This now has an approval from @avivkeller after the latest commit. My remaining uncertainty is whether replacing the assignment with assert.ok() is considered required, since that would add assertion work to the measured callback.

If the current implementation is acceptable, could the thread be resolved and CI started? Otherwise, please let me know the specific implementation you would prefer.

The only benchmark covers creating selected and non-selected
tests when running with --test-only.

The mock timers benchmark covers enabling timer mocks, setTimeout,
setInterval, setImmediate, scheduler.wait, AbortSignal.timeout,
mocked Date.now(), setTime(), and runAll().

Refs: nodejs#55723
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
Use calibrated n values for the test runner mock-timers and
test-only benchmarks.

Reduce the test options benchmark configurations to keep the
benchmark suite smaller.

Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
@luanmuniz
luanmuniz force-pushed the benchmark-test-runner-only-mock-timers branch from e5623a8 to 8369ca2 Compare July 18, 2026 19:08
@luanmuniz luanmuniz changed the title Add benchmarks for node only mode and mock timers. benchmark: add test-only and mock timers cases Jul 18, 2026
@luanmuniz

luanmuniz commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the commit-message lint failure by adding the benchmark subsystem and removing the trailing punctuation. The updated commit now passes core-validate-commit locally.

Could someone please reapprove the updated commit and trigger CI again?
Thank you!

Due to lack of evidence and review requirement
the avoidV8Optimization variable was removed
and left the callbacks as no-ops.

Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 17, 2026
@nodejs-github-bot nodejs-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 Aug 17, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/64097
✔  Done loading data for nodejs/node/pull/64097
----------------------------------- PR info ------------------------------------
Title      benchmark: add test-only and mock timers cases (#64097)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     luanmuniz:benchmark-test-runner-only-mock-timers -> nodejs:main
Labels     benchmark, author ready, commit-queue, commit-queue-squash, test_runner
Commits    3
 - benchmark: add test-only and mock timers cases
 - benchmark: calibrate test runner benchmarks
 - benchmark: remove optimization guard
Committers 1
 - Luan Muniz <luan@luanmuniz.com.br>
PR-URL: https://github.com/nodejs/node/pull/64097
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64097
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 23 Jun 2026 17:50:53 GMT
   ✔  Approvals: 2
   ✔  - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/64097#pullrequestreview-4673879801
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/64097#pullrequestreview-4954346663
   ✘  GitHub CI is still running
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/32067715646

@RafaelGSS

Copy link
Copy Markdown
Member

can you rebase so github ci is triggered again?

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.94%. Comparing base (cf30b2e) to head (9dfd597).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64097      +/-   ##
==========================================
+ Coverage   90.11%   91.94%   +1.82%     
==========================================
  Files         752      400     -352     
  Lines      251861   178855   -73006     
  Branches    47365    27313   -20052     
==========================================
- Hits       226955   164440   -62515     
+ Misses      16238    14087    -2151     
+ Partials     8668      328    -8340     

see 485 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.

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. benchmark Issues and PRs related to the benchmark subsystem. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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