-
-
Notifications
You must be signed in to change notification settings - Fork 186
chore: improve noise during tests #26
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
chore: improve noise during tests #26
Conversation
|
@devdumpling is attempting to deploy a commit to the danielroe Team on Vercel. A member of the Team first needs to authorize it. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
danielroe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so much nicer - thank you! ❤️
| // Exclude files that cause parse errors during coverage remapping. | ||
| // The V8 coverage provider uses rolldown to parse source files, but | ||
| // rolldown seems to currently fail on Vite's SSR transform output (`await __vite_ssr_import__`). | ||
| exclude: [ | ||
| '**/node_modules/**', | ||
| 'cli/**', | ||
| 'server/utils/readme.ts', | ||
| 'server/utils/code-highlight.ts', | ||
| 'server/utils/npm.ts', | ||
| 'server/utils/shiki.ts', | ||
| 'server/utils/jsr.ts', | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to ping on closed PR @devdumpling - happy to discuss this else where too.
How do I reproduce these errors? Even after removing the exclude array here I don't see any parsing errors 🤔
Overview
I've been spending some time trying to better understand the repo and like to start by running tests to get a sense of where testing is at.
However, I was hitting quite a bit of noise, and noticed it seems to be mostly two issues:
double
sharpdepWe have two versions of sharp being installed by dependencies, which is then causing
objcto complain down the line. I'd think it's safe to resolve this by overriding thesharpdep. It's only a single minor diff between the two if you look atpnpm why sharp -r. Doing this both resolves the issue and cleans up the double dep.Additionally, I've pinned the
onlyBuiltDepsto specific versions. This is a recent pnpm feature that I've been implementing at work since there have been a lot of accidental builds (e.g. shai hulud).pnpmalready catches a lot, but this makes it even more explicit.vitest / rolldown unable to parse issues
Currently if you run
pnpm test:unityou'll get a slew of noise around parsing errors. I believe what's happening is that they're trying to parse coverage for some top level async await patterns in specific files. This might be fixed by moving fromv8as the provider toistanbul, but didn't want to shake it up too much.I thought for now it might be reasonable to simply ignore the offending files from coverage to limit the noise, but if people feel this loss of visibility is dangerous (it only affects coverage) or illusory, then we can keep it off.
Before / After
This is what you see before (note all the parsing noise that makes TDD hard if you're into that)
Screen.Recording.2026-01-24.at.5.38.07.PM.mov
aaaaand after!
Screen.Recording.2026-01-24.at.5.39.18.PM.mov