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.

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

test_runner: change ts default glob #57359

Merged

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Mar 7, 2025

This PR changes the default glob for matching.ts files.
Basically will match only *test.{ts,mts,cts} inside the test folder.
This is the safest and middle ground to avoid breaking users.
I'd not consider a breaking change in v23 since typescript support is experimental.

Fixes: #56546

@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 7, 2025
@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Mar 7, 2025
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (4cc69f9) to head (464688e).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/utils.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57359      +/-   ##
==========================================
+ Coverage   90.20%   90.21%   +0.01%     
==========================================
  Files         630      630              
  Lines      185307   185307              
  Branches    36271    36267       -4     
==========================================
+ Hits       167160   167181      +21     
- Misses      11085    11089       +4     
+ Partials     7062     7037      -25     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 89.02% <100.00%> (-0.49%) ⬇️
lib/internal/test_runner/utils.js 58.66% <66.66%> (ø)

... and 22 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.

@marco-ippolito marco-ippolito force-pushed the drop-some-ts-test-runner branch from fd014be to 844d7b0 Compare March 7, 2025 15:13
@marco-ippolito marco-ippolito force-pushed the drop-some-ts-test-runner branch from 844d7b0 to 464688e Compare March 7, 2025 15:14
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

I have a rough feeling this is semver-major.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Mar 7, 2025

Since ts is experimental its should be fine, also from my pov we are fixing a bug.
For example by matching all .ts, it was matching also .d.ts.
I know it might break some people but this is a much safer default

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2025
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

@mcollina
Copy link
Member

mcollina commented Mar 7, 2025

@marco-ippolito ok

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

@JakobJingleheimer
Copy link
Member

For v24.0, can we remove the restriction requiring a test folder?

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Mar 7, 2025

I think the issue is that people keep their test.ts in the src folder and transpile them, so they will be executed twice.
Once node v20 goes EOL we can be more lax and include other patterns

@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 7, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@nodejs-github-bot
Copy link
Collaborator

@pmarchini pmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 9, 2025
@nodejs-github-bot nodejs-github-bot merged commit 9df0ff7 into nodejs:main Mar 9, 2025
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9df0ff7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test runner matching every .ts and .js if glob is not provided
8 participants