Repository navigation
test: drop Code Interpreter server-behavior tests now covered in e2b-dev/code-interpreter - #1956
Conversation
…dev/code-interpreter 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 SummaryThis 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. |
![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 (e2b-dev/sdk-harness). This PR only touches tests and CI, so I checked the rules a test change can affect: T-1/T-1a/T-2 (JS / sync-Python / async-Python parity of the tests that remain or move), T-59 (InvalidArgumentError/InvalidArgumentException for pre-operation validation), and T-58/T-60 (specific error classes in assertions). Most of the diff deletes server-behavior tests and removes the matching systemd excludes from the workflows. No TASTE rule covers that.
Violations: 1
packages/code-interpreter-js/tests/contexts.test.ts: the relocatedpass context and languagetest callsrunCodewith the wrong argument shape and accepts any rejection. It never reaches the client-sideInvalidArgumentErrorguard, unlike its Python mirrors in this PR (T-59, T-1). Details are in the inline comment.
The Python test_pass_context_and_language additions (sync + async) and the conftest.py / setup.ts helper removals comply.
Package ArtifactsBuilt from d9cca20. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.0.1-devin-1791375995-trim-ci-sdk-tests.0.tgz ./e2b-2.53.2-devin-1791375995-trim-ci-sdk-tests.0.tgzCLI ( npm install ./e2b-cli-2.21.2-devin-1791375995-trim-ci-sdk-tests.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.3-devin-1791375995-trim-ci-sdk-tests.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-devin-1791375995-trim-ci-sdk-tests.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.0.0+devin.1791375995.trim.ci.sdk.tests-py3-none-any.whl ./e2b-2.53.1+devin.1791375995.trim.ci.sdk.tests-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.3+devin.1791375995.trim.ci.sdk.tests-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.6.1+devin.1791375995.trim.ci.sdk.tests-py3-none-any.whl |
There was a problem hiding this comment.
🔍 Devin Review: 2 flags
Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it deletes a very large amount of test coverage (~3000 lines, 66 files) across both SDKs based on a claim that the coverage moved to another repository, a human look would still be worthwhile to confirm that migration actually happened before this coverage is lost here.
What was reviewed: confirmed no src/ changes accompany the test deletions; verified the CI workflow edits (code_interpreter_js_tests.yml, code_interpreter_python_tests.yml) correctly stop excluding the now-removed systemd test files; checked that the removed wait_for_kernel/waitForKernel readiness helpers in conftest.py/setup.ts have no remaining callers after the java/r kernel tests were deleted; verified the new contexts tests (JS and both Python sync/async) correctly preserve the one SDK-surface assertion that was in the deleted test_kernels files.
Extended reasoning...
Pure test-suite deletion (66 files removed, 2 small files added) plus matching CI workflow cleanup across packages/code-interpreter-python and packages/code-interpreter-js; no src/ or production code is touched, so there is no injection/auth/data-exposure surface here. The mechanics are internally consistent (no dangling helper references, CI exclusions updated to match deletions, the one real SDK assertion preserved in new contexts tests), which supports the PR's own description. The decisive factor for defer rather than approve is scale and policy risk: deleting ~3000 lines of behavior-test coverage based on an external claim that it was ported to a sibling repository is a large judgment call the repo's CLAUDE.md conventions flag as worth scrutinizing, and that verification (does e2b-dev/code-interpreter#345 actually cover this) is outside what this diff alone can confirm.
![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: 58e6b4c7d6
ℹ️ 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".
…rough and chart deserialization coverage Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
can you check Devin's two flags review? |
|
Both flags were against the first commit (58e6b4c) and are already fixed in 7e178ad:
The re-review on 7e178ad reports 0 flags. |
1 participant
Summary
Follow-up to e2b-dev/code-interpreter#345, which ports the Code Interpreter behavior suite into the template repo as a direct-HTTP (httpx) suite. The SDK suites in
packages/code-interpreter-pythonandpackages/code-interpreter-jsnow only test SDK surface; everything that asserted on server/template behavior is removed here (~-2900 LOC, nosrc/changes).Removed (py sync + async, js):
charts/*live chart-shape tests (the Pythoncharts/test_json.pyto_jsontest stays),test_default_kernels/defaultKernels(js/ts/r/java/bash hello-worlds),test_cwd,test_bash,env_vars/*(per-language × sandbox-level/per-execution/overwrite),test_statefulness,test_execution_count,test_display_data,test_data,test_custom_repr_object,images/*,test_systemd, andtest_kernels(its one SDK check,test_pass_context_and_language→InvalidArgumentException/InvalidArgumentError, moved intotest_contexts/contexts.test.ts).With the java/r kernel tests gone, the
_wait_for_kernel*/waitForKernelreadiness-retry helpers and their unit tests (test_java_kernel_readiness.py,javaKernelReadiness.test.ts) have no callers and are removed fromconftest.py/setup.tstoo.Replaced with SDK-focused coverage (from review):
tests/{sync,async}/test_env_vars.py,tests/envVars.test.ts— one per-executionenvs→env_varspass-through test per implementation (the HTTP suite can't see the SDK's request mapping).tests/charts/test_deserialize.py— fixture-based unit tests for_deserialize_chart/Result(chart=…)/to_jsoncovering line, scatter, bar, pie, box-and-whisker, nested superchart, unknown type and unknown scale. No JS counterpart:Resultin JS stores the rawchartpayload, there is no deserialization step to test.Kept: basic (incl. secure-traffic), callbacks, contexts, interrupt, killed, reconnect, streaming, messaging/utils/sandbox-url/timeout/exception-format unit tests,
performance.py.CI:
code_interpreter_{python,js}_tests.ymlrun_recovery_tests=falsepaths no longer ignore the deleted systemd files (interrupt is the only remaining recovery suite).Verified locally against
code-interpreter-v1: Python 62 → now 77 incl. the new unit tests, JS 38 passed.Linear: SDK-386
Link to Devin session: https://app.devin.ai/sessions/6fd302f062cc459796016eba4e06d9e1
Open in Devin Desktop: https://app.devin.ai/desktop/session/6fd302f062cc459796016eba4e06d9e1?variant=devin
Requested by: @mishushakov