test(e2e): Verify bundler plugins instrument a bundled graphql at runtime - #23670
Merged
Conversation
mydea
force-pushed
the
fn/bundler-graphql-runtime
branch
from
August 27, 2026 08:26
28dc52a to
36b76fb
Compare
mydea
force-pushed
the
fn/bundler-graphql-runtime
branch
from
August 27, 2026 08:26
36b76fb to
04d550b
Compare
mydea
force-pushed
the
fn/bundler-graphql-runtime
branch
from
August 27, 2026 08:29
04d550b to
0e12cc0
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0e12cc0. Configure here.
Contributor
size-limit report 📦
|
mydea
force-pushed
the
fn/bundler-graphql-runtime
branch
from
August 27, 2026 09:36
e5bf0b9 to
07f694a
Compare
mydea
marked this pull request as ready for review
August 27, 2026 11:37
isaacs
approved these changes
Aug 27, 2026
isaacs
left a comment
isaacs
left a comment
Member
There was a problem hiding this comment.
I think this test can be tightened a bit to more definitively cover what it claims to, but it's certainly a improvement in any event.
| output: { entryFileNames: 'main.mjs', format: 'es' }, | ||
| }, | ||
| }, | ||
| plugins, |
Member
There was a problem hiding this comment.
The graphql module would be external by default, so we need to explicitly tell it not to be, or else this test isn't testing what it claims.
Suggested change
| plugins, | |
| // the graphql module is external by default, we need to tell vite | |
| // to make it internal explicitly, so that this test does what it claims. | |
| ssr: external ? {} : { noExternal: ['graphql'] }, | |
| plugins, |
| .map(f => readFileSync(f, 'utf8')) | ||
| .join('\n') | ||
| .replace(/\/\*[\s\S]*?\*\//g, '') | ||
| .replace(/\s+/g, ''); |
Member
There was a problem hiding this comment.
The banner test was kind of irrelevant, and the whitespace stripping a bit brittle, so removing this is good. However, now nothing asserts the shape of the build anymore, which is presumably how graphql was not actually being inlined as expected.
This patch would add a check that it's being inlined or kept external as expected: https://gist.github.com/isaacs/2d9f123a663771811c04178f1089b7f9
Comment on lines
+38
to
+39
| console.log(`__RESULT__${JSON.stringify({ data, spans })}`); | ||
| process.exit(0); |
Member
There was a problem hiding this comment.
This can go flakey if stdout is a pipe, if the exit lands before write flushes, resulting in EPIPE.
Suggestion:
// up at the top of the file
import { writeSync } from 'node:fs';
Suggested change
| console.log(`__RESULT__${JSON.stringify({ data, spans })}`); | |
| process.exit(0); | |
| writeSync(1, `__RESULT__${JSON.stringify({ data, spans })}`); | |
| process.exit(0); |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| process.exit(0); | ||
| } | ||
|
|
||
| void main(); |
Contributor
There was a problem hiding this comment.
Bug: The test harness uses execFileSync without stdio: 'pipe', causing the actual error details from a crashed child process to be lost, hindering debugging.
Severity: MEDIUM
Suggested Fix
Add the stdio: 'pipe' option to the execFileSync call in bundler-instrumentation.ts. This will ensure that the standard output and standard error streams of the child process are captured and made available on the stdout and stderr properties of the error object in the catch block.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/e2e-tests/test-applications/node-rollup/src/entry.mjs#L48
Potential issue: The test harness in `bundler-instrumentation.ts` invokes a child
process using `execFileSync` but fails to configure it to capture the child's output
streams. The `catch` block attempts to read `err.stdout` and `err.stderr` from the error
object, but these properties will be `undefined` because the `stdio: 'pipe'` option is
missing. If the child process crashes due to an unhandled promise rejection (e.g., a
file write error), the test will fail with a generic message, and the actual error
details from the child process will be lost. This silent swallowing of the root cause
makes debugging test failures significantly more difficult.
Also affects:
dev-packages/e2e-tests/test-applications/node-esbuild/src/entry.mjs:48~48dev-packages/e2e-tests/test-applications/node-rolldown/src/entry.mjs:48~48dev-packages/e2e-tests/test-applications/node-vite/src/entry.mjs:48~48dev-packages/e2e-tests/test-applications/node-webpack/src/entry.mjs:48~48
Did we get this right? 👍 / 👎 to inform future reviews.
Move the "does this app need Playwright" decision into the install-playwright action: it resolves @playwright/test from the app and, when the package isn't there, emits an empty version and skips the browser install (and its cache steps) instead of failing on the missing require. Apps that assert via a plain node script (e.g. the node bundler apps) no longer pay for a Playwright install they don't use, with no per-app config or extra workflow step to maintain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nstrumentation Extends the node webpack/vite/rollup/rolldown/esbuild bundler apps from a static banner-grep into a runtime test: each app bundles a real `graphql` workload (inlined, only node builtins external) twice — `plain` (no plugin) and `plugin` (Sentry bundler plugin) — then runs both built bundles and asserts the query still returns data and that only the `plugin` build emits `auto.graphql.diagnostic_channel` spans. The entry disables `enableRuntimeChannelInjection` and runs without `--import`, so the bundler plugin is the only possible injector, making the `plain` build a true negative. Spans are captured via the `spanEnd` hook (transport/lifecycle-independent). The entry body is an async function (not top-level await) so it bundles to both ESM and esbuild's CJS node output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n set Adds two more variants per bundler app that keep graphql external and run the built bundle with `node --import @sentry/node/import`, so the runtime diagnostics-channel hook instruments graphql at load time (the inlined variants exercise the build-time transform instead). Each app now runs four scenarios: - plain (inlined, no plugin, no --import): no graphql spans (control) - plugin (inlined, plugin, no --import): one set, build-time - plain-external (external, no plugin, --import): one set, runtime hook - plugin-external (external, plugin, --import): one set, runtime hook only The assert defines "one set" relative to the build-time run and checks every instrumented scenario emits exactly that count — never zero, never double. The plugin-external + --import case in particular proves the build-time plugin and the runtime hook don't both instrument the same module (the plugin can't touch an external dep, so the runtime hook is the sole injector). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: isaacs <i@izs.me>
…t-utils
Move the duplicated per-app assert.mjs into a single, module-parameterized
`assertBundlerInstrumentation('graphql')` helper in @sentry-internal/test-utils,
collapsing each app's assert to one line. Along the way:
- Fix the Vite app to actually inline vs. externalize graphql via ssr.noExternal
/ ssr.external (rollupOptions.external is inert for Vite SSR builds), so the
"inlined" variants exercise the build-time path they claim to.
- Assert bundle shape (inlined vs external), scanning every emitted chunk since
bundlers split the entry's dynamic import (webpack) into sibling files.
- Hand the run result back through a file (SENTRY_E2E_RESULT_FILE) instead of
stdout, so a piped, buffered write can't be truncated by the child's exit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
fn/bundler-graphql-runtime
branch
from
August 28, 2026 09:02
f49ca59 to
8b67446
Compare
Member
Author
|
@isaacs I refactored this a bit according to your feedback. I moved this as a shared utility into test-utils so we do not need to repeat this all the time. Also added a check to verify that it actually properly inlines/externalizes the library etc. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Turns the five node bundler apps (
node-webpack,node-vite,node-rollup,node-rolldown,node-esbuild) from a static banner-grep into a real runtime test, in the spirit of orchestrion-treeshake-repro: actually run the bundled service against a realgraphqlpackage and verify what gets injected — across both the build-time (bundler plugin) and runtime (--importhook) instrumentation paths.Each app builds the same
graphqlworkload four ways and runs each bundle:plain— graphql inlined, no plugin, no--import: no graphql spans (negative control),plugin— graphql inlined, Sentry bundler plugin, no--import: one set of spans, via build-time injection,plain-external— graphql external,--import: one set, via the runtime hook,plugin-external— graphql external, plugin,--import: one set — the plugin can't touch an external module, so the runtime hook is the sole injector and there's no double instrumentation."One set" is defined relative to the build-time run, so the count stays correct across bundlers and graphql versions. This guards the fix beneath it (#23669) and catches the whole class of "bundle looks instrumented but emits no/double spans" regressions the static grep couldn't.
Shared assertion helper
The per-app
assert.mjswas ~86 identical lines copied five times. It now lives once in@sentry-internal/test-utilsasassertBundlerInstrumentation('graphql'), and each app'sassert.mjsis a single call. The helper is parameterized by a small fixture descriptor ({ moduleName, origin, sourceMarker, assertResult }) rather than hardcoded to graphql, so a future library is one registry entry plus asrc/app.mjsworkload — everything else (the four-variant matrix, the "one set" logic, the shape checks) is generic.entry.mjsis likewise library-agnostic: it runs a conventionalrunWorkload()and ships whatever it returns back asresult.Decisions
rollupOptions.external. A Vite SSR build externalizes deps by default, sorollupOptions.externalnever governed whether graphql was bundled — the "inlined" variants were silently shipping an external graphql and not exercising the build-time path at all. The variants now setssr.noExternal/ssr.externalexplicitly (the latter also wins over the plugin's own force-bundle, leaving graphql for the runtime hook).graphqlimport) or external (keeps the bare import, no inlined source). This scans every emitted chunk in the output dir, because bundlers split the entry'sawait import('./app.mjs')into a sibling chunk (webpack) where the marker lands rather than inmain.*.console.log(...)+process.exit()can truncate or EPIPE when stdout is a pipe (the exit lands before the buffered write drains). The entrywriteFileSyncs its result toSENTRY_E2E_RESULT_FILEand the helper reads it back; a boot crash or a missing file surfaces as an explicit failure instead of being swallowed.enableRuntimeChannelInjection: falseand the non---importbundles run with a plainnode, so the bundler plugin is the only possible injector — making theplainbuild a true negative rather than something the runtime hook could rescue.spanEndhook (+spanToJSON) rather thanbeforeSendSpan, so collection is independent of transport and trace lifecycle. Transport is a no-op and the DSN fake — nothing hits the network.require('node:async_hooks')that@sentry/server-utilsdoes once inlined (an esbuild CJS-in-ESM interop limit, the "separate bug" the repro noted); CJS output is the normal esbuild node target and sidesteps it. This also means esbuild resolves graphql's CJS build, so the suite covers both the.jsand.mjsorchestrion paths.new Set()formatting varies by bundler.