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

--changed not picking up deep imports #4933

Closed
6 tasks done
blake-newman opened this issue Jan 11, 2024 · 1 comment · Fixed by #4934
Closed
6 tasks done

--changed not picking up deep imports #4933

blake-newman opened this issue Jan 11, 2024 · 1 comment · Fixed by #4934
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@blake-newman
Copy link
Contributor

Describe the bug

--changed does not pick up deep changes.

In the attached reproduction run yarn vitest --run --changed=HEAD

  • No changes made will detect no specs to run. This is expected 👍
  • Changing add will detect no specs to run. This is not expected 👎
  • Changing add-twice.ts will detect no specs to run. This is expected 👍

This is problematic for monorepo like setups (or any setup IMO). I expect the --changed flag to deeply look at dependencies to detect possible spec files to run. At moment this is shallow and only direct imports will cause files to change.

It looks to me that the logic in getTestDependecies is trying to do this, as it recursively calls addImports, however the guard clause for checking if the dep is already added is preventing the deep lookup of depenendencies.

getTestDependencies

  • LN: 401 adds the dep to the collected set
  • LN: 403 invokes looking up further dependencies of that dependency
  • LN: 390 checks if that dependency is already added and bails out

As line 401 adds the collected dependency the further lookups of nested dependencies is not running. This causes the changed file dependencies to be shallow which does not accurately trigger related specs when a nested dependency is changed.

In order to correct the changed logic the logic needs to shift a little. Line 403 and 401 can't simply be shifted as otherwise it will cause any cyclic dependencies to cause a stack overflow. Aware that cyclic dependencies shouldn't exist in user code but having a cyclic dependency shouldn't cause vitest to crash.

Reproduction

github.com/blake-newman/vitest-dev-vitest-kx8wc3

(I couldn't reproduce in stackblitz but checking out locally shows issue)

System Info

Not applicable

Used Package Manager

yarn

Validations

@hi-ogawa hi-ogawa added bug p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Jan 19, 2024
@adrianriddle
Copy link

I am seeing something similar to this when using: vitest related, I am assuming they would use the same underlying function.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants