test: split Template suites into mocked unit and e2e tiers (4/4) - #1745
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 47eed85. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787315092-test-tiers-template.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787315092-test-tiers-template.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787315092.test.tiers.template-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 Template test-tier split.
Checked: parity across JS / sync Python / async Python (T-1, T-2), enum vocabulary (T-15, T-41), template builder rules (T-37–T-43), error taxonomy (T-42, T-57–T-59), module structure (T-54). The PR is test-only, so most API-shape rules don't apply; what it does touch — using InstructionType.RUN/WORKDIR/COPY members instead of bare 'RUN' literals in both languages (T-15, T-41) and keeping the two mocked serialization suites name-for-name mirrors (T-1) — is right.
2 violations, both in the new serialization suites: the pinned-behavior TODOs name the wrong target error class (T-42), and InstructionType has to be deep-imported from a subpath (T-54).
Not tied to a changed line, for context rather than action here:
- The new tests document a JS/Python parity gap in the API they exercise (T-1): JS
Template.toJSON(template, computeHashes)takes a second parameter, PythonTemplate.to_json(template)has none and always hashes — which is why the JScomputeHashes adds the copy hash to the payloadtest has no Python mirror. That parameter is also an optional positional boolean, which T-3 (optional params go in a trailing options object) and T-14 (enums over booleans) both rule out; fixing it belongs in a source PR, not this one. - The two mirrors assert the failure case at different strengths — Python pins
pytest.raises(ValueError), JS onlyrejects.toThrow(). OncecalculateFilesHashraises a typed error, tighten the JS side to that class so the mirrors stay 1:1.
| // TODO: should reject with TemplateError once calculateFilesHash stops | ||
| // throwing a bare Error. |
There was a problem hiding this comment.
T-42 (builder precondition and configuration failures raise BuildError / BuildException, never a bare Error): the TODO correctly flags the bare Error, but names the wrong replacement. calculateFilesHash failing because the copy source matches no file is a builder-time precondition failure on the local file context, so the target is BuildError — TemplateError extends SandboxError and is for sandbox-side template/envd failures. Pinning the wrong class in the comment is how the eventual fix lands on the wrong error.
| // TODO: should reject with TemplateError once calculateFilesHash stops | |
| // throwing a bare Error. | |
| // TODO: should reject with BuildError once calculateFilesHash stops | |
| // throwing a bare Error. |
| import { InstructionType } from '../../src/template/types' | ||
| import { calculateFilesHash } from '../../src/template/utils' |
There was a problem hiding this comment.
T-54 (one flat entry point per package — everything public is re-exported from index.ts, no subpath exports): InstructionType is the SDK's own vocabulary for step types (T-15) and the only way to assert on a serialized payload, yet it is not in the export type { ... } from './types' list in src/template/index.ts, so this suite has to reach into src/template/types. Add InstructionType to the package's public exports and import both it and Template from '../../src'. (calculateFilesHash is genuinely internal — the deep import for it is fine.)
| # TODO: should raise TemplateException once calculate_files_hash stops | ||
| # raising a bare ValueError. |
There was a problem hiding this comment.
T-42 (builder precondition and configuration failures raise BuildException, never a bare ValueError): same as the JS mirror — the bare ValueError is the real problem, but the target class should be BuildException, not TemplateException (which extends SandboxException and covers sandbox-side template failures).
| # TODO: should raise TemplateException once calculate_files_hash stops | |
| # raising a bare ValueError. | |
| # TODO: should raise BuildException once calculate_files_hash stops | |
| # raising a bare ValueError. |
| from e2b.template.types import InstructionType | ||
| from e2b.template.utils import calculate_files_hash |
There was a problem hiding this comment.
T-54 (everything public is listed in __init__.py's __all__; no subpath imports): InstructionType is not exported from e2b, so the suite deep-imports e2b.template.types. Export it from the package entry point and import it alongside Template — same fix as the JS mirror, keeping the two surfaces aligned (T-1). calculate_files_hash is internal, so its deep import is fine.
![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-tier split (4/4 in the stack) consistent with the established e2eTest/hostedTest/e2eBuildTemplateTest conventions and the pattern from the prior PRs in this series.
Extended reasoning...
Overview
This PR is test-only: it moves server-side Template build/exists/tags tests from the default unit tier to the e2e tier (e2eTest/e2eBuildTemplateTest), adds new mocked coverage for client-side serialization/hashing logic (serialization.test.ts and its Python mirror), and pins already-mocked stacktrace suites with pytest.mark.mocked. No production/runtime code is touched — only test files under packages/js-sdk/tests and packages/python-sdk/tests.
Security risks
None. The diff touches only test harness wiring (which tier a test runs in) and test assertions; there is no change to auth, crypto, request handling, or any code path that ships to users.
Level of scrutiny
Low-to-moderate is appropriate. This is a mechanical, repetitive refactor (splitting existing tests across test tiers) following a pattern already established and merged in three prior PRs in this same stack (visible in recent commit history: 016c164, 0c481b0, 328c3e3). The one finding from the bug-hunting pass — exists.test.ts using bare e2eTest instead of hostedTest, so it wouldn't correctly skip under the combined E2B_E2E=1+E2B_DEBUG opt-in configuration — is real (confirmed by reading tests/setup.ts and tests/README.md, which document hostedTest as exactly the variant for control-plane-only operations) but is scoped to a niche local-dev combination and is itself labeled a nit; it doesn't affect the default unit tier or plain e2e runs without debug.
Other factors
The Python mirrors of the same tests correctly carry @pytest.mark.skip_debug() alongside the new @pytest.mark.e2e marker, so the JS gap is an isolated omission rather than a systemic misunderstanding of the pattern. No production code, no new dependencies, and the changeset-bot's 'no changeset' notice is expected/correct since this only touches test files, not the published packages.
| import { randomUUID } from 'node:crypto' | ||
| import { expect, test } from 'vitest' | ||
| import { expect } from 'vitest' | ||
| import { Template } from '../../src' | ||
| import { e2eTest } from '../setup' | ||
|
|
||
| test('check if base template name exists', async () => { | ||
| e2eTest('check if base template name exists', async () => { | ||
| const exists = await Template.exists('base') | ||
| expect(exists).toBe(true) | ||
| }) | ||
|
|
||
| test('check non existing name', async () => { | ||
| e2eTest('check non existing name', async () => { | ||
| const nonExistingName = `nonexistent-${randomUUID()}` | ||
| const exists = await Template.exists(nonExistingName) | ||
| expect(exists).toBe(false) |
There was a problem hiding this comment.
🟡 Use hostedTest instead of e2eTest for tests that hit control-plane/build-only APIs, so E2B_DEBUG still skips them -- sweep:e2eTest\( (seen in backgroundBuild.test.ts and exists.test.ts). hostedTest = e2eTest.skipIf(isDebug) is the variant meant for exactly this case; a local envd cannot service Template.buildInBackground or Template.exists, so under E2B_E2E=1 + E2B_DEBUG these tests would hit the wrong server and fail/hang instead of skipping, unlike their Python mirrors which carry @pytest.mark.skip_debug().
Extended reasoning...
tests/setup.ts defines three tiers relevant here: e2eTest = base.skipIf(!isE2E) (no debug guard), hostedTest = e2eTest.skipIf(isDebug), and e2eBuildTemplateTest = buildTemplateTest.skipIf(!isE2E || isDebug). The hostedTest/e2eBuildTemplateTest variants exist specifically for operations that a local envd (used under E2B_DEBUG) cannot stand in for: control-plane routes, real builds, snapshots, the traffic proxy. tests/README.md documents this convention explicitly, and sandbox/create.test.ts / connect.test.ts already follow it.
This PR wraps backgroundBuild.test.ts (Template.buildInBackground, a real server-side build) and exists.test.ts (Template.exists -> aliasExists -> checkAliasExists, a control-plane GET /templates/aliases/{alias} route) in bare e2eTest rather than hostedTest. Neither operation has a local-envd fallback: under E2B_DEBUG, ConnectionConfig.apiUrl resolves to http://localhost:3000, which does not serve the builds API or the templates-alias control-plane route.
Concrete walkthrough for exists.test.ts: run with E2B_E2E=1 E2B_DEBUG=1. isE2E is true so e2eTest does not skip. Template.exists(\"base\") calls checkAliasExists, which issues client.api.GET(\"/templates/aliases/{alias}\") against http://localhost:3000 (the local envd) instead of the real control plane. That route does not exist on envd, so the request fails or 404s, and expect(exists).toBe(true) fails instead of the test being skipped -- exactly the outcome hostedTest exists to prevent. The same reasoning applies to backgroundBuild.test.ts, where Template.buildInBackground would attempt a real server-side build against an envd that has no build endpoint.
This is also a JS/Python parity gap (a rule from CLAUDE.md): the Python mirrors test_exists.py and test_background_build.py (both sync and async) correctly carry both @pytest.mark.e2e and the pre-existing @pytest.mark.skip_debug(), so only the JS side regresses under the combined E2B_E2E + E2B_DEBUG configuration.
Fix is mechanical: replace e2eTest with hostedTest (or equivalently e2eTest.skipIf(isDebug)) as the import and wrapper in both files. Severity is nit -- this only misfires in the niche opt-in combination of E2B_E2E=1 and E2B_DEBUG together; the default unit tier and the plain e2e-without-debug tier are unaffected, and it is test-only code with no production impact.
1 participant
Summary
Top of the stack replacing #1739 (base: #1744). Template builds run server-side, so the build suites move to the e2e tier while the client-side build inputs — hashing, Dockerfile parsing, payload serialization — get mocked coverage.
Server-side builds (
build,backgroundBuild,exists,tags, and the Pythontemplate_{sync,async}equivalents) switch toe2eBuildTemplateTest/@pytest.mark.e2e;E2B_DEBUGskips them too, since a local envd never performs a build.tags.test.tswas mixed, so its mocked assertions moved totemplate/tagsBuild.test.ts, and the fully mockedtest_stacktrace.pysuites are pinned withpytestmark = pytest.mark.mockedso the conftest auto-marker leaves them in the default tier.New mocked coverage in
template/serialization.test.ts(Python:template_sync/test_serialization.py; the hashing/serialization code is synchronous and shared, so there is no async mirror):The missing-source case carries a
TODO:calculateFilesHashcurrently throws a bareError, so the test pins that rather than a typed SDK error.Link to Devin session: https://app.devin.ai/sessions/e237ae2e3d8043fbabca7017400e6e57
Requested by: @mishushakov