Skip to content

tools: wait for test processes instead of polling - #66432

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:test-runner-blocking-wait
Open

mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:test-runner-blocking-wait

Conversation

@mcollina

@mcollina mcollina commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

RunProcess polled the child with poll() and an exponential backoff sleep capped at 100 ms, so every test longer than roughly 300 ms paid 30-75 ms of latency after it had already exited. Measured directly against a blocking wait on a setTimeout child:

child runtime old Execute blocking wait added latency
300 ms 414 ms 380 ms 34 ms
1000 ms 1107 ms 1032 ms 75 ms
3000 ms 3115 ms 3088 ms 27 ms

This replaces the loop with a blocking wait(); a threading.Timer delivers the kill (SIGTERM, or SIGABRT with --abort-on-timeout) if the timeout is crossed, and wait() then returns the post-kill exit code exactly as before. The timer is cancelled as soon as the process exits.

Because workers now observe a child's exit with no delay, a child killed by the same ctrl-c that interrupts the runner could be recorded as a crashed test before the main thread set the shutdown flag, turning "Test aborted." into a "Failed tests:" list. The flag is now set from the SIGINT handler itself so it wins that race.

Parallel suite on an idle 8-core Linux box, -j8, no failures in either run:

wall sum of test durations median test
before 2m56s 1338 s 137 ms
after 2m45s 1258 s 119 ms

Verified: timeout and --abort-on-timeout paths (exit_code -15 / -6, timed_out set), ctrl-c three times in a row aborts immediately with no leftover node processes, pseudo-tty/message/parallel samples pass, make lint-py clean.


AI generated, reviewed by me.

RunProcess polled the child with an exponential backoff that capped at
100 ms, so every test longer than roughly 300 ms paid 30-75 ms of
latency after it had already exited. Across the parallel suite that
adds up to about 160 thread-seconds.

Block in wait() instead and let a timer thread deliver the kill when
the timeout is crossed. Set the shutdown flag from the SIGINT handler
so a worker whose child died from the same ctrl-c does not report it
as a failure before the main thread aborts the run.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UH48w8TCtYmHd4P2UE6HEY
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory. labels Oct 1, 2026
@mcollina
mcollina requested a review from panva October 1, 2026 10:45
@panva panva added 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 Oct 1, 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 Oct 1, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.37%. Comparing base (8bf7793) to head (5c803f0).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66432      +/-   ##
==========================================
- Coverage   90.39%   90.37%   -0.02%     
==========================================
  Files         792      792              
  Lines      275580   275697     +117     
  Branches    52840    52856      +16     
==========================================
+ Hits       249104   249169      +65     
- Misses      16897    16937      +40     
- Partials     9579     9591      +12     

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

@panva panva added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Oct 1, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Oct 1, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. test Issues and PRs related to Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants