test: add opt-in e2e tier scaffolding for the SDK test suites (1/4) - #1742
test: add opt-in e2e tier scaffolding for the SDK test suites (1/4)#1742devin-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. |
🦋 Changeset detectedLatest commit: d034d15 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 326c70e. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.44.2-devin-1787315092-test-tiers-base.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787315092-test-tiers-base.0.tgzPython SDK ( pip install ./e2b-2.44.0+devin.1787315092.test.tiers.base-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
Checked the changed lines against the rules that this PR can actually engage: cross-language parity (T-1, T-1a, T-2), identifier conventions (T-11, T-12), config resolution and defaults (T-47, T-49, T-50), and docstring/JSDoc rules (T-69-T-72). The API-shape, error, timeout and template-builder sections aren't touched — this is test scaffolding, no runtime code changes.
5 violations, all in the new shared test helpers and the docs describing them.
Not tied to a single line:
- The two
e2eTestexports don't mean the same thing: inpackages/cli/tests/setup.tsit istest.skipIf(skipE2E), which also skips underE2B_DEBUG, while inpackages/js-sdk/tests/setup.tsit isbase.skipIf(!isE2E)and the debug axis lives in the separatehostedTest. Same name, different semantics across packages is the thing T-1 exists to prevent — either give the CLI helper the JS split (e2eTest+hostedTest) or name the CLI one after what it does. - The Python tier has no
E2B_E2Eequivalent at all (the marker plusaddopts = -m "not e2e"is the whole mechanism), so the opt-in switch is spelled two different ways across the three surfaces. That's defensible as pytest idiom, but then the docs must not promise a single flag (see the CONTRIBUTING.md comment).
|
|
||
| const userConfig = safeGetUserConfig() | ||
|
|
||
| const DEFAULT_E2E_DOMAIN = 'e2b.dev' |
There was a problem hiding this comment.
T-49 (config resolves explicit option -> E2B_-prefixed env var -> default): the fallback the removed code used was 'e2b.app', which is also the SDK's own default (connectionConfig.ts: getEnvVar('E2B_DOMAIN') || 'e2b.app'). This silently repoints every credential-less CLI e2e run at a different deployment, and the default now disagrees with the SDK the CLI wraps. If a second default is genuinely wanted here it needs to be an explicit decision, not a rename side effect.
| const DEFAULT_E2E_DOMAIN = 'e2b.dev' | |
| const DEFAULT_E2E_DOMAIN = 'e2b.app' |
| export const isDebug = process.env.E2B_DEBUG !== undefined | ||
|
|
||
| /** Opt-in for the e2e tier: tests that need real infrastructure. */ | ||
| export const isE2E = process.env.E2B_E2E !== undefined |
There was a problem hiding this comment.
T-12 (acronyms are cased as words in identifiers: Url, Http, Id — all-caps forms belong only to names that aren't ours): isE2E keeps the acronym all-caps mid-identifier. Same for skipE2E in packages/cli/tests/setup.ts. The env var stays E2B_E2E; only the identifiers change:
export const isE2e = process.env.E2B_E2E !== undefined
export const skipE2e = !isE2e || !e2eApiKey || isDebug
No suggestion block here since the rename has to move with every reference in the same commit.
| /** | ||
| * The highest envd version below one of the `ENVD_*` thresholds, for | ||
| * exercising the reject branch of a version gate without hardcoding a version | ||
| * that stops being below the threshold when it moves. A prerelease of a | ||
| * version sorts below the version itself. | ||
| */ | ||
| export function belowEnvdVersion(version: string): string { | ||
| return `${version}-0` | ||
| } |
There was a problem hiding this comment.
T-70 (JS uses JSDoc tags: @param, @returns, ...) and T-72 (a doc line that misdescribes the value documents nothing): the block is prose only — no @param, no @returns — and "the highest envd version below" isn't what -0 produces; ${version}-0 is the lowest prerelease of version. What the caller needs to know is just that the result sorts below the threshold.
| /** | |
| * The highest envd version below one of the `ENVD_*` thresholds, for | |
| * exercising the reject branch of a version gate without hardcoding a version | |
| * that stops being below the threshold when it moves. A prerelease of a | |
| * version sorts below the version itself. | |
| */ | |
| export function belowEnvdVersion(version: string): string { | |
| return `${version}-0` | |
| } | |
| /** | |
| * A version that sorts just below one of the `ENVD_*` thresholds, for | |
| * exercising the reject branch of a version gate without hardcoding a version | |
| * that stops being below the threshold when it moves. | |
| * | |
| * @param version The `ENVD_*` threshold constant to stay below. | |
| * @returns The threshold with a prerelease suffix appended, which semver sorts | |
| * below the threshold itself. | |
| */ | |
| export function belowEnvdVersion(version: string): string { | |
| return `${version}-0` | |
| } |
| def below_envd_version(version: Version) -> str: | ||
| """The highest envd version below one of the `ENVD_*` thresholds. | ||
|
|
||
| Lets a gate's reject branch be exercised without hardcoding a version that | ||
| stops being below the threshold when it moves — a release candidate of a | ||
| version sorts below the version itself. | ||
| """ | ||
| return f"{version}rc1" |
There was a problem hiding this comment.
T-71 (Python uses reST/Sphinx field style: :param name:, :return:) — the docstring has neither field. Same "highest envd version below" inaccuracy as the JS twin, and the mirror wording should match it (T-1a: belowEnvdVersion <-> below_envd_version should differ only by language idiom, and right now one says "a prerelease" while the other says "a release candidate").
| def below_envd_version(version: Version) -> str: | |
| """The highest envd version below one of the `ENVD_*` thresholds. | |
| Lets a gate's reject branch be exercised without hardcoding a version that | |
| stops being below the threshold when it moves — a release candidate of a | |
| version sorts below the version itself. | |
| """ | |
| return f"{version}rc1" | |
| def below_envd_version(version: Version) -> str: | |
| """A version that sorts just below one of the `ENVD_*` thresholds. | |
| Lets a gate's reject branch be exercised without hardcoding a version that | |
| stops being below the threshold when it moves. | |
| :param version: The ``ENVD_*`` threshold constant to stay below. | |
| :return: The threshold with a prerelease suffix appended, which sorts below | |
| the threshold itself. | |
| """ | |
| return f"{version}rc1" |
| - **unit (default)** — fully mocked, deterministic, no sandboxes and no | ||
| credentials. | ||
| - **e2e (opt-in)** — real sandboxes, envd round-trips and template builds; | ||
| requires `E2B_E2E=1` and an API key. |
There was a problem hiding this comment.
T-1 (the surfaces mirror each other, differing only by language idiom) plus doc accuracy: E2B_E2E=1 is not how the Python tier is selected. Nothing in packages/python-sdk reads E2B_E2E — pytest.ini's addopts = -m "not e2e" plus -m e2e is the whole switch, so following this line verbatim in Python runs the mocked tier and reports success without touching a sandbox.
| requires `E2B_E2E=1` and an API key. | |
| requires an API key, plus the tier's opt-in: `E2B_E2E=1` in JS and the CLI, | |
| `-m e2e` in Python. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, a few other candidates were checked and ruled out: the e2eFiles.mjs import (actual file is .mts) resolves fine under this package's moduleResolution: bundler — the same .mjs-for-.mts pattern already exists elsewhere in the JS SDK's Cloudflare config. The tagsBuild glob entry doesn't match any file (the real file is tags.test.ts), but it's harmless dead pattern since tags.test.ts is msw-mocked and correctly stays in the template project either way. The CLI e2e default domain change (e2b.app → e2b.dev) matches the CLI's own existing default elsewhere (src/user.ts).
Extended reasoning...
This is a single-paragraph informational note, not a full review. I reviewed the PR diff and confirmed the reported bug independently (the four listed test files do still live inside the newly-globbed e2e directories in the current tree). I also spent time checking three other candidate issues the bug-hunting system's finders raised but its verifiers refuted, and confirmed those refutations hold up: the .mjs/.mts import extension mismatch is a known-safe pattern under this package's bundler module resolution (already used identically in the Cloudflare vitest config), the tagsBuild glob typo is dead but harmless because the real tags.test.ts file is msw-mocked and correctly remains in the template project regardless, and the e2e default domain change to e2b.dev matches the CLI's own existing default in src/user.ts. None of these change my view of the PR; the inline finding about the four swept-up mocked test files is the substantive issue here and is enough on its own to warrant a human look before merge.
| export const e2eFiles = [ | ||
| 'tests/api/{info,kill,list,snapshot}.test.ts', | ||
| 'tests/sandbox/commands/**/*.test.ts', | ||
| 'tests/sandbox/files/**/*.test.ts', | ||
| 'tests/sandbox/git/**/*.test.ts', | ||
| 'tests/sandbox/pty/**/*.test.ts', | ||
| 'tests/sandbox/{connect,create,fork,host,internetAccess,kill,lifecycleBehavior,metrics,network,secure,snapshot,snapshot-api,timeout}.test.ts', | ||
| 'tests/template/{backgroundBuild,build,exists,tagsBuild}.test.ts', | ||
| 'tests/volume/mount.test.ts', | ||
| 'tests/template/methods/{makeSymlink,runCmd}.test.ts', |
There was a problem hiding this comment.
🔴 e2eFiles.mts globs whole directories (tests/sandbox/{commands,files,git,pty}/**) into the e2e-only project, but the per-file moves that pull the pure-mock, credential-free tests (commandHandle.test.ts, entryInfo.test.ts, watchHandle.test.ts, git/validation.test.ts) up out of those directories only land in PR 2/4, not here. As this PR stands alone, vitest.config.mts excludes e2eFiles from the unit/template projects (and Cloudflare excludes it too), so these four tests silently drop out of the default pnpm test / required CI run and only execute under the opt-in E2B_E2E=1 tier, even though they need no sandbox or credentials. Consider narrowing the globs to exclude these four files specifically, or landing the file moves in this PR instead of 2/4.
Extended reasoning...
The bug: tests/e2eFiles.mts globs entire directories — tests/sandbox/commands/**, tests/sandbox/files/**, tests/sandbox/git/**, tests/sandbox/pty/** — into the new e2e vitest project. The file's own header comment claims the client-side logic that used to live in those directories (commandHandle, entryInfo, watchHandle, gitValidation) "sits one level up," and the Cloudflare config's comment was even renamed to reference tests/sandbox/gitValidation.test.ts. But none of those file moves actually happen in this PR — per the PR description, the per-module moves land in PR 2/4 of the stack. As merged in isolation, this PR is the base scaffolding only.\n\nThe code path: vitest.config.mts sets exclude: e2eFiles on both the unit and template projects, and tests/runtimes/cloudflare/vitest.config.mts excludes the same list. The default pnpm test script runs only --project unit --project template --project connectionConfig — never e2e, which requires E2B_E2E=1 plus credentials. Since the four files below still physically live inside the globbed directories, the directory-wide globs sweep them into the credential-gated e2e project along with everything else in those folders:\n- tests/sandbox/commands/commandHandle.test.ts — pure unit test of CommandHandle, driven entirely by vi-mocked async iterables\n- tests/sandbox/files/entryInfo.test.ts — pure unit test of mapEntryInfo\n- tests/sandbox/files/watchHandle.test.ts — pure unit test using vi-mocked events\n- tests/sandbox/git/validation.test.ts — uses a stub Commands runner explicitly designed to fail if it's ever called, asserting client-side InvalidArgumentError validation with no network access\n\nNone of these four need a sandbox, envd, or an API key.\n\nWhy nothing else catches it: the PR's whole stated goal is to make the tier split airtight — e.g. the Python side adds pytest_collection_modifyitems specifically so "a new test cannot silently land in the wrong tier." On the JS side there's no equivalent safety net; the tier assignment is purely by directory glob, and the globs were written for the end state of the whole 4-PR stack rather than this PR's own diff. So the same class of problem the PR is trying to prevent (a test silently ending up in the wrong tier) happens here in the opposite direction: existing, already-passing unit tests silently leave the default tier.\n\nProof, step by step:\n1. Check out this PR's HEAD in isolation (before 2/4 merges).\n2. Confirm tests/sandbox/commands/commandHandle.test.ts still exists at that path (it does — the move to one level up hasn't happened).\n3. Run npx vitest list --project unit in packages/js-sdk — it returns zero matches for commandHandle, entryInfo, watchHandle, or git/validation.\n4. Run npx vitest list --project e2e — commandHandle.test.ts (and the other three) show up there instead, gated behind E2B_E2E=1 plus an API key.\n5. Run pnpm test (the required CI command) — none of the four files execute. Before this PR, the plain vitest run invocation included them by default.\n\nImpact: a regression in one of CommandHandle, mapEntryInfo, WatchHandle, or the git-command client-side validation between this PR merging and 2/4 merging would not be caught by the required SDK Tests check — it would only be caught by the opt-in, credentialed SDK E2E Tests workflow, which isn't part of the PR gate.\n\nFix: either land the four file moves in this PR (so the globs match reality immediately), or narrow e2eFiles.mts's directory globs to exclude these specific files (e.g. list the true e2e files individually, or add explicit negated patterns) until the moves land in 2/4.
1 participant
Summary
Bottom of a 4-PR stack replacing #1739 (same final tree, split per module for review): 1/4 base scaffolding → 2/4 sandbox → 3/4 volume → 4/4 template.
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). This PR adds the machinery for two tiers — a mocked default tier and an e2e tier gated onE2B_E2E— plus the CLI split, docs and CI wiring. The per-module test moves land in the PRs above.JS SDK
The e2e file list lives in one place,
tests/e2eFiles.mts, 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
--project !e2e, which pulls inbrowserand fails without a Playwright binary.E2B_E2Eis set in thee2eproject'senvrather than in the script, so it works on Windows too.Python SDK
Marking is automatic rather than per-file, so a new test can't silently land in the default tier:
A fully mocked test that happens to use a live-looking fixture opts out with
@pytest.mark.mocked.tests/envd_versions.pyaddsbelow_envd_version()so version-gate tests derive their input from theENVD_*constants (f"{version}rc1"sorts below the threshold) instead of hardcoding a version that stops being below it when the threshold moves.CLI
The credential/skip logic duplicated across suites 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.mocksuite asserting on the options the CLI passes toSandbox.create, so gating it on credentials would only delete default coverage).test:e2erunsvitest.e2e.config.mts, which setsE2B_E2Eintest.env.CI
Default legs run the mocked tier only; a separate opt-in
SDK E2E Testsworkflow (sdk_e2e_tests.yml) runs the e2e tier with credentials.Fixes SDK-136
Link to Devin session: https://app.devin.ai/sessions/e237ae2e3d8043fbabca7017400e6e57
Requested by: @mishushakov