-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
exclude default setting should exclude e2e tests folder #3102
Comments
We should have a discussion about this with the community - we already changed default globs which cause a lot of errors - please, feel free to discuss it in #3530 |
I think e2e test patterns are unique enough that it would be helpful to discuss specific differences, for example:
|
Thanks for your response! @nickmccurdy totally agree! I also reasoned that if the @sheremet-va The discussion with the community that you speak of, do you say we do it in #3530? |
Yes. I'm not a fan of default excluding. It's much harder to remove a folder than to add it. With this change we can cause some tests to silently not run for example. I don't see why you can't use |
In that case, wouldn't it be best to remove the |
Why? It's not a generic name |
Yes, I think the dilemma is between excluding |
Neither is |
-1 from me for excluding nonstandard folders. For example some repos I contribute to use |
@silverwind Do they have Vitest tests inside |
In this case |
Oh, gitea is cool! 👌 Well, what you mention confirms the point of this issue, which is make By the way, I would like to propose replacing So we cover variants like What do you think? |
It was not me who came up with it, but we do use the |
Vitest should not limit e2e tests as a concept. Vitest is a test runner, you can run unit/integration/e2e tests with Vitest, and no one is stopping you. I think the whole concept of excluding e2e tests is bad by design. Here we should either add My proposal for exclude: [
'**/node_modules/**',
- '**/dist/**',
- '**/cypress/**',
- '**/.{idea,git,cache,output,temp}/**',
+ '**/.{idea,git}/**',
- '**/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build}.config.*',
] @antfu what do you think? |
I tend to agree, a unopinionated default config is best long-term and it stops these kind of pointless discussions. BTW, what is |
It's a folder created by JetBrains IDEs. Since it's standardized (unlike the other three), I decided to keep it. We can also include |
Yes, if you include one editor folder in the list, you also have to include the others, again making this config opinionated. I think the ideal default exclusions are just |
I can't remember why we have the config file in exclude but I agree to remove them from default. For For cypress am don't have a strong opinion, removing it indeed has the value of preventing us from adding more framework specific exclude in the future. |
Well @sheremet-va, I already got the point about e2e in Vitest, thanks for clearing it up. Regarding Playwright and Cypress, since I see that some of you do not have strong opinions about it, I want to say that I would prefer that they be excluded by default. Suppose another test library with a similar popularity emerges in the future. Excluding On balance, I think it's better for DX not to add an extra configuration file. |
The solution I went with and my test: {
exclude: [
'**/node_modules/**',
'**/integration/playwright/**'
],
}, "test:unit": "vitest run",
"test:integration": "playwright test",
"test:integration:ui": "playwright test --ui", |
Clear and concise description of the problem
Since a generally accepted good practice* is to put unit tests next to the code they test, and e2e tests in a separate folder, excluding such a folder would be a better default setting.
In many projects I see that all the configuration file does is modify
include
orexclude
to exclude e2e tests. This change would allow an additional configuration file to be avoided.I can do the PR.
Suggested solution
Add any of the following paths to the default exclude configuration:
Option 1:
'**/tests/**'
This is the most opinionated option. It looks pretty clean, but I can see how some people may have put unit tests inside folders with that name. With this option, they would have to modify the config or rename the folder to something like
/unit-test/
.Option 2:
'**/e2e-tests/**'
Non-opinionated, and not potentially conflicting.
Option 3:
'**/tests/**', '**/e2e-tests/**'
Despite the potential conflict explained in option 1... the goal of default settings is to respond to the most popular settings, so possibly this is also a good option.
Alternative
No response
Additional context
*Validated in all the discussions I found on the subject. For example here, or here.
Validations
The text was updated successfully, but these errors were encountered: