test_runner: improve --test-timeout to be per test by jakecastelli · Pull Request #57672 · nodejs/node

jakecastelli · 2025-03-29T11:32:54Z

Previously --test-timeout is set on per test execution, this is obviously a bug as per test execution is hard to be expected, this patch addresses the issue by setting timeout from per execution to per test.

This patch also fixes a minor issue that --test-timeout is not being respected when running without --test.

Fixes: #57656

nodejs-github-bot · 2025-03-29T11:33:00Z

Review requested:

@nodejs/test_runner

Previously `--test-timeout` is set on per test execution, this is obviously a bug as per test execution is hard to be expected, this patch addresses the issue by setting `timeout` from per execution to per test. This patch also fixes a minor issue that `--test-timeout` is not being respected when running without `--test`.

codecov · 2025-03-29T14:19:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (1123585) to head (44c7757).
⚠️ Report is 1249 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #57672    +/-   ##
========================================
  Coverage   90.22%   90.23%            
========================================
  Files         630      630            
  Lines      185055   185296   +241     
  Branches    36216    36342   +126     
========================================
+ Hits       166975   167204   +229     
+ Misses      11042    11011    -31     
- Partials     7038     7081    +43

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	`89.15% <100.00%> (-0.48%)`	⬇️
lib/internal/test_runner/test.js	`97.33% <100.00%> (+0.01%)`	⬆️
lib/internal/test_runner/utils.js	`58.52% <100.00%> (-0.14%)`	⬇️

... and 82 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

lgtm

cjihrig

A few nits, but this is looking a lot better than the current implementation. Thanks!

…Suite, unref timeout

nodejs-github-bot · 2025-04-09T14:36:26Z

Landed in 67786c1

brianwestphal · 2025-04-09T21:50:48Z

Thank you!

Previously `--test-timeout` is set on per test execution, this is obviously a bug as per test execution is hard to be expected, this patch addresses the issue by setting `timeout` from per execution to per test. This patch also fixes a minor issue that `--test-timeout` is not being respected when running without `--test`. PR-URL: #57672 Fixes: #57656 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

richardlau · 2025-09-20T02:19:34Z

This seemed to cause test failures when cherry-picked to v22.x-staging. It's possible that the tests are relying on test runner features that cannot or have yet to be backported to v22.x-staging.

…child Two fixes for the same failure mode -- an unbounded wait turning a failure into a hang. `npm test` runs unattended inside release.sh, so a hang wedges the release rather than aborting it. 1. The concrete defect. The credential-race test forks children and waits on `child.once("message")` for a "ready" signal, with nothing on the failure path. A child that dies BEFORE signalling -- an import throw, a stale build -- never sends it, so the promise stays pending forever and the whole suite hangs. It now also settles on `exit`, reporting the child's own exit code. A late exit after res() is a no-op on an already-settled promise, which is the normal path here: these children are expected to exit once the race has been signalled. 2. The backstop. node:test has no default per-test timeout, so any future unbounded wait has the same effect. --test-timeout=300000 is deliberately generous -- measured files run ~7.5s worst case, so 5 minutes is ~40x headroom and cannot false-fail, while still converting a hang into a reported failure. Note the semantics: until Node 24 the flag is per-FILE, not per-test (nodejs/node#57672), so the value must clear the slowest FILE. Requires Node >= 20.11.0 (nodejs/node#50443); dev-side script only, so engines is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… a release node:test has NO default per-test timeout, so a test awaiting an event that never arrives runs forever. `npm test` runs unattended inside release.sh, so that turns a wedged release rather than a failed one -- the release just never returns. --test-timeout=300000 is a backstop, deliberately generous: measured test files run ~7.5s worst case here, so 5 minutes is roughly 40x headroom and cannot false-fail, while still converting an infinite hang into a reported failure. Note the semantics: until Node 24 the flag is per-FILE, not per-test (nodejs/node#57672), so the value has to clear the slowest FILE. Verified empirically rather than assumed -- and note that Node already catches the easy case (a pending promise with a drained event loop) on its own; the case this flag actually covers is a hang holding a live handle, e.g. a child process that never messages back. Requires Node >= 20.11.0 (nodejs/node#50443). This affects the dev-side test script only, not package consumers, so engines is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`tasks-cli.test.ts` starts the Hardhat CLI 37 times across its 33 tests. No workflow runs the live scripts; they run locally. The file budgets go when the Node floor moves to a release that has nodejs/node#57672, not when any Node 22 release gets it. Two fixture comments and two test names say what they check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Closes #267 ## What changes On Node 22, `--test-timeout` limits each test file and sets no limit per test. On Node 24 and later it limits each test and not the file ([nodejs/node#57672](nodejs/node#57672), first released in 24.0.0). The package scripts passed 30 s and 120 s as per-test limits, so on Node 22 the runner ended a file that ran longer, even when every test in it passed. One correction to the issue text: on Node 22 the flag does not "also" limit the file. It limits only the file, and no test gets a limit from it. - `scripts/node-test.ts` starts `node --test` with the `--test-timeout` value for the running Node major: the per-test limit on Node 24 and later, a file budget on Node 22. It exits with the runner's code and passes SIGINT and SIGTERM on to it. - `test:unit` and `test:integration` in the four packages call it. | Script | Node 24 and later: limit per test | Node 22: budget per file | | ------------------ | --------------------------------- | ------------------------ | | `test:unit` | 30 s | 300 s | | `test:integration` | 120 s | 600 s | - `test/scripts/node-test.test.ts` (added to the root `test` script) runs fixture files with limits of a few seconds: - a file of five passing 500 ms tests, with a 2 s per-test limit, passes on every version; - a slow test is cancelled at the per-test limit on Node 24 and later, and on Node 22 its file is cancelled at the budget; - the script exits 0 on a pass, 1 on a failing test and 2 on an unknown kind of test, and after SIGTERM it ends with the runner's code within 15 s. - `docs/contributor/testing.md` has a new section, "Test time limits": the two meanings, the budgets, the Node pull request, and what to delete when the Node floor moves to 24. `docs/contributor/tooling.md` names the script. - The comments in `packages/hardhat-kms/test/helpers/hardhat-cli.ts` and `tutorial-return-funds.test.ts` match the new numbers. On Node 22 a hung test is now reported as a cancelled file after up to 10 minutes, not as a failed test. The same tests run on Node 24 and 26, where the runner cancels the hung test by name at its limit. Not in scope: the localstack, examples, live and coverage scripts and `scripts/test-hardhat-versions.ts` (CI runs the localstack, examples, coverage and Hardhat-versions jobs on Node 24; the live scripts run locally), and splitting test files. ## Test plan Ran locally on macOS: - `node --test test/scripts/node-test.test.ts` on Node 22.13.0 (with `NODE_OPTIONS='--import tsx'`), 22.23.3, 24.16.0 and 26.0.0, and on 26.0.0 with the `scripts/fail-on-deprecation.mjs` preload: 10 of 10 pass on each. - Four temporary edits to `scripts/node-test.ts`, each reverted, to check that the test fails: - the per-test limit on every version: fails on Node 22.13.0 (the passing file is cancelled); - `>` in place of `>=` in the version rule: fails on Node 24.16.0 (the slow test is no longer cancelled at 1 s); - no exit-code forwarding: fails on Node 24.16.0; - no signal forwarding: fails on Node 22.13.0 and 24.16.0. - `pnpm --filter @hardhat-kms/gcp run test:unit` through the script on Node 22.13.0 (`--import tsx`) and 24.16.0: 93 of 93 pass on each. - `pnpm run test:unit` for the four packages on Node 24.16.0 (the pre-push hook): passes. - `pnpm run check`, `pnpm run docs:check` and `knip` on Node 24.16.0: pass. Not run locally: - The full `pnpm test`, and `test:integration` through the script on any version. The three CI legs (Node 22.13.0, 24.0.0, 26.0.0) run both. - macOS and Windows CI (`ci-all-os.yml`). The script has not run on Windows. - A hang probe in a real integration file (a test that never settles, to see the 600 s file budget end the file on Node 22). The slow-test fixture covers the same path with a 3 s budget. - Ctrl-C in a terminal. SIGTERM forwarding has a test; SIGINT uses the same handler and has none. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

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 29, 2025

jakecastelli force-pushed the test-timeout-improvement branch from fba5496 to a982c95 Compare March 29, 2025 11:34

aduh95 reviewed Mar 29, 2025

View reviewed changes

Comment thread test/fixtures/test-runner/output/test-timeout-flag.js Outdated

aduh95 reviewed Mar 29, 2025

View reviewed changes

Comment thread test/fixtures/test-runner/output/test-timeout-flag.js Outdated

jakecastelli force-pushed the test-timeout-improvement branch from b79d0c3 to 496c282 Compare March 29, 2025 11:41

jakecastelli force-pushed the test-timeout-improvement branch from 496c282 to c1c48ec Compare March 29, 2025 11:51

fixup! change timeout to 20ms and format the file

c1f5562

aduh95 reviewed Mar 29, 2025

View reviewed changes

Comment thread test/fixtures/test-runner/output/test-timeout-flag.js Outdated

fixup! reduce test timeout and improve test desc

07863c5

pmarchini reviewed Mar 29, 2025

View reviewed changes

Comment thread test/fixtures/test-runner/output/test-timeout-flag.snapshot

mcollina approved these changes Mar 30, 2025

View reviewed changes

pmarchini reviewed Mar 30, 2025

View reviewed changes

Comment thread test/parallel/test-runner-output.mjs

pmarchini approved these changes Mar 30, 2025

View reviewed changes

cjihrig reviewed Mar 30, 2025

View reviewed changes

fixup! hoist timeoutPerTest out of if

180116b

jakecastelli force-pushed the test-timeout-improvement branch from 250ef6d to 8a713ec Compare April 4, 2025 15:55

address PR comments

af2201e

jakecastelli force-pushed the test-timeout-improvement branch from 8a713ec to af2201e Compare April 4, 2025 15:56

jakecastelli commented Apr 4, 2025

View reviewed changes

Comment thread lib/internal/test_runner/test.js

jakecastelli commented Apr 4, 2025

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

jakecastelli force-pushed the test-timeout-improvement branch from 1f2245c to f593a7b Compare April 4, 2025 16:27

remove deuplicated if checks and move this.timeout into FileTest and …

6559293

…Suite, unref timeout

jakecastelli force-pushed the test-timeout-improvement branch from f593a7b to 6559293 Compare April 4, 2025 16:35

MoLow approved these changes Apr 7, 2025

View reviewed changes

fixup! fix tests

24a1936

jakecastelli requested a review from cjihrig April 8, 2025 15:26

jakecastelli added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Apr 9, 2025

pmarchini approved these changes Apr 9, 2025

View reviewed changes

pmarchini added the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 9, 2025

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 9, 2025

nodejs-github-bot merged commit 67786c1 into nodejs:main Apr 9, 2025

jakecastelli mentioned this pull request Apr 10, 2025

test_runner,cli: --test-timeout should be used with --test #53773

Closed

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

pmarchini mentioned this pull request May 12, 2025

Revert https://github.com/nodejs/node/pull/56664 #58282

Closed

MoLow mentioned this pull request Sep 11, 2025

test-runner: fix suite timeout #59853

Merged

richardlau added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Sep 20, 2025

owlstronaut mentioned this pull request Oct 17, 2025

fix: uses ci-friendly startsWith instead of fromJSON npm/template-oss#531

Merged

deniak mentioned this pull request May 7, 2026

Convert tests to TypeScript and remove Node v20 support w3c/specberus#2098

Merged

coderabbitai Bot mentioned this pull request Aug 13, 2026

BZE-278: Harden Phase 3A execution workflow Bet-Zero/scoutzero#501

Merged

8 tasks done

Akagilnc mentioned this pull request Sep 14, 2026

ak-roles: fix(#252): bound canonical test entries with fixed --test-timeout Akagilnc/ak-pi-workflow-roles#912

Merged

This was referenced Oct 4, 2026

Tests: on Node 22 --test-timeout limits each whole test file aelmanaa/hardhat-kms#267

Closed

test: choose --test-timeout by Node major (per file on Node 22) aelmanaa/hardhat-kms#271

Merged

Navigation Menu

Repository navigation

test_runner: improve --test-timeout to be per test - #57672

test_runner: improve --test-timeout to be per test#57672
nodejs-github-bot merged 10 commits into
nodejs:mainfrom
jakecastelli:test-timeout-improvement

jakecastelli commented Mar 29, 2025

Uh oh!

nodejs-github-bot commented Mar 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

mcollina left a comment

Uh oh!

Uh oh!

cjihrig left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

brianwestphal commented Apr 9, 2025

Uh oh!

richardlau commented Sep 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Conversation

jakecastelli commented Mar 29, 2025

Uh oh!

nodejs-github-bot commented Mar 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

brianwestphal commented Apr 9, 2025

Uh oh!

richardlau commented Sep 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov Bot commented Mar 29, 2025 •

edited

Loading