test: mock all template build tests - #1738
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:
|
|
Package ArtifactsBuilt from 6ccdf8c. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787255437-mock-template-build-tests.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787255437-mock-template-build-tests.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787255437.mock.template.build.tests-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: 193dee35f4
ℹ️ 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".
| http.get(apiUrl('/templates/:templateID/files/:hash'), () => { | ||
| return HttpResponse.json({ present: true }) | ||
| }), |
There was a problem hiding this comment.
Return an upload URL and handle the upload for forced or uncached files. The build tests pass forceUpload: true, but this response always reports the file as cached without a URL, so the SDK necessarily skips uploadFile; consequently the copy and symlink build tests can pass even when archive creation or upload orchestration is broken. The Python mock has the equivalent bypass.
AGENTS.md reference: AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
| for (const [index, step] of (body.steps ?? []).entries()) { | ||
| // RUN steps carry the user in args[1]; only users that exist in the | ||
| // base image are accepted, like the real build backend. | ||
| const user = step.type === 'RUN' ? step.args?.[1] : undefined | ||
| if (user && !VALID_USERS.has(user)) { |
There was a problem hiding this comment.
Validate the expected serialized instructions rather than reporting success for every received step list. Apart from the special invalid-user case, this loop accepts arbitrary steps—and also an empty list—without executing or asserting them, so the converted runCmd, makeSymlink, copy, workdir, and start-command tests still pass if the SDK drops or misserializes those operations. The Python mock mirrors the same permissive behavior.
AGENTS.md reference: AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against TASTE.md (e2b-dev/sdk-harness). This is a test-only PR, so most public-surface rules don't apply; I checked the ones that reach test code — enum usage (T-15/T-41), env-var handling (T-50), acronym casing (T-12), naming/parity, and the wire-format rules (T-40, which the mocks follow correctly).
3 issues, 4 inline comments:
?? TEST_API_KEYonprocess.env.E2B_API_KEYtreats an exported-but-empty var as set (T-50) — one comment covering all four new occurrences.- Both mocks dispatch on the instruction type with a bare
'RUN'literal instead ofInstructionType.RUN(T-41/T-15) — one comment per SDK. MockBuildusestemplateID/buildIDfield names on a type we own (T-12).
Everything else looks consistent with the harness taste: the JS mock keeps camelCase wire keys per T-40, the Python mirror returns the SDK's typed TemplateBuildStatusResponse/TemplateTag dataclasses, invalid-user builds still fail with the same BuildError/BuildException semantics, and the sync/async Python mirrors get identical fixtures per T-2.
| onBuildLogs: captureLogs, | ||
| // The placeholder key keeps the mocked template tests independent of | ||
| // E2B_API_KEY being set in the environment. | ||
| apiKey: process.env.E2B_API_KEY ?? TEST_API_KEY, |
There was a problem hiding this comment.
T-50 — an env var set to the empty string means unset. ?? only falls through on undefined/null, so an exported-but-empty E2B_API_KEY='' gets passed as an explicit (highest-precedence) apiKey option and reaches the transport as a malformed credential instead of falling back to TEST_API_KEY.
| apiKey: process.env.E2B_API_KEY ?? TEST_API_KEY, | |
| apiKey: process.env.E2B_API_KEY || TEST_API_KEY, |
The same pattern appears in the other new fallbacks: template/backgroundBuild.test.ts:15, template/exists.test.ts:15, and template/tags.test.ts:64.
| for (const [index, step] of (body.steps ?? []).entries()) { | ||
| // RUN steps carry the user in args[1]; only users that exist in the | ||
| // base image are accepted, like the real build backend. | ||
| const user = step.type === 'RUN' ? step.args?.[1] : undefined |
There was a problem hiding this comment.
T-41 / T-15 — every site that dispatches on instruction.type goes through the InstructionType enum, never a bare 'RUN' literal; the enum is what makes a typo a compile error. fromDockerfile.test.ts already imports it from ../../../src/template/types.
| const user = step.type === 'RUN' ? step.args?.[1] : undefined | |
| const user = | |
| step.type === InstructionType.RUN ? step.args?.[1] : undefined |
(plus import { InstructionType } from '../../src/template/types' at the top, and steps?: { type: InstructionType; ... } in the parsed body type would tighten it further).
| args = step.get("args") or [] | ||
| # RUN steps carry the user in args[1]; only users that exist in | ||
| # the base image are accepted, like the real build backend. | ||
| user = args[1] if step_type == "RUN" and len(args) > 1 else None |
There was a problem hiding this comment.
T-41 / T-15 — line 119 correctly round-trips through InstructionType, but then drops back to a bare "RUN" string literal for the dispatch. Compare against the enum member so the set of instruction types stays written down in one place:
| user = args[1] if step_type == "RUN" and len(args) > 1 else None | |
| user = ( | |
| args[1] | |
| if step_type == InstructionType.RUN.value and len(args) > 1 | |
| else None | |
| ) |
(or keep step_type = InstructionType(step.get("type")) un-.valued and compare step_type == InstructionType.RUN).
| templateID: string | ||
| buildID: string |
There was a problem hiding this comment.
T-12 — all-caps acronym forms (templateID, buildID) belong only to wire fields in the generated client, not to types we own. The JSON response bodies rightly keep the wire spelling, but MockBuild/MockBuildApiState are internal state (and the Python mirror already spells them template_id/build_id), so these fields should be templateId/buildId, converting at the response boundary.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Check whether the files for a hash are already uploaded. Always | ||
| // reporting them as cached (with no upload URL) skips the upload step. | ||
| http.get(apiUrl('/templates/:templateID/files/:hash'), () => { | ||
| return HttpResponse.json({ present: true }) | ||
| }), |
There was a problem hiding this comment.
🟡 The file-cache-check mock always returns { present: true } with no url (mockBuildApi.ts:140-142; Python mock_build_api.py get_file_upload_link mirrors this with url=None), but the real gating logic only calls uploadFile/spoolTarArchive when a non-null url is returned. As a result the forceUpload-driven build tests (e.g. 'build template with symlinks', 'with resolveSymlinks' in both JS and Python) never actually exercise the tar-archive/upload path anymore, though they still pass since they only assert the build call doesn't throw. Consider having the mock return a fake upload URL plus a handler for the PUT so these tests keep covering that code path.
Extended reasoning...
What happens: mockBuildApi.ts (lines 138-142) implements the file-cache-check endpoint (GET /templates/:templateID/files/:hash) by always responding { present: true } with no url field. The Python mock mirrors this exactly: get_file_upload_link in mock_build_api.py returns SimpleNamespace(present=True, url=None) unconditionally.
Why it matters: the real upload-gating logic in both SDKs is (forceUpload && url != null) || (present === false && url != null) (js-sdk index.ts ~1124-1127; python main.py ~118-125, mirrored with force_upload and file_info.url). Both branches require a non-null url. Since the mock never supplies one, this condition is permanently false for every test that routes through it, regardless of whether forceUpload is set. Concretely: Template.build/AsyncTemplate.build never calls uploadFile, so spoolTarArchive (which does the actual tar packaging with follow: resolveSymlinks) is never invoked.
Step-by-step proof: 1) 'build template with symlinks' in build.test.ts calls .copy('folder/*', 'folder', { forceUpload: true }). 2) During instructionsWithHashes, the SDK calls the mocked GET /templates/:templateID/files/:hash, which replies { present: true } (no url). 3) The gating check evaluates forceUpload=true && url != null → false (url is undefined), and present === false... → false since present is true. 4) uploadFile/spoolTarArchive is skipped entirely; the build proceeds straight to triggerBuild. 5) The test only asserts await buildTemplate(template) resolves without throwing — it never inspects logs or upload calls — so it passes green while never touching the archive/upload code for this run.
Why existing code doesn't catch it: the mock's comment ('Always reporting them as cached ... skips the upload step') documents this as an intentional simplification of the file-cache-check semantics, not a conscious tradeoff against the specific forceUpload tests, whose names ('with symlinks', 'with resolveSymlinks') suggest they were written to validate the tar/upload+symlink-resolution path.
Addressing the refutation: one reviewer correctly points out this isn't a total loss of coverage — calculateFilesHash/instructionsWithHashes still runs unconditionally before the gate and does traverse/hash symlinks, so a regression in that hashing logic would still surface. It's also true JS's uploadFile.test.ts and Python's test_upload_file.py cover the raw upload transport directly against a local server, and the PR description explicitly calls out upload_file was deliberately left unpatched for this reason. Those are fair, and they narrow the blast radius: this is not 'upload is completely untested,' it's specifically that the tar-archive → upload leg triggered from a full Template.build() call, gated on forceUpload, is no longer exercised end-to-end by the tests whose names promise that coverage. That's a real, narrow regression in test intent, not a functional bug — nothing in production breaks and all tests still pass.
Suggested fix: have createMockBuildApi's file-cache-check handler return a fake url (e.g. pointing at another mock endpoint) and add an MSW/monkeypatch handler for the PUT so uploadFile/spoolTarArchive actually run during these tests, restoring their original intent without needing live infra.
|
superseded by #1739 |
1 participant
Summary
Template/build tests now run entirely against deterministic in-process mocks — no live build infra or
E2B_API_KEYneeded — following the same pattern as the volume-test mocks (#1734):tests/template/mockBuildApi.ts):createMockBuildApi()returns MSW handlers with per-instance state covering the full build flow —POST /v3/templates(request build), file-cache check (returnspresent: trueso uploads are skipped), build trigger (records log entries, validates RUN users so the invalid-user test still fails deterministically), status polling (waiting→buildingwhile logs drain →ready/error), alias lookup (seedsbase), and tag assign/remove/list with invalid-format 400s.build.test.ts,backgroundBuild.test.ts,exists.test.ts,tags.test.ts, andmethods/{runCmd,makeSymlink}.test.tseach start a server from these handlers and passTEST_API_KEYas fallback.stacktrace.test.ts,abortSignal.test.ts,boundOpts.test.ts, anduploadFile.test.tskeep their existing isolated setups.tests/mock_build_api.py):MockBuildAPIimplements the same semantics and monkeypatches the build-API seams (request_build,get_file_upload_link,trigger_build,get_build_status,check_alias_exists,assign_tags,remove_tags,get_template_tags) in bothe2b.template_syncande2b.template_asyncvia autouse fixtures in thetemplate_sync/template_asyncconftests.upload_fileis deliberately not patched, preserving the local HTTP-transport upload tests, and the stacktrace tests' own monkeypatches still take precedence.@pytest.mark.skip_debug()markers from the mocked template tests.Test-only change, so no changeset.
Verified locally: JS
vitest --project template(138 passed, 3 skipped), Python sync (75 passed) and async (76 passed) template suites — all withE2B_API_KEYunset — plus repo-wide format/lint/typecheck.Link to Devin session: https://app.devin.ai/sessions/c0c4becd74df40918b2497fde636f050
Requested by: @mishushakov