test(js-sdk): run the full unit test suite in a browser - #1609
test(js-sdk): run the full unit test suite in a browser#1609mishushakov wants to merge 13 commits into
Conversation
PR SummaryMedium Risk Overview
Tests were adjusted for the browser host: CORS-enabled sandbox HTTP helpers, Web Crypto and Reviewed by Cursor Bugbot for commit 2abaa12. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 95f82f0. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.48.1-browser-test-suite.0.tgzCLI ( npm install ./e2b-cli-2.18.3-browser-test-suite.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-browser-test-suite.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-browser-test-suite.0.tgzPython SDK ( pip install ./e2b-2.48.0+browser.test.suite-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.0+browser.test.suite-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.5.0+browser.test.suite-py3-none-any.whl |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5626cfff60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
LGTM — test-only PR that expands the browser CI leg to the full unit suite, with no production code changes.
Extended reasoning...
Overview
This PR touches only test infrastructure: the browser vitest config, test files (replacing Node-only APIs like Buffer/node:crypto with WebCrypto/TextEncoder), the CI workflow matrix, package.json devDependencies (dropping React test tooling), and pnpm-lock.yaml. No files under packages/js-sdk/src/ are modified.
Security risks
None. No auth, crypto business logic, or user-facing behavior changes — only test harness and CI configuration.
Level of scrutiny
Low-to-moderate scrutiny is appropriate: this is a test-only change with no production code path affected. I spot-checked the PR's own claims against src/: CommandExitError is indeed exported (ProcessExitError never existed, confirming the described vacuous-assertion bug fix), and the browser-specific skip flags (canFetchSandboxServers, canReadPaginationToken, canObserveStoppedSandbox) correctly mirror existing runtime !== 'browser' CORS-avoidance logic already present in src/connectionConfig.ts. The new capability-gated skips are a reasonable way to surface browser limitations without silently dropping coverage.
Other factors
The bug hunting system found no issues. The PR description is thorough and self-documents the rationale for each test change (duplicate test removal, flaky mock cleanup, Node-API replacements). No changeset is needed since this is test-only, consistent with prior similar PRs (#1600) per CLAUDE.md guidance.
🦋 Changeset detectedLatest commit: 2abaa12 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
bf5d7cf to
5ca9d31
Compare
Promotes the browser leg from a single React smoke test to the full unit +
connectionConfig suite running inside a real headless Chromium via
@vitest/browser + Playwright (`pnpm test:browser`) — the same coverage
test:bun / test:deno / test:cf get. 69 files / 343 tests green.
The React smoke test and its devDependencies are gone (react, react-dom,
@types/react{,-dom}, @testing-library/react, @vitejs/plugin-react,
vitest-browser-react). The one thing it uniquely covered — the SDK driven with
no `process` global at all, as in a real browser bundle — is kept as
tests/runtimes/browser/noProcessGlobal.test.ts. Chromium installation moves
from the default `pretest` hook to `pretest:browser`, so plain `pnpm test` no
longer downloads a browser.
Test bugs the leg surfaced:
- commands/kill and pty/kill asserted `rejects.toThrowError(ProcessExitError)`,
but `ProcessExitError` is not exported from src. Vite's SSR transform
resolved the named import to `undefined`, so both assertions were vacuous;
real ESM in the browser makes it a hard SyntaxError. Now CommandExitError.
- files/read.test.ts had three byte-identical duplicated tests, each
provisioning a second sandbox on every runtime.
- api/http2 and envd/http2 carried dead `vi.doUnmock('undici')` /
`vi.doUnmock('../../src/utils')` calls from an old refactor. The bare
'undici' one made Vite pre-bundle a Node HTTP client mid-run and reload the
page under the running suite — a cold-cache-only flake that broke whichever
file was being collected at the time.
Node-only test APIs are replaced with cross-runtime equivalents: node:crypto
randomUUID/createHash to WebCrypto, Buffer to TextEncoder/TextDecoder,
path.basename inlined.
Limitations a browser physically can't work around are gated by documented
capability flags in tests/setup.ts, so they report as skipped instead of
disappearing into the config's exclude list. Two are real user-facing bugs,
filed as SDK-293 (x-next-token is not in Access-Control-Expose-Headers, so
Sandbox.list pagination silently truncates) and SDK-294 (the stopped-sandbox
502 has no CORS headers, so isRunning() throws instead of returning false).
Closes SDK-292
Co-Authored-By: Claude <noreply@anthropic.com>
These tests drive the SDK and never render anything, so vitest's browser.screenshotFailures (on by default when headless) wrote a PNG of a blank page per failed test into .vitest-attachments/. Turned off, which also lets the .gitignore entry it needed go away. Removes canReadPaginationToken: the API not exposing X-Next-Token via Access-Control-Expose-Headers is fixed upstream in infra#3388, so the six limit-based pagination tests are un-gated and run in the browser like everywhere else. Note infra#3388 is not on production yet, and the browser leg only ever runs against production (staging callers pass node-only, which drops it), so those six tests fail on `paginator.hasNext` until the deploy lands. Co-Authored-By: Claude <noreply@anthropic.com>
Removes canObserveStoppedSandbox. The stopped-sandbox 502 arriving from the edge without CORS headers is fixed in e2b-dev/runtime#3389, so isRunning() can report false in a browser again and checkSandboxHealth can return false, which restores the actionable TimeoutError on the 24 call sites that use it. Also un-gates host.test.ts 'ping server in non-running sandbox', which was under the wrong flag: it asserts the edge's own 502 JSON envelope rather than a response from a server inside the sandbox, so infra#3389 covers it too (the same PR's content-negotiation fix is what keeps the body JSON rather than the HTML error page). Its sibling 'ping server in running sandbox' does read the user's python server, so that one keeps the gate. canFetchSandboxServers stays for the four tests that read a response from a server the test started in the sandbox — no CORS headers there, and one of them sends a custom header that would preflight into python's 501. Its doc comment no longer cites the proxy 502 as an example, since that now carries the header. Until infra#3389 reaches production these 14 tests fail on the browser leg — 13 with 'TypeError: Failed to fetch' and one with the degraded SandboxError in place of TimeoutError, matching the two documented consequences exactly. The Node/Bun/Deno/workerd legs are unaffected (7 files / 26 tests green on Node). Co-Authored-By: Claude <noreply@anthropic.com>
- gate the http-server pause/resume test on canFetchSandboxServers: it fetches a `python -m http.server` the test starts itself, which sends no CORS headers. Masked today by the isRunning failure, it would have started failing the moment infra#3389 rolls out. - import the SDK dynamically in noProcessGlobal, after the process shim is deleted. A static import is evaluated at collection time while the shim is still installed, so a top-level `process` read anywhere in the module graph would have passed here and still crashed a browser app on import. Co-Authored-By: Claude <noreply@anthropic.com>
…browser gate `python -m http.server` sends no CORS headers, so a browser handed the test an opaque `TypeError: Failed to fetch` instead of the response. That was gated behind `canFetchSandboxServers` as inherent, but it isn't: the server is ours, and a real browser app's own server opts in the same way. `corsHttpServerCmd` starts one that does, so the five tests run in Chromium too and the last capability flag is gone. Verified in Chromium against a local instance of the exact generated command: cross-origin GET readable, and a custom-header request survives the preflight via do_OPTIONS. Negative control against a plain `python -m http.server` reproduces the `Failed to fetch` the browser leg was showing. Co-Authored-By: Claude <noreply@anthropic.com>
… was written Six new suites import msw's `setupServer`, whose `msw/node` entry pulls in node:http and can't be served to a browser. Also drops ENABLE_VOLUME_TESTS from the forwarded env — main removed that flag when it mocked the volume tests. Co-Authored-By: Claude <noreply@anthropic.com>
Buffering a ReadableStream went through `new Response(stream)`, whose accepted body types differ per runtime: undici takes any async iterable, a browser takes only its own stream class and stringifies the rest. So a browser uploading a stream from a polyfill or another realm silently sent the 23-byte string "[object ReadableStream]" instead of the file. Draining through the reader — the one part every implementation shares — is correct everywhere and drops the runtime-dependent guess. Note the obvious fix (skip the async-iterable branch of `toDispatchableStream` in the browser) is wrong: it re-wraps a native stream when `globalThis.ReadableStream` has been replaced by a polyfill, which is what that branch exists to prevent. Found by the browser test leg via `toUploadBody leaves an async-iterable foreign stream alone`, which main added while this branch was in review. Co-Authored-By: Claude <noreply@anthropic.com>
ec0622a to
c8387f1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c8387f1. Configure here.
When a sandbox is killed while a request is in flight the SDK probes the sandbox's health to tell that apart from a transient network failure, and returns an actionable TimeoutError when the probe confirms the sandbox is gone. The probe is gated on the connection-dropped message, whose wording is runtime-specific — and the browser's, `network error`, was missing, so killing a sandbox mid-command surfaced a generic `SandboxError: [unknown] network error`. The fragment is broad, which is fine by construction: matching only earns a health probe, and the error stays generic unless the probe confirms the sandbox is gone. A browser reports every failed request identically on purpose, so this is the only wording available. Found by the browser test leg, which attributed this to the missing CORS headers on proxy-synthesized responses. It wasn't that: belt#2068 rolled out to production and the other 13 failures went green, this one didn't. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
infra#3389 was closed unmerged; the fix that landed is belt#2068. Note also which paths are still awaiting the orchestrator rollout, since that is what keeps `sandbox requires traffic access token` red on the browser leg. Comment only — the test stays un-gated deliberately, so the leg goes green on its own once the rollout completes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c8387f1 to
3288bcd
Compare
Resolutions: - packages/js-sdk/package.json: keep main's `catalog:` specifiers, drop the react/testing-library devDeps this branch no longer needs (it replaced the React smoke test with the full suite). Lockfile regenerated. - tests/sandbox/pty/kill.test.ts: main's polling assertion supersedes this branch's error-name fix. - tests/sandbox/lifecyclePayload.test.ts: main's vi.waitFor helpers, with the in-sandbox server still started via corsHttpServerCmd so the browser can read the response. - tests/sandbox/network.test.ts: main rewrote the maskRequestHost test to assert a 200 through the proxy, so its inline python server now sends Access-Control-Allow-Origin. - js_sdk_tests.yml / browser vitest config: honor main's E2B_TEST_MAX_WORKERS on the browser leg and raise max-parallel to cover the seventh job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dev-dependency refresh (#1790) re-added the react/testing-library devDeps this branch removed with the React browser smoke test; kept them out and took the wrangler bump. Lockfile regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolutions: - main hoisted this branch's local `waitForHttpStatus` into tests/setup.ts, so the four import conflicts (host, lifecyclePayload, network, snapshot) now pull it from there alongside `corsHttpServerCmd`, and snapshot.test.ts drops its local copy. - The new httpsPorts test (#1546) reads the body of a response from an inline in-sandbox HTTPS server, so that server now sends Access-Control-Allow-Origin like the other in-sandbox servers. - tests/sandbox/onResumeRequest.test.ts (#1800) imports msw/node, so it joins the browser suite's exclude list. - Lockfile regenerated: main still carries the react devDeps for the React browser smoke test this branch replaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Note
Both upstream blockers have rolled out — the browser leg is green, with nothing gated or skipped for being a browser.
infra#33886Sandbox.list({ limit })pagination testsbelt#206813 stopped-sandbox testsbelt#2068 on the traffic-token pathsandbox requires traffic access tokenThe 15th failure was not an infra problem and is fixed in this PR (
1964dde) — see the browser's connection-drop wording below.pnpm test:browseragainst production, 2026-09-09: 77 files (76 passed, 1 skipped), 424 tests (420 passed, 4 skipped), 0 failing, 143s. The last gap — the proxy answering a traffic-token403and its preflight without CORS headers — is closed on the orchestrator side, sosandbox requires traffic access tokennow reads the403from Chromium as intended. It was deliberately left un-gated rather than skipped, so the leg went green on its own the moment the rollout completed. The Node suite is green on the same tree (--project unit --project connectionConfig: 88 files, 614 passed, 1 skipped). Staging Node legs may still fail repo-wide for reasons unrelated to this branch (staging template builder returningtemplate builder not found, staging auto-pause losing sandboxes).What
Promotes the browser leg from a single React smoke test to the full unit + connectionConfig suite running inside a real headless Chromium (
@vitest/browser+ Playwright) — the same coveragetest:bun/test:deno/test:cfget. 77 files, 76 passing against production, nothing red. The one skipped file istests/runtime.test.ts, whose two tests assert Node/Workers runtime detection offprocess.releaseand so have nothing to assert on a browser host.pnpm test:browser # installs Chromium via pretest:browser, then runs the suiteReact is gone as requested — the smoke test (
tests/runtimes/browser/run.test.tsx) and its seven devDependencies (react,react-dom,@types/react,@types/react-dom,@testing-library/react,@vitejs/plugin-react,vitest-browser-react). Chromium installation moves off the defaultpretesthook ontopretest:browser, so plainpnpm testno longer downloads a browser.The one thing the smoke test uniquely covered — the SDK driven with no
processglobal at all, the way a real browser bundle runs — is kept as a dedicated test, since the rest of the suite runs against aprocess.envshim that would mask a regression there:Failure screenshots are off (
browser.screenshotFailures: false): these tests never render anything, so vitest's default would write a PNG of a blank page per failed test into.vitest-attachments/.Test bugs the leg surfaced
Two assertions were vacuous.
commands/killandpty/killassertedrejects.toThrowError(ProcessExitError)— butProcessExitErrorhas never been exported fromsrc. Vite's SSR transform resolves the missing named import toundefined, sotoThrowError(undefined)asserted nothing on any runtime; real ESM in the browser turns it into a hardSyntaxError. NowCommandExitError.files/read.test.tsran three tests twice — a byte-identical duplicated block, provisioning a second real sandbox for each on every runtime leg.A cold-cache-only flake, root-caused.
api/http2andenvd/http2carried deadvi.doUnmock('undici')/vi.doUnmock('../../src/utils')calls from an old refactor (neither file mocks anything any more). The bare'undici'one made Vite discover and pre-bundle a Node HTTP client mid-run, reloading the browser page under the running suite and breaking whichever file was being collected. Verified: 1-in-4 failure before, three cold-cache runs green after.Node-only test APIs are replaced with cross-runtime equivalents:
node:cryptorandomUUID/createHash→ WebCrypto,Buffer→TextEncoder/TextDecoder,path.basename→ inline.Two real browser bugs found
Both verified over the wire; neither was fixable in the SDK alone.
Pagination silently truncated — the cursor lives in the
x-next-tokenresponse header, which the API didn't name inAccess-Control-Expose-Headers, so a browser withheld it and every page looked like the last:Fixed upstream in infra#3388, rolled out 2026-07-24 — the six tests are un-gated here and pass. SDK-293.
SDK-294 —
isRunning()threw instead of returningfalse. A stopped sandbox's 502 came from the edge without CORS headers, so the probe rejected opaquely (running: 204 + ACAO: */paused: 502 + no ACAO). Because a browser always preflights the SDK's envd requests (they carryE2b-Sandbox-Id/E2b-Sandbox-Port) and a preflight needs an ok status, adding the header to the 502 alone wasn't enough — theOPTIONSpath needed a 2xx too. Fixed upstream in belt#2068 and confirmed on production:isRunning()now returnsfalsefor a stopped sandbox in the browser, and the 13 tests that depended on it pass.The kill-mid-request error was a separate, SDK-side bug — originally filed under SDK-294 as another symptom of the missing CORS headers. It isn't: belt#2068 rolled out, the other 13 went green, and
killing the sandbox while a command is running throws an actionable errorstill failed withSandboxError: 2: [unknown] network error. When the connection drops mid-request the SDK probes the sandbox's health to tell a killed sandbox from a transient blip, but the probe is gated on the connection-dropped message, whose wording is runtime-specific — and the browser's was missing:So the probe never ran and the error stayed generic. The fragment is broad, which is fine by construction: matching only earns a health probe, and the error stays generic unless the probe confirms the sandbox is gone. A browser reports every failed request identically on purpose, so this is the only wording available. Both entry points (
handleRpcErrorWithHealthCheckfor RPC,handleEnvdApiFetchErrorfor the envd HTTP API) share the predicate, so one edit covers both; the per-runtime tables inhandleRpcError.test.tsandhandleEnvdApiError.test.tsgain aBrowserrow. No Python counterpart — it detects this by exception type, not message wording, and has no browser runtime.belt#2068 also fixes the content negotiation the investigation turned up: the proxy chose HTML over JSON by User-Agent sniffing, and browser
fetchcan't override its UA, so an SDK call from a browser got the HTML error page where it expects JSON (Acceptwas ignored). That is un-gated here too.How limitations are handled
No capability flags — nothing is skipped to avoid a browser failure. Four tests do skip, each on an assertion that does not exist in a browser rather than one that fails there: the two
tests/runtime.test.tsdetection tests above;sandbox_url defaults to stable sandbox host in production, which has a browser counterpart asserting the opposite (the stable host is deliberately not used there); andan adopted foreign stream forwards cancellation to its source, because a browser cannot stream a request body at all, so the SDK buffers instead of adopting the stream — the sibling assertionexpect(streamed).toBe(streams)covers that positively. The one still-failing test is left red on purpose, waiting on the upstream rollout rather than gated around; see the callout at the top. The five tests that read a response from a server the test itself starts in the sandbox now start a CORS-enabled one, viacorsHttpServerCmdintests/setup.ts:This was originally gated behind a
canFetchSandboxServersflag as an inherent browser limitation. It isn't one: the server is the test's own, and a real browser app's server opts into CORS exactly the same way — so the gate was hiding coverage ofgetHostand the proxy path rather than documenting a wall.Access-Control-Allow-Headersplusdo_OPTIONSalso covers the traffic-access-token test, whose custom header the browser preflights.Confirmed on the browser leg: all five tests this un-gates pass —
ping server in running sandbox,sandbox works without token,auto-resume wakes paused sandbox on http request,pause and resume a sandbox with http server(that one needed belt#2068, since it callsisRunning()on a paused sandbox), andsandbox requires traffic access token, which reads a proxy-synthesized403after a preflight that can't carry the token — the last piece of the belt#2068 rollout. That's the CORS server working end-to-end through the proxy against production.Two in-sandbox servers are written inline by their own tests rather than started from
corsHttpServerCmd— themaskRequestHostheader-capturing server and the self-signed HTTPS backend behindhttpsPorts— and both sendAccess-Control-Allow-Originfor the same reason.The
excludelist is only for genuinely Node-only suites (tests/bundle/**readsdistvianode:fs,tests/undici.test.tsresolves packages offprocess.versions.node) plus the ten suites that mock the API withmsw/node, whose entry pulls innode:httpand can't be served to a browser. Porting those needssetupWorkerplus a service worker from a public dir — tracked as a follow-up on SDK-292, and worth doing since cancellation is the most runtime-divergent part of this SDK.A real bug the leg caught
mainaddedtoUploadBody leaves an async-iterable foreign stream alonewhile this branch was in review, and it failed in Chromium withexpected '[object ReadableStream]' to be 'hello'. Not a test artifact — the mechanism, confirmed directly:toDispatchableStreamtreats async-iterability as "the platform will accept this", which is an undici extension rather than web behavior. So buffering a stream vianew Response(stream)in a browser stringified it, and this silently uploaded 23 bytes of text instead of the file:The fix drains the stream through its reader — the one part every implementation shares — instead of guessing what the platform accepts. Note the obvious fix is wrong: skipping the async-iterable branch in the browser re-wraps a native stream when
globalThis.ReadableStreamhas been replaced by a polyfill, which is precisely what that branch exists to prevent (there's a test for it). Verified in Chromium and Node.CI
New
browsermatrix leg onubuntu-22.04, with the Playwright cache moved onto it (no other leg needs Chromium any more). The leg is dropped bynode-onlystaging callers, and it's the only leg whose result depends on the API's CORS headers — the matrix comment now says so, rather than claiming the non-Node legs add no backend signal.Notes
toBlobstream drain (below) and the browser connection-drop wording (above). Everything else is tests and CI.mainby merging (four times so far; earlier revisions were rebased). What conflicted each time and how it was resolved: Playwright moved into aplaywright:installscript, sopretest:browserdelegates to it; newmsw/nodesuites are added to the browser suite'sexcludelist as they land (ten now);packages/js-sdk/package.jsonconflicts against every dev-dep refresh, sincemainstill carries the seven React devDeps this branch drops — resolved to main's versions andcatalog:specifiers minus those;pnpm-lock.yamlis always reset to main's and regenerated withpnpm install --lockfile-onlyrather than hand-merged, so its only delta from main is the React tree;main's rewrites of the shared live tests (polling assertions,vi.waitForhelpers, and hoisting this branch's localwaitForHttpStatusintotests/setup.ts) are taken wholesale, keeping only thecorsHttpServerCmdcall sites on top. Note the Playwright bump to ^1.62 invalidates a local Chromium cache —pnpm run playwright:install; CI is unaffected.vi.stubEnvanddelete process.env.Xkeep working in the browser because the shim aliasesprocess.envtoimport.meta.env, which is where vitest puts configenvand wherestubEnvwrites. VerifiedE2B_DEBUG=1 pnpm test:browsercorrectly skips the debug-gated tests.python -m http.serverreproduces theTypeError: Failed to fetchthe browser leg was showing, so the probe is meaningful rather than vacuous. Also checked there's exactly oneAccess-Control-Allow-Originheader — a duplicate would make browsers reject the response.noProcessGlobal.test.tsimports the SDK dynamically inside the test body — a static import is evaluated at collection time, while theprocessshim is still installed, so it only covered the call path and not module evaluation. (An earlier revision also put thecanFetchSandboxServersgate onpause and resume a sandbox with http server; that gate was deleted wholesale later in the branch when the tests moved tocorsHttpServerCmd.)ProcessExitErrorones this fixes).tests/api/list.test.tsduplicates five tests byte-for-byte (only the threebetaPauseones intentionally differ, instance vs static), andfiles/write.test.ts:130repeatswrite fileas a strict subset of the test at:6— together ~7 redundant sandboxes per run per leg. Left alone rather than deleting tests outside this change's scope.Closes SDK-292
🤖 Generated with Claude Code