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 matching every .ts and .js if glob is not provided #56546

Open
marco-ippolito opened this issue Jan 10, 2025 · 5 comments
Open

Test runner matching every .ts and .js if glob is not provided #56546

marco-ippolito opened this issue Jan 10, 2025 · 5 comments
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 10, 2025

Version

Test on v23.6.0 and v22.10.0

Platform

All

Subsystem

test_runner

What steps will reproduce the bug?

Consider this folder structure:

└── test
    ├── fixtures
    │   ├── boom.js
    │   └── boom.ts
    └── index.test.js

When running node --test the test runner will execute ./test/fixtures/boom.js.
In v23 it will also execute ./test/fixtures/boom.ts, since --experimental-strip-types has been unflagged.

marcoippolito@marcos-MacBook-Pro test % node --test /Users/marcoippolito/Documents/projects/test/test/fixtures/boom.js:1 throw new Error('boom'); ^

Error: boom
at Object. (/Users/marcoippolito/Documents/projects/test/test/fixtures/boom.js:1:7)
at Module._compile (node:internal/modules/cjs/loader:1739:14)
at Object..js (node:internal/modules/cjs/loader:1904:10)
at Module.load (node:internal/modules/cjs/loader:1473:32)
at Function._load (node:internal/modules/cjs/loader:1285:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:234:24)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:151:5)
at node:internal/main/run_main_module:33:47

Node.js v23.6.0
✖ test/fixtures/boom.js (36.07275ms)
'test failed'

/Users/marcoippolito/Documents/projects/test/test/fixtures/boom.ts:1
throw new Error('boom TS');
^

Error: boom TS
at Object. (/Users/marcoippolito/Documents/projects/test/test/fixtures/boom.ts:1:7)
at Module._compile (node:internal/modules/cjs/loader:1739:14)
at Object.loadTS [as .ts] (node:internal/modules/cjs/loader:1831:10)
at Module.load (node:internal/modules/cjs/loader:1473:32)
at Function._load (node:internal/modules/cjs/loader:1285:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:234:24)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:151:5)
at node:internal/main/run_main_module:33:47

Node.js v23.6.0
✖ test/fixtures/boom.ts (62.136209ms)
'test failed'

✔ should return true (0.3725ms)
ℹ tests 3
ℹ suites 0
ℹ pass 1
ℹ fail 2
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 72.075583

✖ failing tests:

test at test/fixtures/boom.js:1:1
✖ test/fixtures/boom.js (36.07275ms)
'test failed'

test at test/fixtures/boom.ts:1:1
✖ test/fixtures/boom.ts (62.136209ms)
'test failed'

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Maybe this is intended behavior but I'd would expect matching .test.js.
I think it's an undesired side effect to execute everything.
I immagine breakages due to a lot of .ts fixtures being executed.

What do you see instead?

Everything is executed.

Additional information

I know changing this would be a breaking change, but I dont think it's sane as it is.

@marco-ippolito marco-ippolito added the test_runner Issues and PRs related to the test runner subsystem. label Jan 10, 2025
@targos
Copy link
Member

targos commented Jan 10, 2025

It matches the files because they are under a test folder.
The default matching behavior is documented here: https://nodejs.org/docs/latest/api/test.html#running-tests-from-the-command-line

@pmarchini
Copy link
Member

As correctly pointed out by @targos, this behaviour is intended.
I believe this is mostly a logic inherited from tapjs.
Checking other tools, this behaviour is different, and only **/*.{test,spec}.?(c|m)[jt]s?(x) are being matched.

IMHO, it is common to have test helpers/fixtures under the test directory.
For this reason, I believe we should consider changing this behaviour, even if it means introducing a breaking change.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 10, 2025

Mocha also checks for a test/ directory. Independent from that, I would be -1 to changing this default. It's not just a breaking change, but an aggressive breaking change because people often run their tests across multiple versions of Node. The introduction of globbing (while a great change) has caused many headaches between Node 20 and 22. This change would not have nearly the same upside.

@marco-ippolito
Copy link
Member Author

I think executing only .test.js/ts would be a better default behavior.

@AugustinMauroy
Copy link
Member

I agree that a huge breaking change. But I'm also convinced that **/**.test.{cjs,mjs,js} or **/**-test.{cjs,mjs,js} is the healthiest for the ecosystem. Knowing that the test runner has not yet been given too much attention

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants