test_runner: add timeout support to test plan #56765

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 { 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 🙂

nodejs-github-bot · 2025-02-17T08:43:06Z

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

pmarchini · 2025-02-17T09:21:42Z

There's a related failure in the CI, I'll investigate ASAP

pmarchini · 2025-02-23T10:55:50Z

I should have addressed the CI issue (my bad for a platformTimeout inside the snapshots).

@cjihrig @atlowChemi, I'm refactoring the tests to use the run function instead of snapshots.
I think it would be better to do this in a separate PR, starting with a first commit that includes both the "old" and "new" tests.

This PR is almost stale, and the refactor will also affect tests that are not part of this PR.

WDYT?

nodejs-github-bot · 2025-02-23T12:49:54Z

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

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>

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

pmarchini added 3 commits February 14, 2025 10:03

test_runner: refactor TestPlan constructor

53fcd1e

test_runner: refactor namings

20cfb75

doc: update plan wait option documentation

33993c8

pmarchini force-pushed the issue/56758 branch from 6bed95c to 33993c8 Compare February 14, 2025 09:04

atlowChemi approved these changes Feb 16, 2025

View reviewed changes

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

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

This comment was marked as outdated.

Sign in to view

test: remove platformTimeout

90e566f

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

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

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

Open

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>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: add timeout support to test plan #56765

test_runner: add timeout support to test plan #56765

pmarchini commented Jan 26, 2025

nodejs-github-bot commented Jan 26, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading

cjihrig left a comment

cjihrig Jan 28, 2025

pmarchini Feb 10, 2025

jsumners commented Jan 28, 2025

mcollina left a comment

nodejs-github-bot commented Feb 9, 2025

atlowChemi Feb 9, 2025

jsumners Feb 10, 2025

pmarchini Feb 10, 2025

jsumners Feb 10, 2025

pmarchini Feb 10, 2025

pmarchini Feb 14, 2025

jsumners Feb 14, 2025

pmarchini Feb 14, 2025

atlowChemi Feb 16, 2025

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Feb 17, 2025

pmarchini commented Feb 17, 2025

pmarchini commented Feb 23, 2025

nodejs-github-bot commented Feb 23, 2025

mcollina left a comment

cjihrig commented Feb 23, 2025

nodejs-github-bot commented Feb 23, 2025

		const { Readable } = require('node:stream');

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

test_runner: add timeout support to test plan #56765

test_runner: add timeout support to test plan #56765

Conversation

pmarchini commented Jan 26, 2025

nodejs-github-bot commented Jan 26, 2025

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners commented Jan 28, 2025

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Feb 17, 2025

pmarchini commented Feb 17, 2025

pmarchini commented Feb 23, 2025

nodejs-github-bot commented Feb 23, 2025

mcollina left a comment

Choose a reason for hiding this comment

cjihrig commented Feb 23, 2025

nodejs-github-bot commented Feb 23, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading