feat(wrangler): createTestHarness API#14169
Conversation
🦋 Changeset detectedLatest commit: c30299f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
✅ All changesets look good |
|
Review posted successfully to PR #14169. Summary of what I flagged:
|
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
@cloudflare/wrangler-bundler
commit: |
32239cf to
e906464
Compare
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Looks great to me! 🤩
I just left some minor comments
(It is a late in the day today, I might have missed some bits, I might have another review tomorrow as well to go a bit more in depth in a few parts 😄 )
|
|
||
| this.on("error", (event: ErrorEvent) => { | ||
| if (event.recoverable) { | ||
| return; |
There was a problem hiding this comment.
what does this do? (could you add a code comment?)
There was a problem hiding this comment.
DevEnv suppress bundling errors right now as they are likely recoverable by the users (e.g. build failed / fail to parse an invalid config) and there is no way to know if the build fails outside. I am now populating those errors with recoverable: true so createServer() can capture those through the error events and throw a proper error in the test environment.
I will add some code comments :)
There was a problem hiding this comment.
It looks like reusing the error event here could be problematic as we are using event.once() in several places which will throw when it sees an error event. I might need to populate these using a different event name.
There was a problem hiding this comment.
Switched to a new event (buildFailed) for these recoverable errors in 0589d35.
| * Cloudflare account ID used when an operation requires account context. | ||
| * Defaults to the account selected by Wrangler auth when needed. | ||
| */ | ||
| accountId?: string | undefined; |
There was a problem hiding this comment.
what about also including the account token as input? 🤔
There was a problem hiding this comment.
Good question. It works with the account token defined in the environment variables already. If we add an option for that, users are likely still reading from the env vars:
const server = createServer({
workers: [ ... ],
apiToken: process.env.MY_API_TOKEN,
});Something like this?
There was a problem hiding this comment.
yeah that's what I was thinking 🙂
users could create different servers with different accounts and tokens, that would be super flexible (although I am not 100% sure why someone would need it 😅)
For example
const serverA = createServer({
workers: [ ... ],
accountId: "xxx",
apiToken: process.env.MY_API_TOKEN_A,
});
const serverA = createServer({
workers: [ ... ],
accountId: "xxx",
apiToken: process.env.MY_API_TOKEN_B, // note: this token has different permissions compared to token A
});
// This server simulates workers present on a completely different account
const externalServer = createServer({
workers: [ ... ],
accountId: "yyy",
apiToken: process.env.ACCOUNT_Y_API_TOKEN,
});By the way, if I remember correctly, this isn't even possible today with startWorker since Wrangler assumes that the api token is a single one for the whole process 😓
So this might be difficult to implement here... maybe it could be considered as an improvement later on it we want? 🤔
There was a problem hiding this comment.
Yeah, that makes sense! But I would prefer not to support it for now. Since we have dropped the dev server aspect of this API, I have simplified the options it accepts and removed accountId as well.
We can revisit these options later. For now, let's keep the API simple and recommend users set CLOUDFLARE_ACCOUNT_ID and CLOUDFLARE_API_TOKEN instead.
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
|
||
| const server = createServer({ | ||
| workers: [ | ||
| { configPath: path.resolve(helper.tmpPath, "./wrangler.jsonc") }, |
There was a problem hiding this comment.
small q: is this path.resolve() needed? will it work with just "wrangler.jsonc"?
There was a problem hiding this comment.
It resolves relative path with process.cwd() by default. So a plain "wrangler.jsonc" should work for most users.
| ); | ||
| }); | ||
|
|
||
| it("routes fetches based on worker routes", async ({ expect }) => { |
There was a problem hiding this comment.
Ooh, this is interesting! We don't really expose this anywhere else—we should make a big deal of it in the changelog
There was a problem hiding this comment.
I have some similar ideas about routing to Workers based on routes in the Vite plugin so this fits in nicely.
| name: "auxiliary-worker", | ||
| main: "src/auxiliary.ts", | ||
| compatibility_date: "2026-05-20", | ||
| routes: ["auxiliary.example.com/*"], |
There was a problem hiding this comment.
It looks like this is the Wrangler config format rather than the new one we're working on. I think that's fine for now, and we can add support for the new config format under an experimental flag in future. @jamesopstad something to be aware of as a follow-on to the wrangler config PR
| expect(workerCountAfterRejectedUpdate).toBe(1); | ||
| }); | ||
|
|
||
| it("rejects update when a worker fails to build", async ({ expect }) => { |
There was a problem hiding this comment.
In the not-very-distant future I expect we'll want to not support builds at all with this API, and require a build output directory to be passed in. Does that line up with your thinking?
There was a problem hiding this comment.
Yep. I added this because build errors are considered recoverable in wrangler dev, but should be fatal in this test API.
| const response = await server.getWorker("missing-worker").fetch("/"); | ||
| expect(response.status).toBe(404); |
There was a problem hiding this comment.
Could you expand on this choice? I think I'd expect getWorker() to throw in this case, but I haven't thought about it as much as you have
| /** The configuration path of the worker, or a normalized configuration object. */ | ||
| config?: string | Config; |
There was a problem hiding this comment.
This isn't really how this is intended to work. The purpose of config is to provide the base config for a Worker, which will then be overriden by the other SDW properties. Is there a reason people need to pass in custom properties to config rather than the other SDW properties? Everything relevant in config should be overrideable.
There was a problem hiding this comment.
Discussed this with Samuel in person.
For context, this is mostly a minimal hack to avoid introducing a translation layer to map wrangler config back to the sdw interface.
| /** Treat this as the primary worker in a multiworker setup (i.e. the first Worker in Miniflare's options) */ | ||
| multiworkerPrimary?: boolean; | ||
| /** Whether to infer the local request origin from configured routes. */ | ||
| inferOriginFromRoutes?: boolean; |
There was a problem hiding this comment.
Why does this need to be configurable? Should it also be configurable in a config file?
There was a problem hiding this comment.
Wrangler currently override the host of the request url based on the first Worker routes in https://github.com/cloudflare/workers-sdk/pull/14169/changes#diff-524c0b6c44e4e63168f1849abe9420fbb7c44f64c3a6166f44515fdd00b2118dR155
But I find it quite confusing for multi workers especially in a test environment, so I added this option and disable it for createServer().
I don't think we have such logic in the vite plugin.
| const secrets = configParam.secrets | ||
| ? { | ||
| ...configParam.secrets, | ||
| required: configParam.secrets?.required?.filter( | ||
| (secret) => inputBindings?.[secret]?.type !== "secret_text" | ||
| ), | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
createServer() can override vars and secrets per Worker.
This checks whether a required secret was already injected by that override via inputBindings, so the required secrets logic doesn't consider them missing even they are absent from .env or .dev.vars.
| outboundService?: ServiceFetch | undefined; | ||
| }; | ||
|
|
||
| export type WorkerHandle = { |
There was a problem hiding this comment.
followup—this should support other handlers, not just scheduled
There was a problem hiding this comment.
I am planning to add email() handler support next. Any other handlers you have in mind?
| }, | ||
| }, | ||
| legacy: { | ||
| site: (config) => { |
There was a problem hiding this comment.
Why don't we just not support Workers Sites here? They're very legacy and I'm not sure we should be adding them to a net-new API
There was a problem hiding this comment.
Yeah, I agree. Dropped workers site support in c5e2e18.
1c09491 to
58e8d93
Compare
778a379 to
c5e2e18
Compare
There was a problem hiding this comment.
This looks great! Nice work @edmundhung.
For my understanding, is the only thing preventing us using Miniflare directly rather than startDevWorker here that this currently needs to do bundling?
| ); | ||
| }); | ||
|
|
||
| it("routes fetches based on worker routes", async ({ expect }) => { |
There was a problem hiding this comment.
I have some similar ideas about routing to Workers based on routes in the Vite plugin so this fits in nicely.
| ], | ||
| })) | ||
| ).rejects.toThrowErrorMatchingInlineSnapshot( | ||
| `[Error: Updating the number of workers running in the server is not supported.]` |
There was a problem hiding this comment.
Why is this, out of interest?
| await expect(response.text()).resolves.toBe("Hello from inline config"); | ||
| }); | ||
|
|
||
| it("loads default .env files for config path workers", async ({ expect }) => { |
There was a problem hiding this comment.
Are these loaded relative to the Worker's config file?
There was a problem hiding this comment.
Just wanted to say that this might be the nicest test file I've seen in workers-sdk! Very easy to follow.
| /** | ||
| * Wrangler environment to load from the config file. | ||
| */ | ||
| env?: string; |
There was a problem hiding this comment.
This is OK for now but note that with the Build Output API the config will always be flat.
| /** | ||
| * Test-only vars that override vars from the Wrangler config. | ||
| */ |
There was a problem hiding this comment.
OK for now but In the new config we're splitting vars into text and json to better reflect the underlying types and how they're shown in the dashboard.
| const request = new Request(input, init); | ||
| const headers = new Headers(request.headers); | ||
|
|
||
| headers.set("MF-Route-Override", worker); |
There was a problem hiding this comment.
Just a note for the future (maybe next Miniflare version). I think we should make these MF- prefixed headers internal to Miniflare and instead expose the properties we need on miniflare.dispatchFetch().
Fixes n/a.
This introduces
createTestHarness()for integration testing WorkersIt runs Workers in a local preview environment using production build output and works with both Wrangler projects and Workers built by the Cloudflare Vite plugin.
Use it from any Node.js test runner to send requests to individual Workers, trigger scheduled events, reset the server between tests, and mock outbound requests with libraries that intercept
globalThis.fetch(), such as MSW:A picture of a cute animal (not mandatory, but encouraged)