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: improve --test-timeout to be per test #57672

Merged
merged 10 commits into from
Apr 9, 2025
Merged

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

merged 10 commits into from
Apr 9, 2025
+96 −8

Conversation

jakecastelli
Copy link
Member

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
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 Mar 29, 2025
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from fba5496 to a982c95 Compare March 29, 2025 11:34
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from b79d0c3 to 496c282 Compare March 29, 2025 11:41
@jakecastelli
test_runner: improve --test-timeout to be per test
c1c48ec 
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`.
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from 496c282 to c1c48ec Compare March 29, 2025 11:51
@codecov Codecov
Copy link

codecov bot commented Mar 29, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (1123585) to head (44c7757).
Report is 105 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
mcollina approved these changes Mar 30, 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 reviewed Mar 30, 2025
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from 250ef6d to 8a713ec Compare April 4, 2025 15:55
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from 8a713ec to af2201e Compare April 4, 2025 15:56
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from 1f2245c to f593a7b Compare April 4, 2025 16:27
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from f593a7b to 6559293 Compare April 4, 2025 16:35
@jakecastelli jakecastelli requested a review from cjihrig April 8, 2025 15:26
cjihrig
cjihrig reviewed Apr 8, 2025
View reviewed changes
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from f554872 to 215f820 Compare April 8, 2025 17:11
@jakecastelli jakecastelli force-pushed the test-timeout-improvement branch from 215f820 to 04f280b Compare April 8, 2025 17:15
cjihrig
cjihrig approved these changes Apr 9, 2025
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for getting this done!

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@pmarchini
Copy link
Member

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
Copy link
Collaborator

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

@jakecastelli
Copy link
Member Author

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

@jakecastelli 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 pmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025
@nodejs-github-bot 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 nodejs-github-bot merged commit 67786c1 into nodejs:main Apr 9, 2025
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 67786c1

@brianwestphal
Copy link

Thank you!

@jakecastelli jakecastelli mentioned this pull request Apr 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@mcollina mcollina mcollina approved these changes

@cjihrig cjihrig cjihrig approved these changes

@MoLow MoLow MoLow approved these changes

@pmarchini pmarchini pmarchini 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. needs-ci PRs that need a full CI run. 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.

--test-timeout behavior bug
8 participants
@jakecastelli @nodejs-github-bot @pmarchini @brianwestphal @mcollina @cjihrig @MoLow @aduh95