fix(vitest): Fix `cacheDir` not being honored if `deps.optimizer.{mode}.enabled` is `true`

jacoberdman2147 commented 1 week ago

Description

Fixes #6733

So initially it just seemed like the viteConfig.cacheDir set within optimizer should have been prioritizing the one defined in the config but there were other issues with the way cacheDir was being handled within the resolveOptimizerConfig function so I decided to remove the cacheDir logic entirely since it seemed to be unnecessary.

With this, I also fixed an issue where the runVitest function used within some of the tests wasn't properly passing through the options that were given to it. This uncovered the failures within cache.test.ts that should have been happening all along.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

[x] It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
[x] Ideally, include a test that fails without this PR but passes with it.
[x] Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

[ ] Run the tests with pnpm test:ci.

Documentation

[ ] If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

[x] Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

netlify[bot] commented 1 week ago

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
Latest commit	9f7c89dae6447ec57448256ea89b487fafc32428
Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/67353b5cf6dadc0008d19280
Deploy Preview	https://deploy-preview-6902--vitest-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jacoberdman2147 commented 1 week ago

So I spent a while looking into the test failures and I'm not really sure what to do. The specific web.test.ts and ssr.test.ts work fine before my last commit to remove the resolver plugin's handling of cacheDir, but without that commit we run into issues with cache.test.ts in the default case with optimizer enabled. The issue (with all commits) seems to be that the config for the ViteNodeServer isn't using the resolved cacheDir that we get from the resolveConfig method in the Vitest class, but I'm not sure if it should be or not. I don't completely understand how it all works but it seems like the cache for resolved modules and stuff being based around .vite by default could maybe make sense, but it may also make sense to pass down our resolved cacheDir, I'm not sure.

hi-ogawa commented 1 week ago

Thanks for investigating this. I took a look and indeed the current state is quite complicated. But my intuition is that we shouldn't need to change createVitest. We need to figure out how to deal with cacheDir overwritten here for optimize deps:

https://github.com/vitest-dev/vitest/blob/5e6de2742f329b7e65fe3dff4827b5134d777009/packages/vitest/src/node/plugins/optimizer.ts#L22-L23

and we again deriving cache.dir from it

https://github.com/vitest-dev/vitest/blob/5e6de2742f329b7e65fe3dff4827b5134d777009/packages/vitest/src/node/config/resolveConfig.ts#L677-L681

Note that Vite's cacheDir for optimize deps and Vitest's cache.dir for ResultCache are two completely different concepts. Probably there's some limits what we can do, but let me try something in https://github.com/vitest-dev/vitest/pull/6910/.

If we have to choose one, probably test/optimize-deps should pass and cache.test.ts is the one broken.

jacoberdman2147 commented 1 week ago

Yeah it looks like your fix better addresses the cacheDir behavior, thanks for looking into that. As far as the tests go though, that web optimizer test for cache.test.ts wasn't actually getting that optimizer option through to the plugin like it should be, and that's why I had the change in createVitest. Without it, those tests aren't actually testing the behavior that they say they are so it seems like we might still want to keep that part? That is, unless the change you made here https://github.com/vitest-dev/vitest/pull/6910/files#r1843197601 addresses that piece, in which case, I'll close this PR.

hi-ogawa commented 6 days ago

As far as the tests go though, that web optimizer test for cache.test.ts wasn't actually getting that optimizer option through to the plugin like it should be, and that's why I had the change in createVitest. Without it, those tests aren't actually testing the behavior that they say they are so it seems like we might still want to keep that part? That is, unless the change you made here https://github.com/vitest-dev/vitest/pull/6910/files#r1843197601 addresses that piece, in which case, I'll close this PR.

Your observation is right. Passing test: options in createVitest does make sense to me, but I wasn't entirely sure if it has any downside, so I did a smaller change for now. Maybe we'll need to revisit that in the future, but I think we can close this. Thanks again for the contribution :pray:

vitest-dev / vitest