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

docs: Clarify how to run unit tests in a browser-like environment #14243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DanielSidhion
Copy link

Closes #14239.

The docs had some missing code when setting up vitest for testing in a browser-like environment, which this PR addresses. It also clarifies that this is needed for tests that use $effect.root.

Given that a new svelte project (with vitest) created with the CLI doesn't change vite.config.js to include that browser-like environment config, it's easy to miss this extra config that is needed.

Copy link

changeset-bot bot commented Nov 10, 2024

⚠️ No Changeset found

Latest commit: ea9e08a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14243-svelte.vercel.app/

this is an automated message

@dominikg
Copy link
Member

see #14239 (comment)

the use of jsdom should be limited

@DanielSidhion
Copy link
Author

Can you please explain in more details why using jsdom should be limited, and why it's not the right way? You made these assertions without explanations.

Here's my reasoning for this PR: the docs already have a step to tell vitest to use the browser entry points, and having vitest use jsdom seems like a missed step rather than a new step. This is especially true because the docs have an example that doesn't work (using $effect.root) if you're just following the docs.

If your concern is with bringing the "browser version" of code of all packages into tests, the docs also already have a link to testing-library/svelte-testing-library#222 (comment) which should give people good pointers on how to separate those things.

@dominikg
Copy link
Member

if you add jsdom environment to test server code, and then load code that references eg the window global, that code might pass your test, but can fail when deployed into a node environment because there window is not defined.

See the example config i made here: https://github.com/dominikg/vitest-example-svelte5/blob/main/vitest.workspace.ts

where there are 2 "vitest workspaces" for tests, one in a client environment with jsdom and svelte-testing-library, and the other a node environment without client addons. Only files with .svelte in their name get tested in the client environment to avoid tests for server-only not failing as needed.

The template for adding vitest with sv add and our docs need to be updated to better address this. The current issue with "all files are tested in node environment so tests requiring a browser env fail" is not best solved with "lets run all tests in a browser env and risk others failing because of it"

@DanielSidhion
Copy link
Author

Thank you! I understand better now the issue. I can do a bigger pass on this docs page to add this information, would that be ok, or is it a requirement to update the docs and sv add at the same time?

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.

Effects in unit tests don't run
3 participants