Repository navigation
test(cli): remove duplicate, dead-code and vacuous CLI tests - #2141
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Package ArtifactsBuilt from c5d16df. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.1.3-devin-1794604309-prune-cli-tests.0.tgz ./e2b-2.64.1-devin-1794604309-prune-cli-tests.0.tgzCLI ( npm install ./e2b-cli-2.24.1-devin-1794604309-prune-cli-tests.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.9.2-devin-1794604309-prune-cli-tests.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.5.1-devin-1794604309-prune-cli-tests.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.1.2+devin.1794604309.prune.cli.tests-py3-none-any.whl ./e2b-2.64.0+devin.1794604309.prune.cli.tests-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.11.10+devin.1794604309.prune.cli.tests-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.7.2+devin.1794604309.prune.cli.tests-py3-none-any.whl |
There was a problem hiding this comment.
TASTE.md review: no violations (0). Checked T-49/T-50 (the new process.env.E2B_DOMAIN || 'e2b.app' treats an empty env var as unset), T-59/T-63 (chunkBytesBySize is now module-private, so its plain Error falls under the internal-helper exemption) and T-54 (the removed exec_helpers exports were never part of a package entry point). The rest of the diff only deletes tests.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Apart from the inline findings, I checked two other things and neither is a problem.
- The removed
exec_helpersexports have no importers left inpackages/, and the CLI package publishes only abinentry. Removing them doesn't change the public surface, so no changeset is needed. - The integration tests no longer read the domain from
~/.e2b/config.json. This loses nothing:UserConfig(src/user.ts) has noE2B_DOMAINordomainfield, so that fallback could never supply a value.
Extended reasoning...
This change deletes tests in packages/cli and removes dead stdin helpers from exec_helpers.ts. It touches no security-sensitive production code paths. The inline findings already cover the coverage gaps, so this note only records the concerns that were checked and ruled out.
|
check findings ^ |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
Both findings were valid and are fixed in ad7ce5e:
I didn't put back the |
Summary
Applied the TDD skill (
skills/tdd/SKILL.md) topackages/cli/testsand removed tests that don't catch any regression the remaining suite wouldn't. Tests only, plus deletion of dead helpers, so no changeset needed.Dead code (tests + source removed)
readStdinIfPiped/readStdinFrominexec_helpers.tshave had no production caller sinceexecswitched tostreamStdinChunks. Deleted them and their 5 tests.chunkBytesBySizeis now module-private. Its 5 direct tests duplicated thestreamStdinChunkssplitting and empty-chunk tests;maxBytesis a constant in production.Duplicates
sandbox/list.test.ts: dropped the JSON default-asc sort and "does not mutate input" tests. The table-path locale/tie-break/desc tests already go throughsortSandboxes. Also droppedstartedAt === startedAt.toLocaleString(), which just restated the implementation, and renamed that test to "formats state and metadata for display".create_lifecycle.test.ts: "lifecycle and timeout together" combined two mappings already tested on their own. "omits empty envs" was already asserted by the explicit-timeout test.fork.test.ts: dropped--count 101;21already covers values above the max.Vacuous / never-run integration code
backend_integration.test.ts"exec runs a command with piped stdin" accepted either6or0, so it could not fail. Real piping coverage lives inexec_pipe.test.ts.exec_pipe.test.ts: removed thebinary_random_sha256case. It only ran behindE2B_PIPE_INTEGRATION_STRICT/BINARY/STRICT, which no workflow sets, andbinary_nul_ff_hexalready covers binary.userConfig.E2B_DOMAIN/userConfig.domain, fields that don't exist onUserConfig. Simplified toprocess.env.E2B_DOMAIN || 'e2b.app'.Net: 265 → 249 tests. Offline, everything passes except
template/create.test.ts, which fails the same way onmainwithoutE2B_API_KEY.backend_integrationandexec_pipewere also run live with a key and passed.Kept after review: the
auth.test.tsstale-lock test, because it is the only test that runs the productionCONFIG_LOCK. Also the table-pathbuildTableRows(..., 'desc')test.Not changed (follow-up candidates):
exec_passthrough,exec_stdin_flag,exec_close_stdin,create_lifecycleandproject.test.tsmock the CLI's own modules (exec_helpers,src/terminal,src/utils/urls,src/api). They cover real behavior, but they are coupled to internals and would be better rewritten against a local HTTP server, asattribution.test.tsandtemplate/delete.test.tsalready do.Link to Devin session: https://app.devin.ai/sessions/ba0ccf3e32064be4ad6191007b87c968
Open in Devin Desktop: https://app.devin.ai/desktop/session/ba0ccf3e32064be4ad6191007b87c968?variant=devin
Requested by: @mishushakov
🟢 Devin Review found nothing to fix · View findings