src/test.ts is picked up as a test

[danielroe]

Describe the bug

This may not be a bug but I would consider it non-intuitive.

A file named src/test.ts is picked up as a test. I would suggest that test.ts should be required as a suffix rather than directly as a file name.

Context:
Our CI started failing because we have a test command in our source code: https://github.com/nuxt/nuxt/actions/runs/5193588330/jobs/9364317315?pr=21410

Introduced in glob changes in #3392.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-e9txbr?file=test.ts,test/basic.test.ts&initialPath=__vitest__/

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
  npmPackages:
    @vitest/ui: latest => 0.32.0 
    vite: latest => 4.3.9 
    vitest: latest => 0.32.0

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[stackblitz]

Start a new pull request in StackBlitz Codeflow.

• vitest-dev-vitest-e9txbr (Open reproduction)

[sheremet-va]

Our new patterns don't actually align with Jest 100%: 🤔

Jest:

**/?(*.)+(spec|test).[jt]s?(x)

Vitest:

**/?(*.)(spec|test).[jt]s?(x)

But it is still expected change, since the same happens in Jest: https://jestjs.io/docs/configuration#testmatch-arraystring

It will also find files called test.js or spec.js.

[sheremet-va]

I've added a note to release notes about this breaking change.

[Ky6uk]

How to exclude files now? I had test.exclude in the vite.config.ts before v0.32, but it seems doesn't work anymore. I'm trying to exclude 'src/**/__tests__/*.cy.ts' files that had been used for Cypress, but they're picked by Vitest.

[sheremet-va]

How to exclude files now?

The mechanism didn't change. Only default values. If you have custom patterns, it should work. Check that your pattern is correct.

[Ky6uk]

Ahh, my bad. Just checked it closely and exclude was under test.coverage, not just test in the config file. All good now. 👍

[deluksic]

In our codebase, switching to v0.32.0 started running __tests__/testHelper.ts as a test and it fails due to No test suite found

EDIT: setting include: ["**/*.{test,spec}.?(c|m)[jt]s?(x)"] which is the default according to the include docstring, helps

[sheremet-va]

EDIT: setting include: ["**/*.{test,spec}.?(c|m)[jt]s?(x)"] which is the default according to the include docstring, helps

I don't know what documentation you are reading but in 0.32.0 the default is this:

'**/__tests__/**/*.?(c|m)[jt]s?(x)', '**/?(*.){test,spec}.?(c|m)[jt]s?(x)'

[sheremet-va]

This is an expected breaking change which is described in the release notes.

[deluksic]

I understand this is not official docs, but it should probably be updated here as well. This .d.ts file is distributed with 0.32.0.

[nickserv]

Our new patterns don't actually align with Jest 100%: 🤔

Jest:

**/?(*.)+(spec|test).[jt]s?(x)

Vitest:

**/?(*.)(spec|test).[jt]s?(x)

@sheremet-va It seems like our glob parser allows . literals in * matches, as one.two.test.js is correctly run by both Vitest and Jest. So they should be functionally identical.

But it is still expected change, since the same happens in Jest: https://jestjs.io/docs/configuration#testmatch-arraystring

It will also find files called test.js or spec.js.

@sheremet-va is right, picking up files named test was done by me intentionally in #3392 for Jest support. You can still use the old Vitest config if you have files that aren't tests named test, though you may want to consider renaming your files if there are Jest users working on your code.

We should still keep this open for docs though.

[danielroe]

Honestly, when I used jest I never named or even considered naming a test file src/test.ts.

I still think it is counter intuitive and would advise against supporting this pattern. I think the fact that jest had to have a separate sentence explaining that this was the case is a sign of a non intuitive pattern. In general one thing I love about vitest is how zero configuration it is, which is why I raised the issue in the first place. Good defaults/conventions matter.

But it is totally up to you. I will just be adding the pattern to our exclude array. Renaming the file in our own context would be be more confusing.

[sheremet-va]

I think the fact that jest had to have a separate sentence explaining that this was the case is a sign of a non intuitive pattern. In general one thing I love about vitest is how zero configuration it is, which is why I raised the issue in the first place. Good defaults/conventions matter.

I am open to discussing the best course of action here. I do agree that the pattern seems non-intuitive. We can revert the test.ts change if people want to.

On the same note, I think Vitest team should reevaluate what we mean by Jest compatibility and make a paragraph in docs about this. People's requests are getting ridiculous by this point - adding features just because "they work like this in jest, makes for easier migration, and so on".

[nickserv]

As someone who's used Jest for years, I've often used test.js files to demo it in projects with just one test file, and I also think it's a useful pattern for higher level tests like e2e tests and type tests that involve an entire codebase. Several times I've created test.js files and wondered why Vitest wasn't running them. I'm experienced with Vitest, so I was able to fix the issues, but beginners are more likely to assume Vitest isn't working, give up, and continue using Jest. I've gotten positive feedback when I mentioned this change to Jest users.

Can we please start by documenting this change (which I'd be willing to work on) and waiting for more feedback? While I respect the concerns of other maintainers around back compatibility, we did release this with a compatibility warning, and we could prevent additional confusion by improving the docs. In Nuxt's case, I think modifying Vitest's includes or excludes would be the best solution.

Alternatively if the Vitest team decides not to pursue Jest compatibility by default, can we please document this and ideally add some sort of option/config/codemod for Jest compatibility? This could include other differences in defaults from Jest including globals: true.

[so1ve]

I really hate this change :( Using test or spec as a suffix to identify the test files is intuitive, like what d.ts does.

test.ts is too common. Sometimes we create a test.js file to debug locally, vitest is unhappy; If some frameworks have a src/test.ts file to run tests in user's workspace, vitest is unhappy. After all, test.ts is not special enough to identify whether a file is a spec file. Anyway, I'm glad to see vitest team is open to revert this change ❤️

[nickserv]

I really hate this change :( Using test or spec as a suffix to identify the test files is intuitive, like what d.ts does.

Personally as a Jest user, I find assuming a test.ts file is a test more intuitive. I understand that perspective though, which is why I'm still in favor of documenting it more clearly.

If some frameworks have a src/test.ts file to run tests in user's workspace, vitest is unhappy.

Do you have an example of this? Specifically I'm interested in knowing if any frameworks have their own test.js file that wouldn't be compatible with Jest/Vitest APIs.

[so1ve]

Do you have an example of this?

I can't figure out an example right now, sorry :(
However, I think many developers create a test.js/ts file locally to test their library.

[posva]

It's very common for codebases to have mocks and other test utilities within the tests folders, they shouldn't be picked up. Right now even a file named utils.ts is picked up, it's not just files named test.

Including only spec, or test files is a better default for any project, small or big because it enforces a helpful, longstanding convention that, by default, filters out non-test files. In other words, the previous behavior was sensitive default that encouraged better organization practices. Because of this I really think reverting this change would be beneficial for all vitest users. This is one of the points where vitest had a better default than jest 😄

[so1ve]

I've created a draft PR for this, you can merge it once this suggestion is accepted, or simply close it :)

[sheremet-va]

I've created a draft PR for this, you can merge it once this suggestion is accepted, or simply close it :)

PR should be a revert of #3392 if we decide to go back (at least include/exclude)

[so1ve]

I guess we can partially revert it because it still has quite a few parts worth keeping, such as simplified globs. That's why I created a new PR :)

[nickserv]

While I'm still open to discussion on this change, can we please publish docs on these changes before deciding to revert work and publish another breaking change? I agreed to add docs previously, and at the time I didn't realize this was being published without docs. I think most of the confusion that has been communicated in this thread is resulted from a lack of docs clarifying the change (outside of the release notes). Additionally, this issue still has the labels of a documentation fix, not a breaking change. I've pushed #3737 to help resolve the original issue with missing docs.

[posva]

Personally (but I think I’m not the only one) my problem is not about docs, my problem is the new default is worse than the previous one for the mentioned reasons.

It’s not about labeling either, those are just organizational details. As an example: you could label this as a regression and a fix if you wanted to but what matter is not that it breaks, all software does and that’s just fine. What matter is that a sensitive default is adopted (or rather restored)

[antfu]

I wasn't aware of the initial PR. Here are my thoughts:

• As most of us here agree with the fact that this default is counter-intuitive, I think we should revert it in the next breaking changes, to provide sensible defaults for better DX.
• As for "Jest-compactible", to me is something "good to have" rather than following everything with Jest. We do have quite a few misalignments with Jest, that we have noted in the migration guide. So, in this case, I think we should document in the migration note that our defaults are different than Jest, instead of another way around. As ppl would check the docs more carefully on migrations, but not day-to-day use.

To make a trade-off, we could introduce a preset system, or just a bare Jest-favor default object that makes for easier opt-in. Something like:

import { defaultJestConfig } from 'vitest'

export default defineConfig({
  test: {
    ...defaultJestConfig,
	// user configs
  }
}