test: split SDK suites into a mocked unit tier and an opt-in e2e tier - #1739
test: split SDK suites into a mocked unit tier and an opt-in e2e tier#1739devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 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:
|
🦋 Changeset detectedLatest commit: 8e72370 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Package ArtifactsBuilt from 11f56bd. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787256556-test-tiers.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787256556-test-tiers.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787256556.test.tiers-py3-none-any.whl |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98bb6bd5c9
ℹ️ 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".
| "test:e2e": "E2B_E2E=1 vitest run --project e2e", | ||
| "test:browser": "E2B_E2E=1 vitest run --project browser", |
There was a problem hiding this comment.
In the inspected JS SDK workflow, the e2e matrix includes windows-latest, but this package script uses POSIX assignment syntax; the CLI adds the same script while its workflow also targets Windows. Package scripts use cmd.exe by default on Windows, as described in the npm scripts documentation, so these jobs fail before Vitest starts with E2B_E2E treated as a command. Set the variable in the workflow env block or use a cross-platform helper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The flag is no longer set in the package script: JS sets it in the e2e project's env in vitest.config.mts, and the CLI got a vitest.e2e.config.mts that merges the base config with env: { E2B_E2E: '1' }. test:e2e/test:browser are now plain vitest run invocations, so they work under cmd.exe.
| query: { metadata: { sandboxTestId } }, | ||
| }) | ||
| const sandboxes = await paginator.nextItems() | ||
| sandboxTest('list sandboxes', async ({ sandbox, sandboxTestId }) => { |
There was a problem hiding this comment.
When E2B_DEBUG=true, Sandbox.create returns the fixed local debug_sandbox_id without provisioning through the control plane, but this test now proceeds to Sandbox.list(), which still queries the hosted control-plane API and cannot find that local sandbox. The previous skipIf(isDebug) prevented this failure; restore it for control-plane tests (as both sync and async Python tests still do) so E2B_E2E=1 pnpm test:e2e remains usable against local envd.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. tests/setup.ts now exports hostedTest/hostedSandboxTest (e2eTest/sandboxTest plus skipIf(isDebug)), and every control-plane test uses them — api/{list,info,kill,snapshot}, plus the lifecycle/connectivity/snapshot/metrics/network files. e2eBuildTemplateTest skips under debug too, since builds are always server-side. E2B_E2E=1 pnpm test:e2e against local envd is usable again.
![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 unit/e2e test split. Checked the rules the diff can actually touch: enum vocabulary (T-15, T-41), envd version gating (T-55), error domains and specificity (T-42, T-57, T-58, T-59, T-60), named defaults (T-47), and JS/sync-Python/async-Python parity of the new mocked mirrors (T-1, T-2).
The tiering itself reads clean: the opt-in flag follows the E2B_ env-var convention (T-49), the e2e glob list has a single home, and the new mocked suites are mirrored across JS, sync Python and async Python.
6 violations, all in newly added test code:
- 2 x bare
'RUN'/'WORKDIR'/'COPY'instruction literals instead ofInstructionType(T-15, T-41) — JS and the Python mirror. - inline envd version strings at the call site instead of the
ENVD_*threshold constants (T-55), which also drifted the JS/Python pair apart (0.2.4vs0.2.9for the same gate, T-1). - a 404 assertion on the
NotFoundErrorbase rather thanFileNotFoundError(T-58, T-60), diverging from the Python mirror which does assertFileNotFoundException. - two new assertions that pin down error classes TASTE forbids:
TemplateError/TemplateExceptionfor asandbox.filesargument gate (T-57, T-59) and a bareError/ValueErrorfor a builder precondition (T-42). Both are pre-existing SDK behavior, so the fix belongs in the SDK, not here — but freezing them in a new test makes them contractual, so they should at least be marked. - a magic
'e2b.app'default domain in the shared CLI test setup (T-47), which also disagrees with thee2b.devdefault the CLI and SDK use.
Not tied to a line: template_sync/test_serialization.py has no template_async mirror. T-2 wants sync and async as separate mirrors, but the hashing/serialization utilities under test are genuinely shared and non-async, so the single copy is fine — worth a one-line note in tests/README.md so the next person doesn't read it as an oversight. Same for the deliberate non-gating of create_lifecycle.test.ts, which the PR description explains but the test file doesn't.
| assert.isUndefined(payload.fromTemplate) | ||
| assert.deepEqual( | ||
| payload.steps.map((step: { type: string }) => step.type), | ||
| ['RUN', 'WORKDIR'] |
There was a problem hiding this comment.
T-41 / T-15 — every site that filters or dispatches on instruction.type goes through InstructionType; a bare 'RUN' / 'WORKDIR' / 'COPY' literal is never a legitimate read of the enum. That is exactly what makes a typo a compile error instead of a silently-passing (or silently-vacuous) assertion — steps.find((step) => step.type === 'COPY') on line 102 returns undefined on a typo and assert.isUndefined(copyStep(...)) still passes.
Add import { InstructionType } from '../../src/template/types' and use the members here and on line 102.
| ['RUN', 'WORKDIR'] | |
| [InstructionType.RUN, InstructionType.WORKDIR] |
There was a problem hiding this comment.
Fixed — the suite imports InstructionType from src/template/types and both the step-order assertion and the COPY lookup compare against enum members, so a typo is a compile error.
| assert payload["startCmd"] == "python main.py" | ||
| assert payload["readyCmd"] == "curl -f http://localhost:8000" | ||
| assert payload.get("fromTemplate") is None | ||
| assert [step["type"] for step in payload["steps"]] == ["RUN", "WORKDIR"] |
There was a problem hiding this comment.
T-41 / T-15 — same as the JS mirror: reads of instruction.type go through InstructionType, never a bare "RUN" / "WORKDIR" / "COPY" literal (line 102 too). InstructionType is a str Enum, so comparing it against the serialized payload works unchanged.
Add from e2b.template.types import InstructionType.
| assert [step["type"] for step in payload["steps"]] == ["RUN", "WORKDIR"] | |
| assert [step["type"] for step in payload["steps"]] == [ | |
| InstructionType.RUN, | |
| InstructionType.WORKDIR, | |
| ] |
There was a problem hiding this comment.
Fixed — same change in the Python mirror: from e2b.template.types import InstructionType, used for the step-order assertion and the COPY lookup (it's a str Enum, so the payload comparison is unchanged).
|
|
||
| describe('commands', () => { | ||
| test('rejects stdin:false below ENVD_COMMANDS_STDIN', async () => { | ||
| const sandbox = sandboxWithEnvd('0.2.4') |
There was a problem hiding this comment.
T-55 — envd-version-dependent behavior is expressed through the named ENVD_* threshold constants in envd/versions, never an inline version string at the call site. These new suites hardcode a version per gate ('0.2.4', '0.1.3', '0.6.2', '0.6.3', '0.6.1', '0.5.6') plus a '0.6.4' default in readFormat/uploadMode; the test name references ENVD_COMMANDS_STDIN but nothing ties the literal to it. Bump a constant and the tests keep passing while no longer exercising the gate — and the '0.6.4' default silently sits below the next threshold added.
Derive the versions from the constants instead, e.g. import ENVD_COMMANDS_STDIN and pin the below-threshold and at-threshold cases relative to it (a small belowVersion(ENVD_COMMANDS_STDIN) helper in tests/setup.ts, and ENVD_VERSION_WATCH_NETWORK_MOUNTS as the "supported envd" default).
Related T-1: the JS mirror uses '0.2.4' and the Python one "0.2.9" for the same gate — a symptom of the literals not having a single source.
There was a problem hiding this comment.
Fixed. Both suites now import the ENVD_* thresholds (ENVD_COMMANDS_STDIN, ENVD_FILE_METADATA, ENVD_OCTET_STREAM_UPLOAD, ENVD_DEFAULT_USER, ENVD_VERSION_RECURSIVE_WATCH, ENVD_VERSION_FS_EVENT_ENTRY_INFO, ENVD_VERSION_WATCH_NETWORK_MOUNTS) and derive the reject-branch version from the threshold instead of hardcoding one: belowEnvdVersion(v) in tests/setup.ts (a -0 prerelease) and below_envd_version(v) in tests/envd_versions.py (an rc1 prerelease). Supported-path defaults use ENVD_DEBUG_FALLBACK, so a new threshold can't leave them silently below the gate, and the JS/Python divergence is gone.
| await expect(files.read('/home/user/missing.txt')).rejects.toThrowError( | ||
| NotFoundError | ||
| ) | ||
| await expect( | ||
| files.read('/home/user/missing.txt', { format: 'stream' }) | ||
| ).rejects.toThrowError(NotFoundError) |
There was a problem hiding this comment.
T-58 / T-60 — prefer specific over generic: assert FileNotFoundError, not the shared NotFoundError base. 404 on a file op is defined to land on FileNotFoundError (envd/api.ts maps 404 and Code.NotFound to it), so the base-class assertion passes even if the mapping regresses to a SandboxNotFoundError — and it disagrees with the Python mirror, which does assert FileNotFoundException (T-1).
Update the import on line 6 to FileNotFoundError and the test name on line 128 accordingly.
| await expect(files.read('/home/user/missing.txt')).rejects.toThrowError( | |
| NotFoundError | |
| ) | |
| await expect( | |
| files.read('/home/user/missing.txt', { format: 'stream' }) | |
| ).rejects.toThrowError(NotFoundError) | |
| await expect(files.read('/home/user/missing.txt')).rejects.toThrowError( | |
| FileNotFoundError | |
| ) | |
| await expect( | |
| files.read('/home/user/missing.txt', { format: 'stream' }) | |
| ).rejects.toThrowError(FileNotFoundError) |
There was a problem hiding this comment.
Fixed — the 404 case asserts FileNotFoundError, not the base NotFoundError.
| sandbox('0.6.1').files.write('/home/user/hello.txt', 'hello world', { | ||
| metadata: { origin: 'unit-test' }, | ||
| }) | ||
| ).rejects.toThrowError(TemplateError) |
There was a problem hiding this comment.
T-59 / T-57 — this is argument validation that runs before any operation starts (no request leaves the SDK), so the contract is InvalidArgumentError; TemplateError names a template build that never existed, and a sandbox.files failure should extend SandboxError in any case. Same for the watchDir gates in versionGates.test.ts (lines 49/57/65) and the Python TemplateException mirrors.
The throw itself is pre-existing SDK behavior, so don't change it in a test-only PR — but a new assertion turns it into a documented contract. Either assert on the message and leave the class unpinned, or keep the class with a // TODO(T-59): should be InvalidArgumentError marker so the eventual fix is findable.
There was a problem hiding this comment.
The assertion now names the class the gate actually throws and carries a TODO that this contract is wrong: version-gating an argument on sandbox.files should reject with InvalidArgumentError, not TemplateError. Left as a follow-up rather than changing SDK behavior in a test-only PR.
| }) | ||
|
|
||
| test('hashing a source that matches no file fails', async () => { | ||
| await expect(filesHash('nope.txt', '/app/')).rejects.toThrow() |
There was a problem hiding this comment.
T-42 — builder precondition failures raise BuildError / BuildException, never a bare Error or ValueError. calculateFilesHash currently throws a bare Error (and calculate_files_hash a bare ValueError, which the Python mirror asserts on line 51), and an untyped .rejects.toThrow() locks that in: it would keep passing after the SDK is fixed, and it documents no contract today.
Same call as above — the SDK fix is out of scope for a test-only PR, so mark it rather than pinning the wrong class:
| await expect(filesHash('nope.txt', '/app/')).rejects.toThrow() | |
| // TODO(T-42): should reject with BuildError once calculateFilesHash stops throwing a bare Error. | |
| await expect(filesHash('nope.txt', '/app/')).rejects.toThrow() |
There was a problem hiding this comment.
Same treatment: the missing-source case asserts only that it rejects, with a TODO that it should be a TemplateError once calculateFilesHash stops throwing a bare Error (ValueError in Python). Freezing the wrong class in a test would make the fix look like a regression.
| process.env.E2B_DOMAIN || | ||
| userConfig?.E2B_DOMAIN || | ||
| userConfig?.domain || | ||
| 'e2b.app' |
There was a problem hiding this comment.
T-47 — defaults live in named constants, not magic values at the call site. This one also disagrees with the default everywhere else: packages/cli/src/user.ts and the SDK's ConnectionConfig use e2b.dev, so an e2e run with no E2B_DOMAIN and no ~/.e2b/config.json points the CLI at a different domain than the SDK it drives.
The literal moved here from the two test files rather than being introduced, but promoting it to shared setup is the moment to name it — import the SDK's default domain, or at minimum:
| 'e2b.app' | |
| DEFAULT_E2E_DOMAIN |
(with const DEFAULT_E2E_DOMAIN = 'e2b.dev' above, matching T-49's explicit-option → env-var → default chain.)
There was a problem hiding this comment.
Fixed — tests/setup.ts has a named DEFAULT_E2E_DOMAIN = 'e2b.dev' used as the last fallback after E2B_DOMAIN and the user config.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // The gates pass, so the call proceeds to the RPC and fails on the network | ||
| // instead — the point is that it is not a TemplateError. | ||
| const sandbox = sandboxWithEnvd('0.6.4') | ||
|
|
||
| await sandbox.files | ||
| .watchDir('/home/user', noop, { | ||
| recursive: true, | ||
| includeEntry: true, | ||
| allowNetworkMounts: true, | ||
| requestTimeoutMs: 1_000, | ||
| }) | ||
| .then( | ||
| () => assert.fail('expected the request to fail without a sandbox'), | ||
| (err: Error) => assert.notInstanceOf(err, TemplateError) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🟡 In versionGates.test.ts (lines 68-84), the "accepts the gated options on a supported envd" test uses envd 0.6.4, which passes all three client-side version gates and lets files.watchDir() proceed into a real RPC call to https://49983-sbx-version-gate.sandbox.e2b.dev — actual production e2b.dev infrastructure — instead of stopping at the mocked boundary like its sibling files (readFormat.test.ts, uploadMode.test.ts).
Extended reasoning...
This file is not in tests/e2eFiles.mts, so it runs in both the default unit vitest project (pnpm test) and the Cloudflare workerd project (pnpm test:cf) — the two tiers this PR's own description advertises as fully mocked, deterministic, and requiring "no VM provisioning, no credentials" (even calling out running with E2B_API_KEY unset). Every other test in this same file passes version gates that fail client-side, so the request never leaves the process; this one test is the sole exception where the gates pass and execution falls through to this.rpc.watchDir(...) and await handleWatchDirStartEvent(events) in src/sandbox/filesystem/index.ts, which actually opens a connect-rpc stream.
Unlike readFormat.test.ts and uploadMode.test.ts, which mock the same kind of sandbox-pointed request with an msw setupServer, versionGates.test.ts has no msw server and no handler for this domain at all. tests/globalFetchFallback.setup.ts (the only setup file for this project) just reroutes the SDK onto globalThis.fetch so msw could intercept it if a server were running — it does not mock any response itself. With no server listening, the request escapes straight to the real network and hits sandbox.e2b.dev with a nonexistent sandbox ID on every default pnpm test and pnpm test:cf run.
Concretely: envd 0.6.4 clears ENVD_VERSION_RECURSIVE_WATCH, ENVD_VERSION_FS_EVENT_ENTRY_INFO, and ENVD_VERSION_WATCH_NETWORK_MOUNTS, so recursive: true, includeEntry: true, and allowNetworkMounts: true all pass validation → watchDir calls this.rpc.watchDir(...) → the connect-rpc transport (built from envdRpcFetch) issues an HTTPS request to 49983-sbx-version-gate.sandbox.e2b.dev → the request either times out after requestTimeoutMs: 1_000, DNS-fails, or gets a real 4xx/5xx from e2b.dev's edge → the test only asserts err not instanceof TemplateError, which is true for every one of those outcomes, so the test passes regardless.
That last point is also why this doesn't break CI today: no matter what the network does, the assertion holds. But it means the test's "pass" is not actually verifying client-side behavior at all — it's a no-op assertion riding on an unintended live network call, contradicting the fully-mocked/deterministic tier boundary this PR is establishing everywhere else in the same file and its sibling files.
Fix: give this test the same treatment as readFormat.test.ts/uploadMode.test.ts — add an msw handler for https://49983-sbx-version-gate.sandbox.e2b.dev/... that returns a canned response (or an error), so the assertion actually exercises the SDK's error-mapping instead of live network variance. Alternatively, since the real intent here appears to be "prove the gates don't reject," the test could stop right after confirming the gate check passes rather than letting the call proceed to the RPC layer.
There was a problem hiding this comment.
Fixed — the accepted-path test now runs against an msw setupServer that answers the envd RPC, so it asserts the SDK sent the request instead of reaching a real sandbox URL and passing on the resulting failure.
| "async_sandbox_factory", | ||
| "build", | ||
| "async_build", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| def pytest_collection_modifyitems(items): | ||
| for item in items: | ||
| if isinstance(item, pytest.Function) and not E2E_FIXTURES.isdisjoint( | ||
| item.fixturenames | ||
| ): | ||
| item.add_marker(pytest.mark.e2e) |
There was a problem hiding this comment.
🟡 The fixture-name heuristic in pytest_collection_modifyitems (conftest.py:60-65) auto-marks any test using the build/async_build fixture as e2e, but it doesn't account for tests that fully mock that fixture. tests/sync/template_sync/test_stacktrace.py and tests/async/template_async/test_stacktrace.py each declare 27 tests (54 total) on build/async_build, yet both files have an autouse mock_template_build fixture that monkeypatches the request/upload/status calls so Template.build() never hits the network. As a result, all 54 fully-mocked stacktrace-mapping tests get silently excluded from the default uv run pytest run (via addopts = -m "not e2e") and only execute under the opt-in pytest -m e2e tier, losing default per-PR coverage of that logic with no warning.
Extended reasoning...
The bug: pytest_collection_modifyitems in packages/python-sdk/tests/conftest.py (lines 53-65) marks a test as e2e purely based on whether one of its requested fixture names is in E2E_FIXTURES = {"sandbox", "sandbox_factory", "async_sandbox", "async_sandbox_factory", "build", "async_build"}. This is a reasonable heuristic for most of the suite, since build/async_build normally drive a real server-side template build. But pytest.ini's new addopts = --import-mode=importlib -m "not e2e" means anything tagged e2e is silently dropped from the default uv run pytest run that gates every PR.
Where it breaks: tests/sync/template_sync/test_stacktrace.py and its async mirror tests/async/template_async/test_stacktrace.py each declare the build/async_build fixture as a parameter on 27 test functions (54 total). Both files also define an autouse fixture, mock_template_build, that monkeypatches request_build, trigger_build, get_file_upload_link, and get_build_status on template_(sync|async)_main/build_api_mod. Because that fixture is autouse, it patches these calls for every test in the file before the build/async_build fixture actually invokes Template.build(...), so the "build" never leaves the process — no network call, no credentials needed, no live infra touched. These are pure client-side tests of the stacktrace step-index mapping logic.
Why the existing code doesn't catch this: the hook only inspects item.fixturenames, which lists every fixture a test transitively depends on by name, with no way to know whether the fixture it's keying on was overridden or wrapped by another (autouse) fixture that neutralizes its side effects. E2E_FIXTURES treats build/async_build as an unconditional live-call signal, which is true everywhere else in the suite but false here.
Impact: all 54 stacktrace-mapping tests move from the required default tier into the opt-in pytest -m e2e tier, which needs E2B_API_KEY and typically only runs on the e2e label or manual dispatch. This directly undercuts the PR's own stated goal — "client-side logic … stays in the default tier" — for a meaningful chunk of tests, and it happens silently: nothing fails, SDK Tests stays green, and the gap is easy to miss since the tests still pass when someone does run the e2e tier.
Step-by-step proof:
- A test like
test_stacktrace.py::test_maps_step_index_for_run_cmd(build)requests thebuildfixture. pytest_collection_modifyitemssees"build" in item.fixturenamesand callsitem.add_marker(pytest.mark.e2e).pytest.ini'saddopts = -m "not e2e"deselects it from a plainuv run pytestinvocation.- But
mock_template_build(autouse in the same file) has already patchedrequest_build/trigger_build/get_file_upload_link/get_build_status, so when thebuildfixture callsTemplate.build(...), it resolves entirely against the mocks — no network I/O, noE2B_API_KEYrequirement. - Net effect: a fully mocked, credential-free test is excluded from the tier explicitly designed to hold "fully mocked, credential-free" tests, and only runs when someone opts into
-m e2e.
Suggested fix: either mark these two files with an explicit override (e.g. pytestmark = [pytest.mark.skip_debug()] plus something like a local fixture-name exemption), or have the heuristic check whether mock_template_build (or an equivalent) is also present in item.fixturenames and skip the auto-e2e marking in that case. A sweep: pattern could generalize this: any file with an autouse mock_*_build fixture alongside a build/async_build parameter should be exempted.
There was a problem hiding this comment.
Fixed — pytest.ini declares a mocked marker and pytest_collection_modifyitems skips auto-marking anything carrying it. Both template_{sync,async}/test_stacktrace.py set pytestmark = pytest.mark.mocked next to the autouse fixture that mocks every build call, so those 54 tests stay in the default tier despite requesting the build fixture.
| assert.deepEqual(readSignatureExpected, readSignatureReceived) | ||
| }) | ||
|
|
||
| test.skipIf(isDebug)('signing generation with expiration', async () => { | ||
| e2eTest('signing generation with expiration', async () => { | ||
| const operation = 'read' | ||
| const path = '/home/user/hello.txt' | ||
| const user = 'root' |
There was a problem hiding this comment.
🟡 packages/js-sdk/tests/sandbox/secure.test.ts is matched by the e2eFiles glob ('secure' in the sandbox list), so the whole file — including three tests that only call the pure client-side getSignature() helper ('signing generation', 'signing generation with expiration', 'static signing key comparison') — now runs only under pnpm test:e2e with credentials instead of the default pnpm test tier they ran in before this PR. Per the PR's own split criterion (client-side logic stays default) and the pattern it already applies for fork.test.ts/connect.test.ts, these three should be extracted into a mocked companion file (e.g. secureSignature.test.ts) alongside forkPayload.test.ts and connectDebug.test.ts.
Extended reasoning...
What the bug is
getSignature() in src/sandbox/signature.ts is a pure, client-side function: it builds a string from path/operation/user/envdAccessToken/expiration and returns a local sha256-derived signature. It never opens a socket, calls envd, or touches a sandbox.
tests/sandbox/secure.test.ts has three top-level tests that exercise only this function:
signing generation(originally line 33)signing generation with expiration(originally line 60)static signing key comparison(originally line 92)
Each one calls getSignature() directly and asserts the result against an independently-computed (or hardcoded) hash string — no sandboxTest fixture, no network call, no envd round-trip.
The code path that triggers it
Before this PR, these three tests used test.skipIf(isDebug), so they ran unconditionally in the default pnpm test run (skipped only under E2B_DEBUG). This PR converts them to e2eTest (base.skipIf(!isE2E)) and adds secure.test.ts to the e2eFiles glob in tests/e2eFiles.mts (tests/sandbox/{...,secure,...}.test.ts). That glob is consumed by vitest.config.mts as the exclude list for the unit and template projects and as the include list for the new e2e project. Because file-level exclude applies to the whole file, not per-test, all three of these pure-logic tests are removed from the projects that back pnpm test and only run under pnpm test:e2e (needs E2B_E2E=1 plus credentials).
Why existing code does not prevent it
The rest of secure.test.ts (the describe('secure sandbox', ...) block) genuinely needs a live sandbox — it writes files and fetches signed download URLs — so moving the file to e2e reads as correct at a glance. Nothing in the PR's tooling flags that a subset of tests in an e2e-tagged file do not actually use the e2e fixtures; the mixed-file problem is exactly what the PR solves elsewhere (see below), just missed here.
Impact
Three assertions that used to run on every pnpm test invocation (including required CI) now silently stop being part of that signal. They still exist and still pass under pnpm test:e2e, so nothing is deleted or broken at runtime — this is a coverage/organization regression, not a functional bug. A change to getSignature()'s hashing scheme would no longer be caught by the default/required test tier.
How to fix it
Follow the same extraction pattern the PR already applies to other mixed files: pull the three pure getSignature() tests out of secure.test.ts into a new file (e.g. tests/sandbox/secureSignature.test.ts) that is not matched by the e2eFiles glob, using a plain test(...) (or test.skipIf(isDebug)) instead of e2eTest. This mirrors fork.test.ts -> forkPayload.test.ts and connect.test.ts -> connectDebug.test.ts in this same diff.
Step-by-step proof
- Check the glob:
tests/e2eFiles.mtsline ~18 is'tests/sandbox/{...,secure,...}.test.ts', which matchestests/sandbox/secure.test.ts. - Check
vitest.config.mts: theunitproject'sexcludespreads...e2eFiles, and thetemplate/cloudflare configs do too — sosecure.test.tsis excluded from every projectpnpm testruns (--project unit --project template --project connectionConfig). - Check the
e2eproject: itsincludeise2eFiles, sosecure.test.ts(the whole file) is only picked up bypnpm test:e2e. - Inside that file,
signing generation,signing generation with expiration, andstatic signing key comparisoneach call onlygetSignature(...)from../../srcandassert.deepEqual/assert.equalagainst a value computed withnode:crypto— nosandboxfixture parameter, nofetch, nosandboxTest. - Therefore: running
pnpm testtoday executes zero of these three assertions, whereas before this PR (withtest.skipIf(isDebug)) they ran every timeE2B_DEBUGwas unset — i.e., in virtually all CI runs.
There was a problem hiding this comment.
Fixed — the pure getSignature tests moved to tests/sandbox/secureSignature.test.ts, which stays in the unit tier. secure.test.ts keeps only the live download/reconnect assertions.
|
check all comments |
|
any volume tests we should make e2e? right now it's all mocked |
|
attach the PR to the linear issue |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
One: real mount content. Everything the existing volume suites assert — CRUD request shaping, error mapping, pagination, the content transport — is client-side, so mocking it is right and they stay in the default tier. What no mock covers is that a mounted volume is actually the same storage in two sandboxes, so I added that:
Note the e2e mount test provisions two sandboxes and hasn't been run against live infra yet — I don't have approval to spend on the e2e tier. Say the word and I'll run |
Done — the description now ends with |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
1 participant
Summary
Every SDK package now has two test tiers: a fully mocked default tier and an opt-in e2e tier. Before this,
pnpm test/pytestprovisioned sandboxes and built templates, so the "unit" run needed credentials, live infra and minutes; the only gate wasE2B_DEBUG, which is a different axis (local envd vs. provisioned sandbox).The split follows what an assertion actually verifies: client-side logic (payload shaping, config propagation, version gating, format switching, error mapping, pagination, URL construction, pure utilities) stays in the default tier; anything crossing the RPC boundary into envd or the control plane (process execution, filesystem round-trips, PTY, git in the VM, lifecycle against live infra, server-side builds) is e2e.
Result — no VM provisioning, no credentials, deterministic:
JS SDK
The e2e file list lives in one place,
tests/e2eFiles.mts, and is consumed byvitest.config.mts(as thee2eproject'sincludeand theunit/templateprojects'exclude) and by the Cloudflare workerd config'sexclude. Adding a behavioral test means adding one glob.The default script names projects explicitly rather than using
--project !e2e, which pulls inbrowserand fails without a Playwright binary.browseris e2e (it provisions a sandbox from a browser bundle) and moved out of the default run.Mixed files were split so client-side assertions stay in the default tier: pure helper suites moved out of the behavioral directories (
commands/commandHandle→sandbox/commandHandle,files/entryInfo,files/watchHandle,git/validation→sandbox/gitValidation), and mocked assertions living inside behavioral files were extracted intosandbox/connectDebug,sandbox/forkPayload,sandbox/lifecycleBehavior(the e2e remainder) andtemplate/tagsBuild.New mocked coverage for logic that was previously only exercised e2e — all msw/canned-response based, asserting on the request the SDK sends:
sandbox/versionGates—commands.run({ stdin: false })belowENVD_COMMANDS_STDIN;filesystem.watchDirrecursive/includeEntry/allowNetworkMountsgates.sandbox/readFormat—text/bytes/blob/streamswitch, default user, gzip header, empty body, 404 →NotFoundError.sandbox/uploadMode— multipart vs. octet-stream decision, old-envd fallback, streams, gzip, metadata, multi-file request shape.template/serialization— file-hash stability and content/path sensitivity, missing source, Dockerfile/template payload serialization, registry config, COPY hash inclusion.Python SDK
Marking is automatic rather than per-file, so a new test can't silently land in the default tier:
The handful of tests that hit live APIs without one of those fixtures (API kill of a non-existing sandbox, MCP gateway creation, template
exists, background build) got an explicit@pytest.mark.e2e. Sync and async mocked mirrors of the JS extractions were added (test_version_gates,test_read_format,test_upload_mode, plustemplate_sync/test_serialization— the hashing/serialization utilities are shared, so it isn't duplicated for async).CLI
The credential/skip logic duplicated in two files is now shared in
tests/setup.tsand keyed on the opt-in:exec_pipe.test.tsandbackend_integration.test.tsreuse it.create_lifecycle.test.tsdeliberately does not: it is avi.mock-based suite asserting on the options the CLI passes toSandbox.create, so gating it behind credentials would remove default coverage rather than add it.CI
SDK Tests(required, every PR) runs the unit tier only. The reusable workflows gained ane2e: booleaninput that switches the run topnpm test:e2e/pytest -m e2e/ CLItest:e2e; a new opt-inSDK E2E Testsworkflow calls them withe2e: trueon manual dispatch (optionally against staging) or when a PR carries thee2elabel. Playwright install steps only run in the e2e leg.Note: the staging legs of
SDK Testsnow re-run the mocked tier against the staging domain, so per-PR backend-compatibility signal moves to the e2e workflow. Happy to make the staging legs e2e-on-PR instead if that signal should stay automatic.Docs:
tests/README.mdin each package (which tier a test belongs in, how to run each) and a table inCONTRIBUTING.md.Usage
Review follow-ups
Second commit, in response to review:
E2B_E2E=1moved out of the package scripts (POSIX-only, breaks the Windows CI legs) into thee2eVitest project'senvand a newpackages/cli/vitest.e2e.config.mts.hostedTest/hostedSandboxTestrestore theskipIf(isDebug)skip for control-plane tests, soE2B_E2E=1 pnpm test:e2estill works against a local envd.ENVD_*constants plus abelowEnvdVersion/below_envd_versionhelper (a prerelease of the threshold) instead of hardcoded literals, and the accepted-path test runs against msw rather than a real sandbox URL.InstructionTypemembers; the 404 case assertsFileNotFoundError; the two wrong-error-class assertions carry TODOs instead of freezing the wrong contract.@pytest.mark.mockedopts a test out of fixture-based e2e marking — the template stacktrace suites mock every build call, so their 54 tests stay in the default tier.getSignaturetests extracted tosandbox/secureSignature.test.ts; CLI e2e domain falls back to a namedDEFAULT_E2E_DOMAIN = 'e2b.dev'.volume/mountPayload.test.ts+tests/shared/volume/test_mount_payload.py(unit) covervolumeMountsrequest shaping, andvolume/mount.test.ts+ sync/asynctest_mount.py(e2e) assert a mounted volume is the same storage in two sandboxes — the one volume behavior no mock can cover.Fixes SDK-136.
Link to Devin session: https://app.devin.ai/sessions/e237ae2e3d8043fbabca7017400e6e57
Requested by: @mishushakov