-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: switch from uvu to vitest #9899
Conversation
|
headers: {}, | ||
multiValueHeaders: { | ||
'set-cookie': [`flavor=sugar; Expires=${wednesday}`, `diameter=6cm; Expires=${thursday}`] | ||
} | ||
}); | ||
}); | ||
|
||
test.run(); |
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.
I'm in it for this. 😝
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.
All things being equal, this seems like a minimal footprint in terms of changes. (Have someone w/ more knowledge also review though...)
packages/kit/package.json
Outdated
@@ -66,7 +66,7 @@ | |||
"test:integration": "pnpm -r --workspace-concurrency 1 --filter=\"./test/**\" test", | |||
"test:cross-platform:dev": "pnpm -r --workspace-concurrency 1 --filter=\"./test/**\" test:cross-platform:dev", | |||
"test:cross-platform:build": "pnpm test:unit && pnpm -r --workspace-concurrency 1 --filter=\"./test/**\" test:cross-platform:build", | |||
"test:unit": "uvu src \"(spec\\.js|test[\\\\/]index\\.js)\"", | |||
"test:unit": "vitest run", |
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.
"test:unit": "vitest run", | |
"test:unit": "vitest run", | |
"test:unit:watch": "vitest", |
or even watch mode by default? vitest disables it automatically on CI
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.
I don't know if it's worth it. It's easier to type npx vitest
than pnpm test:unit:watch
. It's also easier to remember as it works across projects vs each project having its own set of scripts in the package.json
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.
If you are planning to use Vitest in templates, I recommend using just vitest
in commands (without run
). There is a reason why Vitest has watch mode enabled by default, and I think it's worth it (we changed how it detects CI, so it's more stable since the answer was given).
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.
thanks! we've actually been offering vitest
in our templates for quite awhile (it's an option that lets you setup playwright, vitest, both, or neither) and do use just the vitest
command there
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.
I looked at using vitest
rather than vitest run
here, but the reason it doesn't work is that it hangs pnpm
try { | ||
const response = await fetch('https://domain-b.com'); | ||
await response.text(); | ||
assert.unreachable('should have thrown cors error'); | ||
throw new Error('should have thrown cors error'); | ||
} catch (e) { | ||
assert.ok(e instanceof Error); | ||
assert.match( | ||
e.message, | ||
/CORS error: No 'Access-Control-Allow-Origin' header is present on the requested resource/ | ||
if (!(e instanceof Error)) { | ||
throw new Error(`Expected an Error to be thrown but received ${typeof e}`); | ||
} | ||
assert.isTrue( | ||
e.message.includes( | ||
"CORS error: No 'Access-Control-Allow-Origin' header is present on the requested resource" | ||
) | ||
); | ||
} |
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.
I think this can be replaced with a expect.rejects.toThrowError
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.
nice, updated
We recommend users to use
vitest
and offer it as an option increate-svelte
, so it would be good if we used it as well so that we have some familiarity with it and can make sure it works well, are better equipped to answer questions, etc.For me locally,
uvu
runs the 242 unit tests in thekit
package in 4.7s.vitest
in 7s by default or 5.7s withisolate: false
andsingleThread: true
which would be equivalent to uvu's defaults.Vitest has a larger group of maintainers, better documentation, and more advanced functionality such as the Vitest UI, a VS Code extension, etc. so there's probably some time tradeoff that would be acceptable given that vitest has so many other points going for it.
This didn't migrate the
create-svelte
orpackage
tests yet, which were doing some more advanced things and I might need some help from the original authors with in followup PRs.It's about 200 lines less code if you ignore the additional lines in
pnpm-lock.yaml
(theuvu
dependency is still present since not all packages have been migrated yet).