test: add Volume mount payload unit and live mount e2e coverage (3/4) - #1744
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 82bfaa2. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787315092-test-tiers-volume.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787315092-test-tiers-volume.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787315092.test.tiers.volume-py3-none-any.whl |
There was a problem hiding this comment.
TASTE.md review — test-only PR, so I checked the rules that bind on how tests exercise the public surface: parity across JS / sync Python / async Python (T-1, T-2), factory-vs-constructor acquisition (T-4), the positional/optional argument rule (T-3), option naming (T-10), and the timeout/config rules (T-44…T-50, nothing relevant here). The tier split itself and the msw / monkeypatch shapes match the convention the base PR established, and the e2e tier correctly holds only the one behavior a mock can't assert.
3 findings, all about the Volume-instance mount source:
- JS payload test builds a
Volumethrough its constructor (T-4). - Python does the same, and positionally, unlike every existing Volume test (T-4, T-3).
- The async payload mirror covers only one of the three sync cases (T-1, T-2).
Not tied to a changed line:
- The PR description documents the option as
volumes: { '/mnt/data': 'my-volume' }, but the actual surface (and the code in this PR) isvolumeMounts/volume_mounts. Worth fixing in the description so it doesn't read as a rename — option names must mirror exactly across SDKs (T-10). - Non-TASTE nit: the e2e volume names use
Date.now()in JS anduuid4()in Python.uuid4()in both would avoid collisions between concurrent runs against the same account.
| }) | ||
|
|
||
| test('Sandbox.create accepts a Volume instance as the mount source', async () => { | ||
| const volume = new Volume('vol-1', 'my-volume', 'volume-token') |
There was a problem hiding this comment.
T-4 — objects with remote lifecycles are obtained through async static factories (Volume.create, Volume.list); constructors are internal wiring. This is the first new Volume(...) call site in the JS SDK, and a test is documentation: it cements a construction path TASTE says users never take, and it also spells out the optional token positionally, which is exactly the chain T-3 rules out.
msw is already intercepting here, so the compliant form is to get the volume the way a caller would — add a http.post(apiUrl('/volumes'), …) handler returning { volumeID: 'vol-1', name: 'my-volume', token: 'volume-token' } and then const volume = await Volume.create('my-volume', { apiKey: TEST_API_KEY }). That keeps the test asserting the public path end to end instead of a hand-assembled object.
| def test_create_accepts_a_volume_instance_as_the_mount_source( | ||
| monkeypatch, test_api_key | ||
| ): | ||
| volume = Volume("vol-1", "my-volume", "volume-token") |
There was a problem hiding this comment.
T-4 / T-3 — same constructor point as the JS side, plus a local inconsistency: token, domain, debug and proxy are optional on Volume.__init__, and every existing Volume test constructs by keyword (tests/sync/volume_sync/test_volume_content.py, tests/test_volume_client.py). Passing them positionally is the argument order TASTE rejects.
At minimum match the existing convention:
| volume = Volume("vol-1", "my-volume", "volume-token") | |
| volume = Volume(volume_id="vol-1", name="my-volume", token="volume-token") |
Better, mirror whatever the JS test ends up doing and obtain the instance from Volume.create against the mocked transport.
| async def test_async_create_maps_mount_paths_to_named_volume_mounts( | ||
| monkeypatch, test_api_key | ||
| ): | ||
| body = await _async_request_body( | ||
| monkeypatch, test_api_key, {"/mnt/data": "my-volume"} | ||
| ) | ||
|
|
||
| assert body["volumeMounts"] == [{"name": "my-volume", "path": "/mnt/data"}] |
There was a problem hiding this comment.
T-1 / T-2 — sync and async are full mirrors with the same method names and semantics, so their coverage shouldn't diverge either. Sync gets three cases (omitted, string name, Volume instance); async gets only the string-name one. The gap that matters is the instance case: AsyncSandbox.create maps AsyncVolume instances through its own transform in sandbox_async/main.py, and that mapping is asserted nowhere in the mocked tier — the only place an AsyncVolume mount source appears is the e2e test, which won't run by default.
Add the two missing async cases (None → no volumeMounts key, and {"/mnt/data": AsyncVolume(...)} → [{"name": …, "path": …}]) so the async mirror is complete. _async_request_body already takes the mounts argument, so each is a three-line test.
There was a problem hiding this comment.
LGTM — test-only PR, no production code changes.
Verified the mocked payload assertions ({name, path} shape, Volume instance vs string mount source, omission when no volumes) against the actual JS (sandboxApi.ts) and Python (sandbox_api.py) implementations — they match exactly. E2E mount tests follow the established pattern (create sandbox, write via mount, kill, recreate, read) and are correctly gated behind hostedTest/@pytest.mark.e2e.
Extended reasoning...
Overview
This PR adds test-only coverage for Volume mounts: a mocked unit test verifying the shape of the volumeMounts payload sent in the sandbox create request (JS and Python), and an e2e test verifying that content written to a volume-mounted path in one sandbox is readable from a second sandbox mounting the same volume. No production/SDK source files are touched.
Security risks
None. These are additive test files; no new code paths, auth logic, or data handling are introduced in shipped code.
Level of scrutiny
Low. Test-only change with no runtime impact. I still verified the assertions against the real implementation (packages/js-sdk/src/sandbox/sandboxApi.ts and packages/python-sdk/e2b/sandbox/sandbox_api.py) since the PR description quotes a stale payload shape (volumeName/mountPath) that doesn't match either the tests or the actual code (both use name/path) — the tests themselves are correct, only the description text is outdated.
Other factors
The e2e tests are properly gated behind hostedTest (JS) and @pytest.mark.e2e + skip_debug (Python) so they won't run in the default/mocked tier, consistent with the tiering effort described across this stack of PRs. Cleanup (kill sandboxes, destroy volume) is handled in try/finally blocks in both languages.
Summary
3/4 of the stack replacing #1739 (base: #1743). Volume needed no migration — the existing suites (
volume/volume.test.ts,volume/file.test.ts,volume_sync,volume_async) are already msw/monkeypatch based and stay in the default tier. This PR only fills the two gaps the tier split exposed.Mocked, default tier — how a volume reaches the create-sandbox request:
E2E tier — the one Volume behavior a mock can't assert, that content actually survives on the real mount across sandboxes:
Not yet run against live infra.
Link to Devin session: https://app.devin.ai/sessions/e237ae2e3d8043fbabca7017400e6e57
Requested by: @mishushakov