Use vitest#expect from the local context#12373
Conversation
|
petebacondarwin
left a comment
There was a problem hiding this comment.
I wonder if test.each is a great pattern if we can't access the expect on the test function callback? I wonder if going forward there is an alternative approach for these iterations?
| @@ -1,3 +1,4 @@ | |||
| // eslint-disable-next-line workers-sdk/no-vitest-import-expect -- test.each pattern | |||
There was a problem hiding this comment.
There are other tests in this file that don't use test.each that perhaps should be getting their expect from the callback args?
There was a problem hiding this comment.
We have a lint rule to prevent shadowing that wouldn't be very happy with that. But again we'll tackle those later.
We can but getting the typings to work is a little trickier. When Claude tried to convert everything in one go, it failed after 6h two times. That's why the PRs are handling simpler cases first and then I'll teach Claude how to fix the more complex cases. |
|
Thanks for the review Pete, added some details to your questions |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
Part of #12346
Part of a series (#12347, #12356) handling simple refactors, one package at a time to keep the review simpler.
The code changes are courtesy of OpenCode/Opus
Note that the tests inside the templates have not been updated
Summary
I've successfully migrated the create-cloudflare package to use context-based expect for vitest concurrency safety. Here's the breakdown:
Changes Made
These files were converted to use { expect } from the test context:
These files use patterns incompatible with context-based expect:
Verification
A picture of a cute animal (not mandatory, but encouraged)