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

test: avoid vitest checking publicDir #15197

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

patak-dev
Copy link
Member

Description

I saw that Vitest when run in the monorepo was checking for vite-path/public while working on #15195. This PR disables the publicDir feature for vitest as we aren't using it.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Nov 30, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

sheremet-va
sheremet-va previously approved these changes Nov 30, 2023
@patak-dev patak-dev added the test label Nov 30, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I think it makes sense to disable in the vitest configs, but do we also need to disable in the manual createServer calls? I think it might be more consistent to leave it unset.

sapphi-red
sapphi-red previously approved these changes Dec 1, 2023
@sapphi-red
Copy link
Member

sapphi-red commented Dec 1, 2023

I think it makes sense to disable in the vitest configs, but do we also need to disable in the manual createServer calls? I think it might be more consistent to leave it unset.

I thought about that too. I'm slightly leaning toward not disabling it in the manual createServer calls, but either is fine for me.

@patak-dev
Copy link
Member Author

Because of the way we use vitest to run unit tests in the monorepo, these deep createServer calls end up with the publicDir enabled at (monorepo-root)/public. I agree we shouldn't disable the public repo in the playgrounds (were it is enabled at (playground-root)/public but these createServer in the unit tests are testing something unrelated to it and it is quite confusing to have it enabled at the monorepo root to me. I can re-enable them though if you prefer that, let me know.

@bluwy
Copy link
Member

bluwy commented Dec 1, 2023

Perhaps we can set the root instead so it's correctly testing against a directory. I would prefer if we don't adjust those publicDir options though unless there's a perf improvement.

@patak-dev patak-dev dismissed stale reviews from sapphi-red and sheremet-va via 3875bd6 December 1, 2023 10:07
@patak-dev
Copy link
Member Author

There is a perf improvement now, but it will be negated if we merge:

I'll remove the changes there for now then, and only leave the ones in Vitest. One problem we may have though is that if these servers start to use the public dir at one point, it may fail because we disable the feature in the vitest config for unit tests (so I still think it is better to disable them, but not a big deal)

@patak-dev patak-dev merged commit 56ef54e into main Dec 1, 2023
10 checks passed
@patak-dev patak-dev deleted the test/avoid-public-dir-checks-in-vitest branch December 1, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants