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

fix(vitest): test deep dependencies change detection #4934

Conversation

blake-newman
Copy link
Contributor

@blake-newman blake-newman commented Jan 11, 2024

--changed flag is not correctly looking up deep dependencies, as such the changed flag does not trigger specs if a sub dependency is changed.

Alters the logic to add the collected dependency after recursive addImports is called, otherwise the logic returns early because that dependency has already been added to the collected deps set. So it will not look deeply.

Resolves #4933
(Also Resolves #5009)

Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 89678fc
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65aa529760ee3400086f6841

@blake-newman blake-newman force-pushed the blake.newman/fix-changed-detection-deep-deps branch from 27e50f0 to 4e5589a Compare January 15, 2024 11:03
Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! (also reproduction and your analysis are awesome!)

This looks like a bug to me since the same logic is also used for vitest related https://vitest.dev/guide/cli.html#vitest-related and it also misses deep dependency. I checked it based on your repro https://stackblitz.com/edit/github-ndav2w?file=package.json

For the sake of verifying this PR, I don't think setting up new tests for --changed is necessary (actually there seems none currently probably because it's tedious to setup git), but It would be great if you can expand existing tests in test/changed/fixtures/related to show that deep dependencies is covered.

packages/vitest/src/node/core.ts Outdated Show resolved Hide resolved
Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test case for this change. The tests for this feature are located in test/changed

@blake-newman blake-newman force-pushed the blake.newman/fix-changed-detection-deep-deps branch 4 times, most recently from 0f98151 to 3bfbfc8 Compare January 19, 2024 10:40
`--changed` flag is not correctly looking up deep dependencies, as such
the changed flag does not trigger specs if a sub dependency is changed.

Alters the logic to add the collected dependency after recursive `addImports`
is called, otherwise the logic returns early because that dependency
has already been added to the collected deps set. So it will not
look deeply.
@blake-newman blake-newman force-pushed the blake.newman/fix-changed-detection-deep-deps branch from 3bfbfc8 to 89678fc Compare January 19, 2024 10:44
@blake-newman
Copy link
Contributor Author

@sheremet-va @hi-ogawa i've added the relevant tests (including imports / re-export syntax).

I also tested locally to ensure that the specs fail if the changes are not applied.

Copy link
Contributor

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@sheremet-va sheremet-va merged commit 9c7c0fc into vitest-dev:main Jan 20, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vitest related with barrel files --changed not picking up deep imports
3 participants