test_runner: add timeout support to test plan by pmarchini · Pull Request #56765 · nodejs/node

pmarchini · 2025-01-26T00:44:49Z

This PR attempts to address #56758.

One major concern about this feature: to allow the timeout to interrupt the tests the --test-force-exit flag is required.

I'm opening this PR as a draft to validate the implementation and behaviour.
Documentation still needs to be updated!

nodejs-github-bot · 2025-01-26T00:44:54Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/test.js

codecov · 2025-01-27T19:23:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.34%. Comparing base (79f96b6) to head (90e566f).
Report is 73 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56765      +/-   ##
==========================================
+ Coverage   89.10%   90.34%   +1.24%     
==========================================
  Files         665      629      -36     
  Lines      193203   184445    -8758     
  Branches    37220    36038    -1182     
==========================================
- Hits       172158   166646    -5512     
+ Misses      13771    10918    -2853     
+ Partials     7274     6881     -393

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	`97.31% <100.00%> (+0.22%)`	⬆️

... and 114 files with indirect coverage changes

cjihrig

This is looking better.

Probably worth cc'ing @jsumners here as well.

doc/api/test.md

cjihrig · 2025-01-28T14:01:19Z

test/fixtures/test-runner/output/test-runner-plan.js

+const { test, suite } = require('node:test');
 const { Readable } = require('node:stream');

+suite('input validation', () => {


I would actually add all of these new tests to a separate "regular" test file. I don't think we necessarily need to check snapshot output for these. Snapshot tests are more tedious to maintain over time (they need regenerated and introduce merge conflicts). Plus, the linter doesn't run on fixture files.

As soon as possible I'll update these tests

lib/internal/test_runner/test.js

jsumners · 2025-01-28T17:37:42Z

This is looking good to me. I'm disappointed that we can't default a timeout (I believe there will be confusion about why, likely broken, tests never end), but this is still a win.

mcollina

lgtm

nodejs-github-bot · 2025-02-09T16:44:12Z

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

atlowChemi · 2025-02-09T17:19:34Z

doc/api/test.md

+    * If a number, it specifies the maximum wait time in milliseconds
+      before timing out while waiting for expected assertions and subtests to be matched.
+      If the timeout is reached, the test will fail.


Should the maximal value be pointed out here?

Are you suggesting that there should be a maximum value possible for the wait?

@jsumners, yes, at the moment the maximum is const { TIMEOUT_MAX } = require('internal/timers');, here a partial reference #11198

I don't see a need for it. If someone wants to set the timeout to Number.MAX_VALUE, then that's on them. I think most people will be reasonable and set it on the order of seconds.

IMHO, validating the maximum value is a good way to inform the user of misuse.
I honestly would avoid reporting this maximum in the documentation, as has been done in this case https://github.com/nodejs/node/pull/56595/files .

@atlowChemi @jsumners If you are okay with this, I would avoid documenting this implementation detail!

I am good with the PR as it is.

I'll just address the last comment related to tests and restart the CIs

@pmarchini I left this comment while approving the PR - so this is non-blocking IMHO 🙂

mcollina

lgtm

cjihrig · 2025-02-23T15:08:48Z

I think it would be better to do this in a separate PR

I agree with this. Get this landed and then refactor.

nodejs-github-bot · 2025-02-23T20:31:16Z

Landed in 8c2df73

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: nodejs#56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

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 Jan 26, 2025

pmarchini requested a review from cjihrig January 26, 2025 00:45

cjihrig reviewed Jan 26, 2025

View reviewed changes

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

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

pmarchini commented Jan 26, 2025

View reviewed changes

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

pmarchini commented Jan 26, 2025

View reviewed changes

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

pmarchini commented Jan 26, 2025

View reviewed changes

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

pmarchini requested a review from cjihrig January 27, 2025 17:47

pmarchini marked this pull request as ready for review January 27, 2025 17:48

pmarchini commented Jan 27, 2025

View reviewed changes

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

pmarchini requested review from MoLow, atlowChemi and jakecastelli January 27, 2025 19:40

cjihrig reviewed Jan 28, 2025

View reviewed changes

pmarchini force-pushed the issue/56758 branch from d9a7124 to 08951bd Compare February 2, 2025 11:15

mcollina approved these changes Feb 2, 2025

View reviewed changes

cjihrig approved these changes Feb 2, 2025

View reviewed changes

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

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

atlowChemi approved these changes Feb 9, 2025

View reviewed changes

pmarchini added 6 commits February 14, 2025 10:03

test_runner: refactor testPlan counter increse

e9a7a7f

test_runner: add timeout support to test plan

9735384

test_runner: add input validation tests and refactor planning with wait

c942eb9

test_runner: refactor TestPlan

927ffdb

doc: add plan wait option to test runner documentation

0665597

test_runner: remove poller

295bd0f

mcollina approved these changes Feb 23, 2025

View reviewed changes

atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2025

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

nodejs-github-bot merged commit 8c2df73 into nodejs:main Feb 23, 2025
69 checks passed

targos pushed a commit that referenced this pull request Feb 25, 2025

test_runner: refactor testPlan counter increse

2abffd4

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

github-actions bot mentioned this pull request Feb 25, 2025

2025-02-26, Version 23.9.0 (Current) #57207

Merged

targos pushed a commit that referenced this pull request Feb 25, 2025

test_runner: refactor testPlan counter increse

116c1fe

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

pmarchini mentioned this pull request Mar 3, 2025

test: refactor test runner run plan tests #57304

Merged

cjihrig mentioned this pull request Mar 8, 2025

node:test should be able to detect a plan has not been met #56758

Closed

RafaelGSS pushed a commit that referenced this pull request Apr 1, 2025

test_runner: refactor testPlan counter increse

03c570c

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 1, 2025

test_runner: refactor testPlan counter increse

a53b9c0

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

aduh95 pushed a commit that referenced this pull request Apr 2, 2025

test_runner: refactor testPlan counter increse

3047608

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS mentioned this pull request Apr 8, 2025

v22.x-staging cherry-pick status #57799

Closed

aduh95 mentioned this pull request Apr 11, 2025

2025-04-23, Version 22.15.0 'Jod' (LTS), @UlisesGascon prepared by @RafaelGSS #57840

Merged

RafaelGSS pushed a commit that referenced this pull request Apr 14, 2025

test_runner: refactor testPlan counter increse

d162975

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

aduh95 pushed a commit that referenced this pull request Apr 14, 2025

test_runner: refactor testPlan counter increse

8cae6ad

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

aduh95 pushed a commit that referenced this pull request Apr 14, 2025

test_runner: refactor testPlan counter increse

c7b6581

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

aduh95 pushed a commit that referenced this pull request Apr 15, 2025

test_runner: refactor testPlan counter increse

c907b7c

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025

test_runner: refactor testPlan counter increse

4c94f31

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025

test_runner: refactor testPlan counter increse

1b33b97

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request May 1, 2025

test_runner: refactor testPlan counter increse

5aa7ef1

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request May 2, 2025

test_runner: refactor testPlan counter increse

2a100a1

PR-URL: #56765 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

LiviaMedeiros mentioned this pull request Jun 23, 2025

test_runner: automatically wait for subtests to finish #58800

Closed

Uh oh!

Conversation

pmarchini commented Jan 26, 2025

Uh oh!

nodejs-github-bot commented Jan 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsumners commented Jan 28, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 23, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jan 27, 2025 •

edited

Loading