feat: Add experimental async transport (port of PR #4572) #5646
4 issues
code-review: Found 4 issues (3 medium, 1 low)
Medium
httpcore[asyncio] dependency breaks Python 3.6/3.7 test environments - `scripts/populate_tox/tox.jinja:90`
The common: httpcore[asyncio] dependency is added without Python version restrictions, but httpcore 1.x requires Python 3.8+. The common test environment runs on py3.6 and py3.7 (line 21 of template). This will cause pip installation failures in those environments. The dependency should be conditioned on Python 3.8+ similar to how py3.8-common: hypothesis is version-restricted.
Also found at:
sentry_sdk/transport.py:553-559
Test will fail because isinstance check fails on Mock with spec - `tests/integrations/asyncio/test_asyncio.py:667-668`
The test creates mock_transport = Mock(spec=AsyncHttpTransport) and assigns it to mock_client.transport. However, in _flush() (asyncio.py:71), isinstance(client.transport, AsyncHttpTransport) will return False because a Mock object with a spec is not an actual instance of the class. This causes early return, so close_async is never called and the assertions at lines 680-681 will fail.
Also found at:
sentry_sdk/client.py:1057
Missing test cleanup leaves global scope client set after test - `tests/test_transport.py:998`
test_async_transport_concurrent_requests sets the global scope client on line 998 but does not register a cleanup finalizer like the other async tests do. This can cause test pollution - if this test fails or if tests run in a certain order, subsequent tests may use the stale client. Compare to test_transport_works_async (line 928) and test_async_transport_rate_limiting_with_concurrency (line 1024) which both properly clean up.
Also found at:
tests/test_transport.py:1031
Low
Unused helper function _make_async_transport_options is dead code - `tests/test_transport.py:54-72`
The _make_async_transport_options helper function is defined but never called anywhere in the codebase. This appears to be dead code that was introduced but not used by any tests. While not a runtime error, it adds unnecessary maintenance burden and could confuse future developers.
Duration: 7m 42s · Tokens: 5.7M in / 61.2k out · Cost: $9.45 (+extraction: $0.01, +merge: $0.00, +fix_gate: $0.00)
Annotations
Check warning on line 90 in scripts/populate_tox/tox.jinja
sentry-warden / warden: code-review
httpcore[asyncio] dependency breaks Python 3.6/3.7 test environments
The `common: httpcore[asyncio]` dependency is added without Python version restrictions, but httpcore 1.x requires Python 3.8+. The common test environment runs on py3.6 and py3.7 (line 21 of template). This will cause pip installation failures in those environments. The dependency should be conditioned on Python 3.8+ similar to how `py3.8-common: hypothesis` is version-restricted.
Check warning on line 559 in sentry_sdk/transport.py
sentry-warden / warden: code-review
[S8Q-6KA] httpcore[asyncio] dependency breaks Python 3.6/3.7 test environments (additional location)
The `common: httpcore[asyncio]` dependency is added without Python version restrictions, but httpcore 1.x requires Python 3.8+. The common test environment runs on py3.6 and py3.7 (line 21 of template). This will cause pip installation failures in those environments. The dependency should be conditioned on Python 3.8+ similar to how `py3.8-common: hypothesis` is version-restricted.
Check warning on line 668 in tests/integrations/asyncio/test_asyncio.py
sentry-warden / warden: code-review
Test will fail because isinstance check fails on Mock with spec
The test creates `mock_transport = Mock(spec=AsyncHttpTransport)` and assigns it to `mock_client.transport`. However, in `_flush()` (asyncio.py:71), `isinstance(client.transport, AsyncHttpTransport)` will return `False` because a `Mock` object with a `spec` is not an actual instance of the class. This causes early return, so `close_async` is never called and the assertions at lines 680-681 will fail.
Check warning on line 1057 in sentry_sdk/client.py
sentry-warden / warden: code-review
[US2-WBK] Test will fail because isinstance check fails on Mock with spec (additional location)
The test creates `mock_transport = Mock(spec=AsyncHttpTransport)` and assigns it to `mock_client.transport`. However, in `_flush()` (asyncio.py:71), `isinstance(client.transport, AsyncHttpTransport)` will return `False` because a `Mock` object with a `spec` is not an actual instance of the class. This causes early return, so `close_async` is never called and the assertions at lines 680-681 will fail.
Check warning on line 998 in tests/test_transport.py
sentry-warden / warden: code-review
Missing test cleanup leaves global scope client set after test
`test_async_transport_concurrent_requests` sets the global scope client on line 998 but does not register a cleanup finalizer like the other async tests do. This can cause test pollution - if this test fails or if tests run in a certain order, subsequent tests may use the stale client. Compare to `test_transport_works_async` (line 928) and `test_async_transport_rate_limiting_with_concurrency` (line 1024) which both properly clean up.
Check warning on line 1031 in tests/test_transport.py
sentry-warden / warden: code-review
[TSX-PKK] Missing test cleanup leaves global scope client set after test (additional location)
`test_async_transport_concurrent_requests` sets the global scope client on line 998 but does not register a cleanup finalizer like the other async tests do. This can cause test pollution - if this test fails or if tests run in a certain order, subsequent tests may use the stale client. Compare to `test_transport_works_async` (line 928) and `test_async_transport_rate_limiting_with_concurrency` (line 1024) which both properly clean up.