test(python-sdk): drop httpcore-era stream reader tests after the pyqwest migration - #1656
test(python-sdk): drop httpcore-era stream reader tests after the pyqwest migration#1656mishushakov wants to merge 1 commit into
Conversation
…west migration `test_file_stream_reader.py` was written against httpcore and never migrated with the rest of the stack. It builds bare `httpx.Client()` instances, so it still passes, but two of its premises no longer describe the shipped path: - `_active_connections()` read `client._transport._pool.connections`, an httpcore-only internal. `PyqwestTransport` has no `_pool` at all. - `request.extensions["timeout"]["read"]` is no longer a per-chunk idle bound. The pyqwest adapter collapses read/write into one whole-operation deadline and exits the timeout scope before the body streams, so it bounds nothing after the response head. The SDK stopped sending per-request timeouts on streamed reads accordingly; idle bounding moved to the dedicated streaming transport, covered by `test_volume_client.py`. Delete the five tests that asserted only httpcore behavior (the two idle-timeout tests, the slow-consumer test, and both abandoned-reader tests) along with the helper. The remaining eight now assert `response.is_closed` — the reader's actual contract, transport-agnostic and stronger than the pool check, since the context-manager tests also assert the response stays open mid-stream. Also remove `tests/bugs/`, whose sole file was a permanently skipped pyautogui repro against the `desktop` template. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
PR SummaryLow Risk Overview
Removes Reviewed by Cursor Bugbot for commit b0a7bbb. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 08dc55b. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.38.4-sydney.0.tgzCLI ( npm install ./e2b-cli-2.16.2-sydney.0.tgzPython SDK ( pip install ./e2b-2.38.0+sydney-py3-none-any.whl |
There was a problem hiding this comment.
LGTM — test-only cleanup, verified against the actual reader contract.
Extended reasoning...
Overview
Test-only PR touching two files under packages/python-sdk/tests/. It deletes 5 httpcore-era tests (and their _active_connections/request.extensions["timeout"] helper plumbing) from test_file_stream_reader.py that asserted behavior specific to the old httpcore transport, re-anchors the remaining 8 tests on response.is_closed, and removes a dead, permanently-skipped tests/bugs/test_envelope_decode.py file. No production code is touched.
Security risks
None. No production code paths, auth, crypto, or permissions are affected.
Level of scrutiny
Low. This is a test-suite cleanup with no runtime behavior change. I verified the core claims directly against the source rather than taking them at face value: FileStreamReader.close()/AsyncFileStreamReader.aclose() in e2b/sandbox/filesystem/filesystem.py call response.close()/response.aclose() respectively, so asserting response.is_closed is indeed the reader's real contract and a faithful (arguably stronger) replacement for the old pool-internals check. I also confirmed that real idle-timeout coverage against the actual pyqwest-backed transport already exists in tests/test_volume_client.py (e.g. test_sync_stream_stall_raises_read_timeout, test_async_explicit_stream_idle_timeout_aborts_stall), so the deleted tests aren't a net loss of coverage for that behavior.
Other factors
The removed tests/bugs/test_envelope_decode.py was unconditionally @pytest.mark.skip'd and exercised an unrelated desktop/pyautogui template, so its removal has no coverage impact. Per CLAUDE.md, test-only PRs don't require a changeset, which matches this PR. No unresolved reviewer comments in the timeline (only bot summaries), and the bug hunting system found no issues.
tests/test_file_stream_reader.pywas written against httpcore and never migrated with the rest of the pyqwest stack — it builds barehttpx.Client()instances, so it still passes green while exercising a transport the SDK no longer ships. Both of its load-bearing premises are dead:_active_connections()readclient._transport._pool.connections(an httpcore-only internal —PyqwestTransporthas no_pool), andrequest.extensions["timeout"]["read"]is no longer a per-chunk idle bound, since the pyqwest adapter collapses read/write into one whole-operation deadline and exits the timeout scope before the body streams — verified directly onPyqwestTransport, where a 0.5s mid-body stall withread=0.05raises noReadTimeout. This deletes the five tests that asserted only httpcore behavior (both idle-timeout tests, the slow-consumer test, both abandoned-reader tests) plus the helper, and re-anchors the remaining eight onresponse.is_closed—FileStreamReader.close()'s actual contract, transport-agnostic and stronger than the pool check, since the context-manager tests now also assert the response stays open mid-stream. Also removestests/bugs/, whose sole file was a permanently@pytest.mark.skip'd pyautogui repro against thedesktoptemplate.The real streaming-idle coverage against actual pyqwest transports already lives in
tests/test_volume_client.py; the SDK stopped sending per-request timeouts on streamed reads for this same reason ine2b/sandbox_sync/filesystem/filesystem.py:207-215.Test-only, so no changeset — matching the repo convention for
test(...)PRs.Verification:
tests/test_file_stream_reader.py8 passed (was 13); full python-sdk unit suite 245 passed;ruff format,ruff checkandty checkclean.Closes SDK-324
🤖 Generated with Claude Code