test: split Sandbox suites into mocked unit and e2e tiers (2/4) - #1743
test: split Sandbox suites into mocked unit and e2e tiers (2/4)#1743devin-ai-integration[bot] wants to merge 1 commit into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Package ArtifactsBuilt from 19e0113. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787315092-test-tiers-sandbox.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787315092-test-tiers-sandbox.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787315092.test.tiers.sandbox-py3-none-any.whl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review of the changed lines only (test tiering, no SDK surface changes). Checked: T-1/T-2 (JS ↔ sync/async Python parity of the new mirrored suites), T-4 (factories vs. internal constructors), T-11 (named aliases over primitives), T-15 (enums vs. bare literals), T-24/T-25 (format overloads), T-44/T-45/T-47 (timeout units and named defaults), T-55 (ENVD_* threshold constants), T-57/T-58/T-59 (error hierarchy and pre-flight validation).
3 violations worth fixing, all in the new unit-tier files:
readFormat.test.tsasserts the default username as a bare'user'literal where the SDK and the Python mirror both go through the named alias (T-11).- The Python version-gate mirrors pin
TemplateExceptionfor pre-flight argument validation onsandbox.fileswithout theTODOits JS mirror carries (T-59, T-1). - The three new JS unit files each re-encode the
@hiddenSandboxconstructor's internal wiring (T-4).
Not line-specific: the new suites do keep the sanctioned sync/async divergence of T-28 — test_version_gates.py calls filesystem.watch_dir(path, recursive=True) with no callback in sync and passes _on_event in async — which is correct, not a parity break. Nothing in the diff touches the public surface, so T-3/T-6/T-22/T-23 are not in play.
| '/home/user/hello.txt' | ||
| ) | ||
|
|
||
| assert.equal(lastQuery?.get('username'), 'user') |
There was a problem hiding this comment.
T-11 — when a concept has a named alias, use it instead of the primitive it aliases: the default system user is defaultUsername (src/connectionConfig.ts), which is exactly what the SDK path under test writes into the query (filesystem/index.ts → user = defaultUsername). The Python mirror already asserts against the alias (assert requests[0].url.params["username"] == default_username), so the bare literal is also the one place the two suites diverge (T-1).
| assert.equal(lastQuery?.get('username'), 'user') | |
| assert.equal(lastQuery?.get('username'), defaultUsername) |
Add the import alongside the existing src imports: import { defaultUsername } from '../../src/connectionConfig'.
| def test_write_rejects_metadata_on_old_envd(test_api_key): | ||
| filesystem = _filesystem(below_envd_version(ENVD_FILE_METADATA), test_api_key) | ||
|
|
||
| with pytest.raises(TemplateException, match="File metadata requires"): |
There was a problem hiding this comment.
T-59 — argument validation that runs before any operation starts belongs to InvalidArgumentException; a TemplateException here names a template build that never existed. The JS mirror pins the same wrong class but flags it (uploadMode.test.ts: // TODO: the gate should reject with InvalidArgumentError …); the Python mirrors silently bless it, so the two suites document different intent (T-1). Keep the assertion matching today's behavior, but carry the same note here and on the three watch_dir gates above:
| with pytest.raises(TemplateException, match="File metadata requires"): | |
| # TODO: the gate should raise InvalidArgumentException — this is argument | |
| # validation on `sandbox.files`, not a template build. | |
| with pytest.raises(TemplateException, match="File metadata requires"): |
| async def test_write_rejects_metadata_on_old_envd(test_api_key): | ||
| filesystem = _filesystem(below_envd_version(ENVD_FILE_METADATA), test_api_key) | ||
|
|
||
| with pytest.raises(TemplateException, match="File metadata requires"): |
There was a problem hiding this comment.
T-59 / T-1 — same as the sync mirror: this pins TemplateException for pre-flight argument validation on sandbox.files, which T-59 assigns to InvalidArgumentException. Mirror the JS TODO so all three surfaces record the same intent.
| with pytest.raises(TemplateException, match="File metadata requires"): | |
| # TODO: the gate should raise InvalidArgumentException — this is argument | |
| # validation on `sandbox.files`, not a template build. | |
| with pytest.raises(TemplateException, match="File metadata requires"): |
| return new Sandbox({ | ||
| ...config, | ||
| sandboxId, | ||
| sandboxDomain: 'sandbox.e2b.dev', | ||
| envdVersion, | ||
| envdAccessToken: 'token', |
There was a problem hiding this comment.
T-4 — Sandbox's constructor is internal wiring (@hidden/@internal; Sandbox.create/connect are the sanctioned entry points), yet this exact literal — sandboxId, sandboxDomain: 'sandbox.e2b.dev', envdVersion, envdAccessToken: 'token' — is now duplicated in three new files (versionGates.test.ts, readFormat.test.ts, uploadMode.test.ts). Any change to that internal shape breaks all three. The Python mirrors avoid it entirely by constructing the sub-namespace under test (Filesystem(...), Commands(...)).
Extract one helper (e.g. mockedSandbox(envdVersion?) in tests/setup.ts) and have the three files call it, so the internal constructor is referenced from a single place.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM aside from the inline nit — this is a mechanical test-tiering refactor with no production code changes.
Extended reasoning...
Overview
This PR (2/4 of a stack) only touches test files in packages/js-sdk/tests and packages/python-sdk/tests. It renames/moves files (e.g. sandbox/commands/commandHandle → sandbox/commandHandle, files/entryInfo → sandbox/entryInfo), switches gating helpers from test.skipIf(isDebug) to the new e2eTest/hostedTest/hostedSandboxTest tiers introduced in part 1/4, and adds new msw-mocked unit coverage (versionGates, readFormat, uploadMode) with JS/Python parity. No src/ production code is modified.
Security risks
None. No production logic, auth, crypto, or permission code is touched — the diff is confined to test files exercising the SDK against mocked or live sandboxes.
Level of scrutiny
Low-to-medium is appropriate: this is a large diff by line count but mechanical in nature (moves, renames, and gating swaps mostly done via consistent find/replace patterns). The one inline finding (lifecycleBehavior.test.ts using e2eTest instead of hostedTest) is a real gating gap, but it only manifests under the narrow E2B_DEBUG=1 E2B_E2E=1 developer combination and does not affect the standard e2e CI run or the default unit tier. I verified the setup.ts gating helpers (e2eTest/hostedTest/hostedSandboxTest) match the described semantics, confirming the finding's mechanics.
Other factors
The new mocked tests (versionGates, readFormat, uploadMode) are duplicated correctly between JS and the sync/async Python SDKs per the repo's cross-SDK parity convention. Two TODOs are called out explicitly in the PR description for known behavior mismatches (TemplateError vs InvalidArgumentError), which is appropriate for pinning current behavior rather than silently masking it. Given the single nit-level finding and the test-only scope, this does not need to block on human review.
| import { assert } from 'vitest' | ||
|
|
||
| import { Sandbox } from '../../src' | ||
| import { e2eTest, template, wait } from '../setup' |
There was a problem hiding this comment.
🟡 All three tests in lifecycleBehavior.test.ts are gated with plain e2eTest instead of hostedTest, so under E2B_DEBUG=1 E2B_E2E=1 they'd run against a local envd (which cannot auto-pause, resume via traffic proxy, or snapshot) instead of being skipped. Sibling suites migrated in this PR (connect, snapshot, host, network) correctly use the hosted variant, and the Python mirror of these same three scenarios in test_create.py keeps @pytest.mark.skip_debug(). Fix: swap e2eTest for hostedTest in the three tests here.
Extended reasoning...
What the bug is: packages/js-sdk/tests/sandbox/lifecycleBehavior.test.ts imports and uses e2eTest from ../setup for all three of its tests. Per tests/setup.ts, e2eTest = base.skipIf(!isE2E) — it only skips when E2B_E2E is unset, with no awareness of debug mode. hostedTest = e2eTest.skipIf(isDebug) additionally skips when E2B_DEBUG is set. tests/README.md documents this contract explicitly: use the hosted variants "when a local envd can'''t stand in for the real thing (control plane, traffic proxy, snapshots), which additionally skip under E2B_DEBUG," and separately clarifies that E2B_DEBUG is an independent axis that does not itself gate a tier — so E2B_E2E=1 E2B_DEBUG=1 is a supported, real combination that this file does not handle correctly.\n\nWhy it matters here specifically: all three tests in this file need exactly the capabilities a local envd cannot provide: timeout-triggered auto-pause + getInfo().state === '\''paused'\'', a filesystem-only auto-pause snapshot that cold-boots on connect(), and auto-resume-on-traffic via sandbox.getHost(8000) going through the real traffic proxy. Sandbox.pause()/betaPause() (src/sandbox/index.ts:659-668) has no debug-mode short-circuit the way kill()/connect()/setTimeout() do, so under the E2B_DEBUG=1 E2B_E2E=1 combination these calls would hit the real control-plane API against a sandbox that is actually a local envd process — not skip, and not behave as asserted.\n\nWhy existing code doesn'''t prevent it: this is purely a copy/gating oversight in the new file. Every sibling suite touched in this same PR that needs the hosted control plane (connect.test.ts, snapshot.test.ts, host.test.ts, network.test.ts) was correctly migrated to hostedTest/hostedSandboxTest. lifecycleBehavior.test.ts is new in this PR (split out of lifecyclePayload.test.ts), and the split apparently didn'''t carry the debug-skip semantics over — confirmed by the Python mirror of these exact three scenarios (test_auto_pause_without_auto_resume_requires_connect, test_auto_pause_filesystem_only_reboots, test_auto_resume_wakes_on_http_request in both test_create.py files) which correctly keep @pytest.mark.skip_debug().\n\nProof, step by step:\n1. Run with E2B_E2E=1 E2B_DEBUG=1 (both set, a supported combination per tests/README.md).\n2. isE2E is true and isDebug is true.\n3. e2eTest = base.skipIf(!isE2E) → skipIf(false) → the test runs.\n4. hostedTest = e2eTest.skipIf(isDebug) → skipIf(true) → would have skipped, but this file doesn'''t use it.\n5. Sandbox.create(template, { lifecycle: { onTimeout: '\''pause'\'', autoResume: false } }) runs against a local envd instead of the hosted control plane.\n6. After the wait, sandbox.getInfo().state is asserted to equal '\''paused'\'' — but a local envd sandbox never transitions to a control-plane '\''paused'\'' state on timeout, so the assertion fails (or the underlying pause()/API calls throw, since pause() has no debug short-circuit).\n\nFix: change the import in lifecycleBehavior.test.ts from e2eTest to hostedTest and swap all three usages.\n\nSeverity: nit — the failure only manifests in the narrow E2B_DEBUG=1 E2B_E2E=1 developer combination. The default unit-test tier is unaffected, and the standard e2e CI run (without debug) passes fine, so nothing breaks at merge time. It'''s a one-word-per-usage fix worth making for tiering consistency with the rest of this PR.
1 participant
Summary
2/4 of the stack replacing #1739 (base: #1742). Moves the Sandbox suites onto the two tiers added in 1/4 and backfills mocked coverage for the client-side logic that was previously only exercised against a live sandbox.
Gating
Behavioral suites (
commands/**,files/**,pty/**,git/**, lifecycle/connectivity,tests/api/**) switch fromtest.skipIf(isDebug)toe2eTest/hostedTest/hostedSandboxTest. Tests that need the hosted control plane (list/info/kill/snapshot, host, network, metrics, signing) keep theisDebugskip on top of the e2e opt-in, since a local envd can't stand in for the control plane, proxy or snapshots.Mixed files split
Pure helper suites moved out of the behavioral directories so they stay in the default tier, and mocked assertions living inside behavioral files were extracted:
New mocked coverage
All msw/canned-response based, asserting on the request the SDK sends and how it handles a fixed response — no VM:
sandbox/versionGates—commands.run({ stdin: false })belowENVD_COMMANDS_STDIN;filesystem.watchDirrecursive/includeEntry/allowNetworkMountsgates. The accepted path is served bysetupServerso it never reaches a real sandbox URL. Thresholds come fromsrc/envd/versionsviabelowEnvdVersion()instead of hardcoded numbers.sandbox/readFormat—text/bytes/blob/streamswitch, default user, gzip handling, empty body, 404 →FileNotFoundError.sandbox/uploadMode— multipart vs. octet-stream decision, old-envd fallback, streams, gzip, metadata, multi-file request shape.test_version_gates,test_read_format,test_upload_mode.Two assertions carry a
TODO: the SDK currently throwsTemplateErrorwhereInvalidArgumentErroris the right contract, so the tests pin today's behavior rather than the intended one.Link to Devin session: https://app.devin.ai/sessions/e237ae2e3d8043fbabca7017400e6e57
Requested by: @mishushakov