exclude default setting should exclude e2e tests folder

GermanJablo commented 1 year ago

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 or exclude to exclude e2e tests. This change would allow an additional configuration file to be avoided.

I can do the PR.

Alternative

No response

Additional context

*Validated in all the discussions I found on the subject. For example here, or here.

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

sheremet-va commented 1 year ago

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

nickserv commented 1 year ago

I think e2e test patterns are unique enough that it would be helpful to discuss specific differences, for example:

e2e test frameworks tend to use directories named after the test framework or patterns it uses (like cypress. playwright, or features) with code proprietary to their test runners. However, unit test frameworks tend to support consistent globals like describe, it, and expect in Mocha, Chai, Jasmine, Jest, and Vitest. As a result, ignoring an incompatible e2e test is more likely to fix a failing test suite than it is to cause a false positive.
e2e tests can be written in entirely different languages than source code, which is generally not possible with integration and unit tests.
Vitest already ignores tests in cypress, so users of other popular e2e test frameworks may assume files from similar e2e test frameworks would also be ignored. If we included e2e test files consistently, it would also be a breaking change.

GermanJablo commented 1 year ago

Thanks for your response!

@nickmccurdy totally agree! I also reasoned that if the cypress folder was being ignored, then playwright should be ignored as well.

@sheremet-va The discussion with the community that you speak of, do you say we do it in #3530? It seems to me that the decision to ignore test.ts is more difficult than the decision to ignore playwright and e2e-tests. Are there any potential risks that you are seeing with this change?

sheremet-va commented 1 year ago

@sheremet-va The discussion with the community that you speak of, do you say we do it in #3530?

It seems to me that the decision to ignore test.ts is more difficult than the decision to ignore playwright and e2e-tests. Are there any potential risks that you are seeing with this change?

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 e2e-tests folder to run e2e tests with Vitest's e2e workspace for example.

nickserv commented 1 year ago

In that case, wouldn't it be best to remove the cypress exclude so Vite is consistent about this? Of course, that would be a breaking change.

sheremet-va commented 1 year ago

In that case, wouldn't it be best to remove the cypress exclude so Vite is consistent about this? Of course, that would be a breaking change.

Why? It's not a generic name

GermanJablo commented 1 year ago

Yes, I think the dilemma is between excluding playwgright and e2e-tests or excluding playwright only. If it's for me, my vote is in favor of excluding both ✋

nickserv commented 1 year ago

In that case, wouldn't it be best to remove the cypress exclude so Vite is consistent about this? Of course, that would be a breaking change.

Why? It's not a generic name

Neither is playwright, making our e2e test excludes inconsistent, so I'm still in favor of this.

silverwind commented 1 year ago

-1 from me for excluding nonstandard folders. For example some repos I contribute to use tests/e2e, you can never make it right for everyone. The default patterns must be as neutral as possible.

GermanJablo commented 1 year ago

@silverwind Do they have Vitest tests inside tests/e2e? If so, could I know why?

silverwind commented 1 year ago

In this case tests/e2e is playwright, as one might expect.

GermanJablo commented 1 year ago

Oh, gitea is cool! 👌

Well, what you mention confirms the point of this issue, which is make Vitest ignore the e2e folder

By the way, I would like to propose replacing **/{cypress,playwright,e2e-tests}/** by: **{cypress,playwright,e2e}**

So we cover variants like cypress_tests, playwright-tests, e2e, _e2e, .tests-e2e, etc.

What do you think?

silverwind commented 1 year ago

It was not me who came up with it, but we do use the foo.test.e2e.js pattern for e2e test files, and if this is a common pattern, it might also serve well because vitest's default pattern **/*.{test,spec}.?(c|m)[jt]s?(x) does already not match on these files, so no folder-based exclusions would be needed.

sheremet-va commented 1 year ago

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 playwright to excludes and add all new test runners all the time or remove cypress and keep it as is. We could also probably remove dist folder as we did in other places.

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?

silverwind commented 1 year ago

I tend to agree, a unopinionated default config is best long-term and it stops these kind of pointless discussions. BTW, what is .idea and why do you want to keep it?

sheremet-va commented 1 year ago

I tend to agree, a unopinionated default config is best long-term and it stops these kind of pointless discussions. BTW, what is .idea and why do you want to keep it?

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 .vscode, but it's usually quite small

silverwind commented 1 year ago

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 node_modules and .git.

antfu commented 1 year ago

I can't remember why we have the config file in exclude but I agree to remove them from default.

For .cache, .temp, etc, maybe we could ignore all the . folders by default, as the naming in Linux already indicates they are "hidden" files.

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.

GermanJablo commented 1 year ago

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 playwright or cypress but not that library is not that bad at all. And adding it would take 1 minute (taking this discussion as a starting point).

On balance, I think it's better for DX not to add an extra configuration file.

lloydjatkinson commented 10 months ago

The solution I went with and my package.json. This way, Vitest isn't picking up the playwright tests (that fail to run in Vitest anyway currently).

    test: {
        exclude: [
            '**/node_modules/**',
            '**/integration/playwright/**'
        ],
    },

"test:unit": "vitest run",
"test:integration": "playwright test",
"test:integration:ui": "playwright test --ui",

vitest-dev / vitest