-
Notifications
You must be signed in to change notification settings - Fork 748
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
Run fixture tests with vitest #2496
Conversation
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/3848495686/npm-package-wrangler-2496 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2496/npm-package-wrangler-2496 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/runs/3848495686/npm-package-wrangler-2496 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3848495686/npm-package-cloudflare-pages-shared-2496 |
7d0083d
to
d1d48a2
Compare
Codecov Report
@@ Coverage Diff @@
## main #2496 +/- ##
==========================================
+ Coverage 72.13% 72.28% +0.14%
==========================================
Files 156 156
Lines 9720 9687 -33
Branches 2546 2547 +1
==========================================
- Hits 7012 7002 -10
+ Misses 2708 2685 -23
|
dfc06f6
to
5272479
Compare
5272479
to
aa91875
Compare
"test": "npx vitest", | ||
"test:ci": "npx vitest" |
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.
Why a different script for 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.
Just seems to be the pattern we've adopted everywhere. Didn't want to change too much all in this one PR so left as-was.
@@ -1,7 +1,7 @@ | |||
{ | |||
"extends": "../tsconfig.json", | |||
"compilerOptions": { | |||
"types": ["node", "jest"] | |||
"types": ["node", "vitest"] |
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.
Actually, is @types/vitest
even a thing? Do we need to specify "vitest"
here?
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.
Ah, I think you're right. Not required. They have vitest/globals
if you're using the global helpers like describe
, so I think my brain just crossed wires and blindly added vitest
here when it isn't needed.
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.
No I don't think it is, Vitest types should just work. Especially if you are explicitly importing in the test utilities which is one of my only requests, which is not to make everything magically global like Jest
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.
A LOT of it.concurrent
I think we can just do it at the top level if we want the suites to run in parallel.
If you use .concurrent
on a suite, every test in it will be run in parallel.
https://vitest.dev/guide/features.html#running-tests-concurrently
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.
With less green from the it.concurrent
I can really see migration more clearly and it looks great. I love the Jest parity they did that is just enough that changes are not drastic but still beneficial!
Adopts vitest for fixture tests (except for the
local-mode-tests
which are a bit different).Author has included the following, where applicable:
Reviewer has performed the following, where applicable: