vitest-dev / vitest

Next generation testing framework powered by Vite.
https://vitest.dev
MIT License
13.05k stars 1.17k forks source link

Watch mode unresponsive to monorepo dependency changes #2532

Open luxaritas opened 1 year ago

luxaritas commented 1 year ago

Describe the bug

I'm attempting to use Vitest in a monorepo, and noticed there are issues getting vitest to re-run tests in watch mode when in-repo dependencies change. I've identified a few specific limitations (if these should be split out into separate issues, let me know): 1) **/dist/** needs to be excluded from watchExclude. This is fine, however I'd expect the default behavior to be smart enough to handle a monorepo where packages are built separately and imported into the package being tested 2) deps.inline has to be set to true for changes to be picked up. Again, this is a reasonable workaround, but since vite "just works" for HMR and watch mode to work correctly, I wouldn't expect it to be necessary for vitest. 3) When rebuilding the dependency with Vite, tests are not rerun - even though if I manually change the file in dist, they are. Additionally, building with Vite then trying to change the file in dist (without restarting the test watcher) also does not cause a test rerun. I tried tweaking the chokidar options via vite's server.watch parameter (namely awaitWriteFinish and atomic) as that seemed like it could affect this, but there was no change in behavior.

Reproduction

Reproduce via https://stackblitz.com/edit/vitest-dev-vitest-fttlzd (ensure you have two terminals available). Stop any watch processes and reset file states between tests.

1) 1) Run turbo run --cwd packages/packageb build:watch in one terminal 2) Change packages/packagea/src/lib.ts 3) Run turbo run --cwd packages/packagea build in the other terminal 4) Note packages/packageb/dist/index.js is updated accordingly 2) 1) Run turbo run --cwd packages/packageb test in one terminal 2) Change packages/packagea/dist/src/lib.ts 3) Note the test watcher updates accordingly 3) 1) Run turbo run --cwd packages/packageb test in one terminal 2) Change packages/packagea/src/lib.ts 3) Run turbo run --cwd packages/packagea build in the other terminal 3) Note the test watcher does not update 4) 1) Comment out deps: { inline: true }, (and/or watchExclude: [],) in packages/packageb/vite.config.ts 2) Run turbo run --cwd packages/packageb test in one terminal 3) Change packages/packagea/dist/src/lib.ts 4) Note the test watcher does not update

Also note that if you run test 3 then 2 without stopping the test watcher, 2 will also fail to update.

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: 7.17.0 - /usr/local/bin/npm
npmPackages:
  vite: 4.0.2 => 4.0.2 
  vitest: 0.26.0 => 0.26.0


### Used Package Manager

npm

### Validations

- [X] Follow our [Code of Conduct](https://github.com/vitest-dev/vitest/blob/main/CODE_OF_CONDUCT.md)
- [X] Read the [Contributing Guidelines](https://github.com/vitest-dev/vitest/blob/main/CONTRIBUTING.md).
- [X] Read the [docs](https://vitest.dev/guide/).
- [X] Check that there isn't [already an issue](https://github.com/vitest-dev/vitest/issues) that reports the same bug to avoid creating a duplicate.
- [X] Check that this is a concrete bug. For Q&A open a [GitHub Discussion](https://github.com/vitest-dev/vitest/discussions) or join our [Discord Chat Server](https://chat.vitest.dev).
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.
luxaritas commented 1 year ago

Just realized that vite's build watch uses rollup's watch mode, not the vite dev server which vitest uses. I changed packageb to be a web app that the vitest dev server can serve, and confirmed that rebuilding the dependency as in case 1 (using vitest instead of vitest build --watch) still caused a proper reload: https://stackblitz.com/edit/vitest-dev-vitest-nhye4s

sheremet-va commented 1 year ago

This should be fixed in 0.31.2.

luxaritas commented 1 year ago

Thanks for the update @sheremet-va - could you point me to the commit/PR in that release that resolves this (since my related PR is still open)?

sheremet-va commented 1 year ago

Thanks for the update @sheremet-va - could you point me to the commit/PR in that release that resolves this (since my related PR is still open)?

3446

luxaritas commented 1 year ago

The issue is not fully resolved yet. I was able to remove the deps inline, but watchExclude still needs to be cleared (as mentioned earlier, easy to work around) and tests to not rerun on a dependency vite build (which is what my PR addresses)

luxaritas commented 1 year ago

Updated Stackblitz: https://stackblitz.com/edit/vitest-dev-vitest-g1bmwe

sheremet-va commented 1 year ago

watchExclude still needs to be cleared (as mentioned earlier, easy to work around)

This is expected behavior.

tests to not rerun on a dependency vite build (which is what my PR addresses)

I left comments in PR.

luxaritas commented 1 year ago

Thanks - I'll revisit the PR once I have some available cycles

sheremet-va commented 1 year ago

This is expected behavior.

On the other hand, it should also be removed from exclude at this point. We are moving away from treating named folders differently.

ekwoka commented 1 year ago

I'm not sure what the situation is right now, but vitest does not respond to rebuilds in the case of a workspace package being rebuilt. I think, ideally, there would be a way to just have vitest directly access the source of the workspace package instead of needing to look at the built package at all.

ahayes91 commented 1 month ago

Just FYI I haven't dived into the full detail here (was snooping through existing issues to get help on something else!) but using an alias might be helpful here, that's how we got coverage working in our monorepo with vitest - https://github.com/vitest-dev/vitest/issues/5218#issuecomment-1952174964

ekwoka commented 1 month ago

@ahayes91 thanks for the suggestion!

It seems like it should work, but it is now just a bit more doubling up of code, and not having a single source of truth...