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

[next] feat(vitest): migrate from jest to vitest #4599

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Oct 3, 2023

This migrates the test runner from jest to vitest for the vue 3 next branch.

Edit: Fixed, see #4599 (comment) for details.

@raimund-schluessler raimund-schluessler added help wanted Extra attention is needed 2. developing Work in progress feature: testing Related to tests dependencies Pull requests that update a dependency file configuration Pull requests that update a config file vue 3 Related to the vue 3 migration labels Oct 3, 2023
@raimund-schluessler raimund-schluessler added this to the 9.0.0 next Vue 3 milestone Oct 3, 2023
@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Oct 3, 2023

However, there seems to be an issue with nextcloud/vite-config. With the config in place, the test runner just gives

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Parse failure: Unexpected token (16:95)
At file: /@vite/env
Contents of line 16: const defines = {"globalThis.process.env.NODE_ENV": ("test"), "globalThis.process.env.": (({}).), "global.process.env.NODE_ENV": ("test"), "global.process.env.": (({}).), "process.env.": (({}).), "__VUE_OPTIONS_API__": true, "__VUE_PROD_DEVTOOLS__": false, };
 ❯ ssrTransformScript node_modules/vite/dist/node/chunks/dep-df561101.js:55343:15
 ❯ ssrTransform node_modules/vite/dist/node/chunks/dep-df561101.js:55318:12
 ❯ Object.ssrTransform node_modules/vite/dist/node/chunks/dep-df561101.js:64992:20
 ❯ ViteNodeServer._transformRequest node_modules/vite-node/dist/server.mjs:360:36
 ❯ ViteNodeServer._fetchModule node_modules/vite-node/dist/server.mjs:329:17
 ❯ MessagePort.<anonymous> node_modules/vitest/dist/vendor-index.b271ebe4.js:59:20

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Without the config file vite.config.mts, the tests run. @susnux Do you have an idea what could be wrong?

@susnux It seems that vitest stumbles upon a dot in the vitest baseConfig. Adjusting it like this:
nextcloud-libraries/nextcloud-vite-config@10dfa94
makes the tests run. Is this dot there for a reason?

@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 3, 2023
@raimund-schluessler raimund-schluessler marked this pull request as ready for review October 3, 2023 12:03
@raimund-schluessler raimund-schluessler changed the title [next] feat(vitest): migrate from jest to vitest [next] feat(vitest): migrate from jest to vitest Oct 3, 2023
@susnux
Copy link
Contributor

susnux commented Oct 3, 2023

makes the tests run. Is this dot there for a reason?

Yes, it replaces e.g. process.env. (see the dot) with ({}].
Basically removing process.env access, I have to look a bit deeper into this as I think it only should be replaced for apps and not libraries.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@raimund-schluessler
Copy link
Contributor Author

makes the tests run. Is this dot there for a reason?

Yes, it replaces e.g. process.env. (see the dot) with ({}]. Basically removing process.env access, I have to look a bit deeper into this as I think it only should be replaced for apps and not libraries.

Should we wait for this until we merge this PR? Or already go ahead since it seems to work for now?

@ShGKme ShGKme removed their request for review October 4, 2023 21:07
@ShGKme
Copy link
Contributor

ShGKme commented Oct 4, 2023

I removed my review request here because I don't feel confident to take responsibility for migration to Vitest.

While it simplifies configuration by sharing Vite bundling config, works with ESM, and in some cases faster (but in some other slower because of re-initialization of each imported module in each test again), from my feelings, it's still quite a young tool with a small team. Even Vite, with all its support, still has many issues with almost every minor update.

At the same time, I don't have much experience with Vitest or its source to know its "special spots".

@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

I removed my review request here because I don't feel confident to take responsibility for migration to Vitest.

While it simplifies configuration by sharing Vite bundling config, works with ESM, and in some cases faster (but in some other slower because of re-initialization of each imported module in each test again), from my feelings, it's still quite a young tool with a small team. Even Vite, with all its support, still has many issues with almost every minor update.

At the same time, I don't have much experience with Vitest or its source to know its "special spots".

Though @nextcloud/vue is not large and we don't have many unit tests, so, we can go back if we find many issues.

@susnux
Copy link
Contributor

susnux commented Oct 5, 2023

Though @nextcloud/vue is not large and we don't have many unit tests, so, we can go back if we find many issues.

That is true and it is a lot easier to configure, especially as dependencies move to ESM and we already getting problems with them. As we can not externalize them for CJS build. Sometime in the future we will have to ESM only too.

@raimund-schluessler
Copy link
Contributor Author

I rebased the vite-config to the latest changes in main and switched to happy-dom. Should be good now.

I think even though vitest is still quite young, its configuration is a lot easier than for jest. If we really find an unfixable problem, we can still go back.

@raimund-schluessler raimund-schluessler removed the help wanted Extra attention is needed label Oct 6, 2023
@raimund-schluessler raimund-schluessler merged commit eb9cea8 into next Oct 6, 2023
15 checks passed
@raimund-schluessler raimund-schluessler deleted the feat/noid/vitest branch October 6, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews configuration Pull requests that update a config file dependencies Pull requests that update a dependency file feature: testing Related to tests vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants