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

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

test/fixtures/test-runner/output/test-timeout-flag.js

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`.

test/fixtures/test-runner/output/test-timeout-flag.js

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.

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

mcollina

lgtm

test/parallel/test-runner-output.mjs

cjihrig

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

lib/internal/test_runner/utils.js

test/fixtures/test-runner/output/test-timeout-flag.js

test/parallel/test-runner-cli-timeout.js

lib/internal/test_runner/harness.js

lib/internal/test_runner/test.js

…Suite, unref timeout

test/fixtures/test-runner/output/test-timeout-flag.js

lib/internal/test_runner/test.js

cjihrig

LGTM. Thanks for getting this done!

nodejs-github-bot · 2025-04-09T04:49:43Z

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

nodejs-github-bot · 2025-04-09T08:01:33Z

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

pmarchini · 2025-04-09T09:11:34Z

Hey @jakecastelli, it seems there's a failure in the CI: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/33540/ ...It's the first time I've seen that specific test be "flaky"

nodejs-github-bot · 2025-04-09T09:12:13Z

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

jakecastelli · 2025-04-09T12:05:33Z

hmm not too sure, I will keep an eye on the reliability report going forward for that test

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: nodejs#57672 Fixes: nodejs#57656 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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.

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

test/fixtures/test-runner/output/test-timeout-flag.js Outdated Show resolved Hide resolved

aduh95 reviewed Mar 29, 2025

View reviewed changes

test/fixtures/test-runner/output/test-timeout-flag.js Outdated Show resolved Hide resolved

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

test/fixtures/test-runner/output/test-timeout-flag.js Outdated Show resolved Hide resolved

fixup! reduce test timeout and improve test desc

07863c5

pmarchini reviewed Mar 29, 2025

View reviewed changes

test/fixtures/test-runner/output/test-timeout-flag.snapshot Show resolved Hide resolved

mcollina approved these changes Mar 30, 2025

View reviewed changes

pmarchini reviewed Mar 30, 2025

View reviewed changes

test/parallel/test-runner-output.mjs Show resolved Hide resolved

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

lib/internal/test_runner/test.js Show resolved Hide resolved

jakecastelli commented Apr 4, 2025

View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved

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 force-pushed the test-timeout-improvement branch from 215f820 to 04f280b Compare April 8, 2025 17:15

cjihrig reviewed Apr 8, 2025

View reviewed changes

test/fixtures/test-runner/output/test-timeout-flag.js Outdated Show resolved Hide resolved

lib/internal/test_runner/test.js Show resolved Hide resolved

jakecastelli added 2 commits April 9, 2025 11:55

fixup! incorrect logic for cli timeout to take precedent

ab0e86a

fixup! linting error

44c7757

cjihrig approved these changes Apr 9, 2025

View reviewed changes

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025

jakecastelli added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 9, 2025

pmarchini approved these changes Apr 9, 2025

View reviewed changes

pmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025

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

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

Uh oh!

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

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

Uh oh!

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!

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

pmarchini commented Apr 9, 2025

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

jakecastelli commented Apr 9, 2025

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