Repository navigation
test(e2e): Add Bun, Deno and Cloudflare variants to react-router-8-framework - #24598
Conversation
size-limit report 📦
|
f08b065 to
9297a36
Compare
JPeer264
changed the title
d358a6c to
4dd044c
Compare
JPeer264
changed the title
adb5cb1 to
fb48ca5
Compare
51e4c78 to
b94af9d
Compare
b94af9d to
6e3b225
Compare
6e3b225 to
c2638e4
Compare
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
c2638e4 to
90bb70c
Compare
5af7f99 to
c5ae31e
Compare
2704c9c to
72f6d6c
Compare
d0fbc38 to
8aefac0
Compare
b789b78 to
57c28ca
Compare
isaacs
left a comment
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.
| - 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?
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.
There was a problem hiding this comment.
That got removed in the combination of this: #24598 (comment)
| 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.
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 😅 🐰
| // 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.
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
…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>
2 participants
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-frameworknow 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-routerthen only provides the framework wrappers, so values from itsinit()(sdk.name, theruntimetag, the/__manifestfilter) are Node-only, and the tests branch on that. Using the runtime SDKs is what makes the Bun variant work:@sentry/nodecreates nohttp.serverspan on Bun,@sentry/bundoes.A variant declares its runtime with a
runtimefield instead of the label. From it, both e2e jobs install Bun or Deno, the runner and CI setRUNTIMEfor the build and the assert command (read with the newgetRuntime()from@sentry-internal/test-utils, andplaywright.config.mjspicks 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 getsvite.cloudflare.config.ts(@cloudflare/vite-plugin+sentryCloudflareVitePlugin) andentry.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 nobuild.ymlchange. The Bun and Deno variants ofhono-4andhono-4-legacysetruntimetoo, 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 runand there is no Express layer on workerd, so there the error transaction stays the request path and the meta tag names thehttp.serverspan. Deno is pinned to v2.9.0, because Deno 2.8 loses async context in socket callbacks and dropsioredisspans.🤖 Generated with Claude Code