Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: add TestContext.prototype.waitFor() #56595

Merged
merged 3 commits into from
Jan 16, 2025
Merged

test_runner: add TestContext.prototype.waitFor() #56595

merged 3 commits into from
Jan 16, 2025

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 14, 2025

This commit adds a waitFor() method to the TestContext class in the test runner. As the name implies, this method allows tests to more easily wait for things to happen.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 14, 2025
@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 14, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (e6a988d) to head (531921e).
Report is 27 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56595   +/-   ##
=======================================
  Coverage   89.19%   89.20%           
=======================================
  Files         662      662           
  Lines      191797   191879   +82     
  Branches    36920    36934   +14     
=======================================
+ Hits       171079   171157   +78     
+ Misses      13558    13554    -4     
- Partials     7160     7168    +8
Files with missing lines Coverage Δ
lib/internal/test_runner/test.js 97.06% <100.00%> (+0.13%) ⬆️

... and 66 files with indirect coverage changes

@mcollina
Copy link
Member

I'm not convinced by the use case with abortOnError: true. For the condition to pass, it should not throw, so in other terms it will either pass or fail after the first interval is passed. For abortOnError: true be coherent with the design of this feature (polling), the condition function return value has to matter, in other terms if it's falsy it should continue polling, and if its falsy waitFor should succeed and resolve.

@targos
Copy link
Member

targos commented Jan 14, 2025

I agree with @mcollina and I also think that throwing an error in an normal case is not intuitive (it also makes it impossible to differentiate a bug from an expected condition)

@cjihrig
test_runner: add TestContext.prototype.waitFor()
59c71ee 
This commit adds a waitFor() method to the TestContext class in
the test runner. As the name implies, this method allows tests to
more easily wait for things to happen.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 14, 2025

I have removed abortOnError completely because I was also not sold on it. As far as throwing errors to indicate failure - I picked that because that's how vitest does it.

targos
targos reviewed Jan 14, 2025
View reviewed changes
doc/api/test.md Outdated Show resolved Hide resolved
@cjihrig cjihrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 14, 2025
pmarchini
pmarchini approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@pmarchini pmarchini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mcollina
mcollina approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

atlowChemi
atlowChemi approved these changes Jan 14, 2025
View reviewed changes
test/parallel/test-runner-wait-for.js Outdated Show resolved Hide resolved
@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 14, 2025
@cjihrig cjihrig added request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 15, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 15, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2025
@nodejs-github-bot nodejs-github-bot merged commit 0e7ec5e into nodejs:main Jan 16, 2025
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0e7ec5e

@cjihrig cjihrig deleted the waits-for branch January 16, 2025 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@pmarchini pmarchini pmarchini approved these changes

@targos targos targos approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjihrig @nodejs-github-bot @mcollina @targos @pmarchini @atlowChemi