Repository navigation
test(e2e): Add Bun, Deno and Cloudflare variants to react-router-8-framework - #24598
JPeer264 merged 1 commit into
Conversation
Sorry, something went wrong.
size-limit report 📦
|
Sorry, something went wrong.
isaacs
left a comment
There was a problem hiding this comment.
Making the name load-bearing feels like it might be a bit hazardous, but it's definitely not broken now. Could either fix with this PR, or make a follow-up issue.
Sorry, something went wrong.
| - name: Set up Bun | ||
| if: | ||
| matrix.test-application == 'node-exports-test-app' || contains(matrix.test-application, 'bun') || | ||
| contains(matrix.label, 'bun') |
There was a problem hiding this comment.
This would also match strings like turbopack bundler.
We could harden that a bit by pinning the test to the end of the string, matching our conventions:
| contains(matrix.label, 'bun') | |
| endsWith(matrix.label, '(bun)') |
Perhaps worth doing the same for the other contains(matrix.label, 'bun') calls further up the file as well?
Sorry, something went wrong.
There was a problem hiding this comment.
Or, actually, if we do set matrix.runtime, then could just use that instead of keying on the name at all.
Sorry, something went wrong.
There was a problem hiding this comment.
That got removed in the combination of this: #24598 (comment)
Sorry, something went wrong.
| const RUNTIMES = ['bun', 'deno', 'cloudflare']; | ||
|
|
||
| /** Returns the runtime that a variant label like `my-app (cloudflare)` ends with, or `undefined`. */ | ||
| export function getRuntimeFromLabel(label: string | undefined): string | undefined { |
There was a problem hiding this comment.
This will turn on file replacement for tests that didn't opt in, which is not a problem today, but might be a surprise in the future.
getRuntimeFromLabel reads the runtime from the label text. Labels that already end in (bun), (deno) or (cloudflare) exist today: hono-4, hono-4-legacy, gen-ai-libraries (cloudflare). Those apps use the same entry.<runtime>.ts / instrument.<runtime>.ts file names, but their scripts pick the files directly. Nothing breaks today only because they have no src/entry.ts or src/instrument.ts (The base files are entry.node.ts / instrument.node.ts). If someone adds hono-4/src/instrument.ts later, the bun/deno variants will silently overwrite it with the runtime file in CI only, and a local in-folder run will act differently.
Also, the label is free text for display. Changing a label (for example if we changed something to react-router-8-framework (Cloudflare, local workerd)) would silently turn the replacement off, and the variant would build with the Node vite.config.ts.
Suggestion: make this an explicit variant field, for example { "runtime": "cloudflare", ... }. getTestMatrix.mjs already spreads variant fields into the matrix include, so CI can pass ${{ matrix.runtime }} to ci:copy-to-temp, and run.ts can read matchedVariant.runtime. The same field could also set RUNTIME for the assert step, which would remove the need for the test:assert:<runtime> scripts and the volta run workaround in the README.
Can definitely be a follow-up, though, if you'd rather put it off for now.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah that is true. I like yours more actually to explicitly set the runtime, rather than taking it from the description. Could have been bad if there would be a node framework called bunny.js 😅 🐰
Sorry, something went wrong.
| // the initial pageload to `/performance` gets 301-redirected to a trailing slash by react-router-serve | ||
| 'url.path': { value: '/performance/', type: 'string' }, | ||
| // the initial pageload to `/performance` gets 301-redirected to a trailing slash by react-router-serve, workerd does not | ||
| 'url.path': { value: RUNTIME === 'cloudflare' ? '/performance' : '/performance/', type: 'string' }, |
There was a problem hiding this comment.
This ternary is repeated a few times. Could it maybe be put in a single place and re-used?
I'm thinking if there's another in the future RUNTIME that needs to behave like cloudflare, it'd be a change in one place.
Could actually put PERFORMANCE_PATH and SERVER_SDK_NAME (and maybe SERVER_PLATFORM) in tests/constants.ts next to RUNTIME. Then build the regex from the path. Each future runtime would then need one edit and not one edit per test.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah actually a trailing slash worked too on Cloudflare. I moved it back to a trailing slash URL and this ternary is gone now
Sorry, something went wrong.
…amework Runs the same Playwright suite on Bun, Deno and Cloudflare (local workerd) as optional variants of the existing app, instead of one app per runtime. Each runtime inits its own SDK (`@sentry/bun`, `@sentry/deno`, `@sentry/cloudflare`), and the tests read the runtime with the new `getRuntime()` from `@sentry-internal/test-utils`. A variant declares its runtime with a `runtime` field. From it, CI installs Bun or Deno, the runner and CI set `RUNTIME` for the build and the assert command, and the runner copies the files named for that runtime over their base files in the temporary copy of the app, for example `entry.server.cloudflare.tsx` over `entry.server.tsx` and `vite.cloudflare.config.ts` over `vite.config.ts`. So a runtime variant needs no framework or bundler config. The Bun and Deno variants of hono-4 and hono-4-legacy now set `runtime` too, because CI no longer reads the runtime from the label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POC for running one framework e2e app on several server runtimes instead of one app per runtime (Linear project P-JS-2537). react-router-8-framework now runs its full Playwright suite on Bun, Deno and Cloudflare (local workerd) as optional variants, next to the Node job.
Each runtime inits its own SDK, the way a user of that runtime would: Node @sentry/react-router, Bun @sentry/bun, Deno @sentry/deno (with --preload=@sentry/deno/import) and Cloudflare @sentry/cloudflare. @sentry/react-router then only provides the framework wrappers, so values from its init() (sdk.name, the runtime tag, the /__manifest filter) are Node-only, and the tests branch on that. Using the runtime SDKs is what makes the Bun variant work: @sentry/node creates no http.server span on Bun, @sentry/bun does.
A variant declares its runtime with a runtime field instead of the label. From it, both e2e jobs install Bun or Deno, the runner and CI set RUNTIME for the build and the assert command (read with the new getRuntime() from @sentry-internal/test-utils, and playwright.config.mjs picks the start command from it), and the runner copies the files named for that runtime over their base files in the temporary copy of the app. So the Cloudflare build gets vite.cloudflare.config.ts (@cloudflare/vite-plugin + sentryCloudflareVitePlugin) and entry.server.cloudflare.tsx, while Bun and Deno reuse the Node build. React Router has no option to pick a server entry, and this way a variant needs no framework or bundler config and no build.yml change. The Bun and Deno variants of hono-4 and hono-4-legacy set runtime too, because CI no longer reads the runtime from the label. The convention is in the e2e README.
Other expected differences: Express is not instrumented under bun run and there is no Express layer on workerd, so there the error transaction stays the request path and the meta tag names the http.server span. Deno is pinned to v2.9.0, because Deno 2.8 loses async context in socket callbacks and drops ioredis spans.
🤖 Generated with Claude Code