test: app router instrumentation with cloudflare plugin#333
Conversation
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped PR. The new cloudflare-dev Playwright project correctly runs vite dev (not wrangler dev) against the app-router-cloudflare example, which exercises configureServer() with @cloudflare/vite-plugin loaded — a code path that wasn't covered by the existing cloudflare-workers project (which only tests the build+wrangler production path).
A few observations:
Looks good:
- The
testMatchpattern mirrors thecloudflare-workersproject structure: cloudflare-specific tests in a dedicated directory plus the sharedinstrumentation.spec.ts. - Port 4178 avoids conflicts with all existing projects (4173-4177).
- The 30s timeout is appropriate for a dev server (no build step).
- CI matrix entry is correct.
Minor note (non-blocking):
- The
tests/e2e/cloudflare-dev/directory doesn't exist yet — the glob for**/cloudflare-dev/**/*.spec.tswill match nothing today, so the project currently only runsinstrumentation.spec.ts. That's fine as a starting point; presumably cloudflare-dev-specific tests will be added in follow-up PRs.
LGTM — this adds meaningful coverage for the dev-mode Cloudflare path with minimal blast radius.
|
Approved PR #333. Here's a summary of the review: Verdict: Approved This is a clean, well-scoped addition. The new The setup is correct: port 4178 avoids conflicts, the 30s timeout is appropriate for a dev server (no build step), and the CI matrix entry is properly added. The One minor note: |
runs certain tests against a vite dev instance with the cloudflare plugin