-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Migrate tests from unstable_dev() to unstable_startWorker() #12053
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
base: main
Are you sure you want to change the base?
Conversation
- Migrate fixtures/local-mode-tests (5 test files) to use unstable_startWorker
- Migrate fixtures/no-bundle-import to use unstable_startWorker
- Delete fixtures/unstable_dev/ (only existed to test the deprecated API)
- Delete packages/wrangler/src/__tests__/api-dev.test.ts (deprecated API tests)
- Remove unstable_dev() test block from e2e/dev-registry.test.ts
- Remove unused Unstable_DevWorker imports from get-platform-proxy tests
- Remove unstable_dev label from auto-assign-issues.ts
Key API changes in migrated tests:
- unstable_dev(path, opts) → unstable_startWorker({ entrypoint: path, ...opts })
- vars: { KEY: val } → bindings: { KEY: { type: 'plain_text', value: val } }
- worker.stop() → worker.dispose()
- worker.fetch('/path') → worker.fetch('http://example.com/path')
…t-pool-workers - Replace unstable_dev() with @cloudflare/vitest-pool-workers - Use SELF.fetch() for testing the worker - Use vi.spyOn(globalThis, 'fetch') to mock outbound requests to remote worker - Configure isolatedStorage: false to preserve KV state between tests - Add resolve.alias to fix promjs broken package.json (main points to lib/index.js but files are at root) - Update tests/tsconfig.json with vitest-pool-workers types All 25 tests pass.
- Migrate middleware.test.ts (26 tests) - Migrate middleware.scheduled.test.ts (10 tests) - Delete unused templates/init-tests/ directory (dead code)
|
| // Let other requests pass through (e.g., prometheus logging) | ||
| // In tests, we don't want these to fail |
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.
| // Let other requests pass through (e.g., prometheus logging) | |
| // In tests, we don't want these to fail |
| vi.spyOn(globalThis, "fetch").mockImplementation( | ||
| createMockFetchImplementation() | ||
| ); |
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.
This should be in a beforeEach()
| beforeAll(() => { | ||
| vi.spyOn(globalThis, "fetch").mockImplementation( | ||
| createMockFetchImplementation() | ||
| ); | ||
| }); |
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.
Get rid of this and use beforeEach()
| vi.spyOn(globalThis, "fetch").mockImplementation( | ||
| createMockFetchImplementation() | ||
| ); |
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.
As above, use a beforeEach
| beforeAll(() => { | ||
| vi.spyOn(globalThis, "fetch").mockImplementation( | ||
| createMockFetchImplementation() |
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.
Do you really need this again? The remote is the same as the one above
| origin: "https://cloudflare.dev", | ||
| "X-CF-Token": token, | ||
| "X-CF-Remote": `http://127.0.0.1:${remote.port}`, | ||
| "X-CF-Remote": `${MOCK_REMOTE_URL}`, |
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.
This template stirng is overkill
| origin: "https://cloudflare.dev", | ||
| "X-CF-Token": token, | ||
| "X-CF-Remote": `http://127.0.0.1:${remote.port}`, | ||
| "X-CF-Remote": `${MOCK_REMOTE_URL}`, |
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.
This template string is overkill
| origin: "https://cloudflare.dev", | ||
| "X-CF-Token": token, | ||
| "X-CF-Remote": `http://127.0.0.1:${remote.port}`, | ||
| "X-CF-Remote": `${MOCK_REMOTE_URL}`, |
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.
This template string is overkill
| origin: "https://cloudflare.dev", | ||
| "X-CF-Token": token, | ||
| "X-CF-Remote": `http://127.0.0.1:${remote.port}`, | ||
| "X-CF-Remote": `${MOCK_REMOTE_URL}`, |
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.
Okay, get rid of any and all redudant template strings
| // promjs has broken package.json (main points to lib/index.js but files are at root) | ||
| resolve: { | ||
| alias: { | ||
| promjs: path.resolve(__dirname, "node_modules/promjs/index.js"), | ||
| }, | ||
| }, | ||
| }); |
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.
Is this needed? Did you have a look at the recommended SSR optimisations for vitest-pool-workers from the docs? Can you apply that to promjs?
| import * as fs from "node:fs"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { unstable_dev } from "../api"; | ||
| import { startWorker as unstable_startWorker } from "../api/startDevWorker"; |
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 import it as startWorker
| import dedent from "ts-dedent"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { unstable_dev } from "../api"; | ||
| import { startWorker as unstable_startWorker } from "../api/startDevWorker"; |
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 import it as startWorker
| // Note: worker.fetch() doesn't work correctly with paths when | ||
| // EXPERIMENTAL_MIDDLEWARE=true is set. The request URL pathname gets | ||
| // lost, causing the worker to not match routes like "/setup". | ||
| // We use native fetch() with the worker URL as a workaround. | ||
| // See: https://github.com/cloudflare/workers-sdk/issues/XXXX |
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.
Change this to a TODO comment. We should get to the bottom of it
- Use beforeEach instead of beforeAll + afterEach re-mock pattern - Remove redundant template strings for MOCK_REMOTE_URL - Remove unnecessary comment about prometheus logging - Use @cloudflare/vitest-pool-workers/latest in tsconfig - Import startWorker directly instead of aliasing - Change worker.fetch() workaround comment to TODO
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: |
- Fix unused imports in dev-registry.test.ts - Fix RequestInfo type to Request | string | URL - Exclude vitest.config.mts from tsconfig type checking
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.
Summary
This PR migrates tests away from the deprecated
unstable_dev()API to use eitherunstable_startWorker()orvitest-pool-workers.fixtures/integration tests tounstable_startWorker()edge-preview-authenticated-proxytovitest-pool-workersmiddleware.test.tsandmiddleware.scheduled.test.tstounstable_startWorker()templates/init-tests/directory (dead code using old API)