Skip to content

test: fix flaky test-bench-stream - #65874

Open
mcollina wants to merge 3 commits into
nodejs:mainfrom
mcollina:fix-bench-stream-timeout-flake
Open

test: fix flaky test-bench-stream#65874
mcollina wants to merge 3 commits into
nodejs:mainfrom
mcollina:fix-bench-stream-timeout-flake

Conversation

@mcollina

@mcollina mcollina commented Sep 7, 2026

Copy link
Copy Markdown
Member

testDeliveryDoesNotConsumeTimeout gave the benchmark a 20ms timeout while stalling the stream consumer for 50ms. The harness only credits delivery time back to the deadline (pause() in lib/internal/bench_runner/harness.js), so the 32 samples the benchmark runs still have to fit inside the 20ms budget. That work costs 1-2ms on an idle machine, so the margin is only ~10x and a loaded CI runner blows through it.

Seen on macOS in https://github.com/nodejs/node/actions/runs/34090282687/job/101642182203 (unrelated PR):

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ Error [ERR_OPERATION_FAILED]: Operation failed: Benchmark timed out after 20ms
    at checkDeadline (node:internal/bench_runner/harness:900:30)
    at runAfterEach (node:internal/bench_runner/harness:917:7)

This scales the timeout and the stall together: the benchmark's own work now has 500ms of headroom, while the consumer still stalls for twice the timeout, so the test keeps failing if delivery time is ever charged against the benchmark.

Measured locally with 64 competing spinners, 100 runs each:

timeout pass fail
20ms (before) 95 5
500ms (after) 100 0

Verified the test still catches the regression it was written for by making pause() stop extending the deadline — it fails with Benchmark timed out after 500ms.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Sep 7, 2026
@mcollina
mcollina requested a review from jasnell September 7, 2026 08:53
@mcollina
mcollina force-pushed the fix-bench-stream-timeout-flake branch from de7d9e4 to 6ace0d8 Compare September 7, 2026 08:53
@mcollina
test: fix flaky test-bench-stream
a603d7f 
testDeliveryDoesNotConsumeTimeout gave the benchmark a 20ms timeout
while stalling the consumer for 50ms. Only delivery time is credited
back to the deadline, so the 32 samples still had to run within 20ms,
which is about a 10x margin on an idle machine and not enough on a
loaded CI runner.

Scale the timeout and the stall together so the benchmark's own work
gets 500ms of headroom while the consumer still stalls for longer than
the timeout.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
mcollina force-pushed the fix-bench-stream-timeout-flake branch from 6ace0d8 to a603d7f Compare September 7, 2026 09:13
@mcollina mcollina added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @mcollina. Please 👍 to approve.

@panva panva added flaky-test Issues and PRs involving tests that fail intermittently in CI. author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 7, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 7, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
aduh95
aduh95 reviewed Sep 7, 2026
View reviewed changes
Comment thread test/parallel/test-bench-stream.js Outdated
@codecov

codecov Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (6f38736) to head (0142cae).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #65874      +/-   ##
==========================================
- Coverage   92.40%   90.19%   -2.21%     
==========================================
  Files         417      771     +354     
  Lines      188648   264911   +76263     
  Branches    28849    50316   +21467     
==========================================
+ Hits       174320   238938   +64618     
- Misses      13995    16926    +2931     
- Partials      333     9047    +8714

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.

@mcollina @aduh95
Update test-bench-stream.js
bbf8d8d 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@mcollina mcollina added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Sep 7, 2026
@panva panva added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 7, 2026
aduh95
aduh95 reviewed Sep 7, 2026
View reviewed changes
Comment thread test/parallel/test-bench-stream.js Outdated
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 7, 2026
@nodejs-github-bot

nodejs-github-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/77147/

@aduh95 aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@ShogunPanda ShogunPanda ShogunPanda approved these changes
@panva panva panva approved these changes
@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fast-track PRs proposed for a shorter-than-standard waiting period before landing. flaky-test Issues and PRs involving tests that fail intermittently in CI. needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mcollina @nodejs-github-bot @ShogunPanda @panva @jasnell @aduh95