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

Update Vitest to v1 #1438

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Update Vitest to v1 #1438

merged 3 commits into from
Jan 30, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Jan 30, 2024

Description

  • Updates Starlight’s test dependencies to use the latest version of Vitest, updating from a pre-release to v1.

  • A couple of test snapshots changed to a nicer string escaping algorithm — I inspected them visually and I don’t think anything important changed, mostly just stuff like:

    - "<el attr=\\"val\\">"
    + `<el attr="val">`
  • Updated Vitest coverage thresholds config to match new format as changed in feat!(coverage): glob based coverage thresholds vitest-dev/vitest#4442

Copy link

changeset-bot bot commented Jan 30, 2024

⚠️ No Changeset found

Latest commit: f694b34

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

Copy link

vercel bot commented Jan 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jan 30, 2024 2:58pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Jan 30, 2024 2:58pm

@github-actions github-actions bot added 🌟 core Changes to Starlight’s main package 🌟 tailwind Changes to Starlight’s Tailwind package labels Jan 30, 2024
@astrobot-houston
Copy link
Collaborator

size-limit report 📦

Path Size
/index.html 5.22 KB (0%)
/_astro/*.js 21.54 KB (0%)
/_astro/*.css 13.14 KB (0%)

Copy link
Member

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

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

I also updated Vitest in various packages just to get nicer snippets and they are definitely better imo so 👍 on my end.

Locally (and CI too actually), I get a few [ERROR] [vite] WebSocket server error: Port is already in use errors but I don't think they're relevant to us.

@delucis
Copy link
Member Author

delucis commented Jan 30, 2024

Locally (and CI too actually), I get a few [ERROR] [vite] WebSocket server error: Port is already in use errors but I don't think they're relevant to us.

Yes, I noticed these too. I think it’s something to do with things happening in parallel? @hippotastic reported the same errors in the parallel builds of the example projects and added a --workspace-concurrency 1 option to that package script as part of #1395. I can try that for the tests too, to see if it helps.

@HiDeoo
Copy link
Member

HiDeoo commented Jan 30, 2024

Yes, I noticed these too. I think it’s something to do with things happening in parallel? @hippotastic reported the same errors in the parallel builds of the example projects and added a --workspace-concurrency 1 option to that package script as part of #1395. I can try that for the tests too, to see if it helps.

Interesting, to add more details, I got them when running only the starlight package tests from its directory so not sure this would help (as I think it's a pnpm workspace flag).

@delucis
Copy link
Member Author

delucis commented Jan 30, 2024

Ah, good point — I think I saw that too. We do have multiple Vitest workspaces — perhaps would need those to be run sequentially.

@HiDeoo
Copy link
Member

HiDeoo commented Jan 30, 2024

I wonder if it could be vitejs/vite#14328 and we should just ignore in this specific context of testing (the last comment provide a Vitest workaround but a bit annoying imo).

@delucis
Copy link
Member Author

delucis commented Jan 30, 2024

I wonder if it could be vitejs/vite#14328 and we should just ignore in this specific context of testing (the last comment provide a Vitest workaround but a bit annoying imo).

Nice find! Yeah, I think adding special handling just to hide a warning is not really essential. Eventually they’ll probably fix it and in the mean time we can just not worry about those logs.

@HiDeoo
Copy link
Member

HiDeoo commented Jan 30, 2024

I wonder if it could be vitejs/vite#14328 and we should just ignore in this specific context of testing (the last comment provide a Vitest workaround but a bit annoying imo).

Nice find! Yeah, I think adding special handling just to hide a warning is not really essential. Eventually they’ll probably fix it and in the mean time we can just not worry about those logs.

I agree, and it looks like Vitest is supposed to have this workaround but it's not working for some reasons.

@delucis
Copy link
Member Author

delucis commented Jan 30, 2024

I agree, and it looks like Vitest is supposed to have this workaround but it's not working for some reasons.

Oh yeah… Well, overriding a global like that is always a bit flaky if some other code somewhere also does something to it, so I guess not totally shocking it’s not working. Still, think I’m happy to merge this for now.

@delucis delucis merged commit c5f4a71 into main Jan 30, 2024
10 checks passed
@delucis delucis deleted the chris/update-vitest branch January 30, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 🌟 tailwind Changes to Starlight’s Tailwind package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants