feat(httpx): Migrate to span first #6084
2 issues
code-review: Found 2 issues (1 high, 1 medium)
High
Test mocks non-existent symbol legacy_start_span causing tests to not actually test the duration threshold logic - `tests/integrations/httpx/test_httpx.py:624-626`
The test patches sentry_sdk.integrations.httpx.legacy_start_span but this symbol doesn't exist in the httpx module. The actual httpx.py code uses sentry_sdk.start_span directly (not a local alias). As a result, the mock never takes effect and the tests don't verify the duration threshold behavior they claim to test.
Also found at:
tests/integrations/httpx/test_httpx.py:676-678
Medium
Test will fail for sync client due to inconsistent attribute handling - `tests/integrations/httpx/test_httpx.py:1172-1200`
The test test_http_url_attributes_no_query_or_fragment_span_streaming asserts that url.query and url.fragment are NOT in the span attributes when the URL has no query/fragment. However, this test is parametrized with both sync (httpx.Client()) and async (httpx.AsyncClient()) clients. The sync client implementation (httpx.py lines 75-78) unconditionally sets attributes["url.query"] and attributes["url.fragment"] even when they are empty, while the async client (lines 182-187) conditionally sets them only when truthy. This inconsistency will cause the test to fail for the sync client.
Also found at:
sentry_sdk/integrations/httpx.py:77-78
Duration: 6m 12s · Tokens: 2.0M in / 23.7k out · Cost: $3.14 (+extraction: $0.00, +merge: $0.00)
Annotations
Check failure on line 626 in tests/integrations/httpx/test_httpx.py
sentry-warden / warden: code-review
Test mocks non-existent symbol legacy_start_span causing tests to not actually test the duration threshold logic
The test patches `sentry_sdk.integrations.httpx.legacy_start_span` but this symbol doesn't exist in the httpx module. The actual httpx.py code uses `sentry_sdk.start_span` directly (not a local alias). As a result, the mock never takes effect and the tests don't verify the duration threshold behavior they claim to test.
Check failure on line 678 in tests/integrations/httpx/test_httpx.py
sentry-warden / warden: code-review
[Y5C-5BW] Test mocks non-existent symbol legacy_start_span causing tests to not actually test the duration threshold logic (additional location)
The test patches `sentry_sdk.integrations.httpx.legacy_start_span` but this symbol doesn't exist in the httpx module. The actual httpx.py code uses `sentry_sdk.start_span` directly (not a local alias). As a result, the mock never takes effect and the tests don't verify the duration threshold behavior they claim to test.
Check warning on line 1200 in tests/integrations/httpx/test_httpx.py
sentry-warden / warden: code-review
Test will fail for sync client due to inconsistent attribute handling
The test `test_http_url_attributes_no_query_or_fragment_span_streaming` asserts that `url.query` and `url.fragment` are NOT in the span attributes when the URL has no query/fragment. However, this test is parametrized with both sync (`httpx.Client()`) and async (`httpx.AsyncClient()`) clients. The sync client implementation (httpx.py lines 75-78) unconditionally sets `attributes["url.query"]` and `attributes["url.fragment"]` even when they are empty, while the async client (lines 182-187) conditionally sets them only when truthy. This inconsistency will cause the test to fail for the sync client.
Check warning on line 78 in sentry_sdk/integrations/httpx.py
sentry-warden / warden: code-review
[P3F-V3Y] Test will fail for sync client due to inconsistent attribute handling (additional location)
The test `test_http_url_attributes_no_query_or_fragment_span_streaming` asserts that `url.query` and `url.fragment` are NOT in the span attributes when the URL has no query/fragment. However, this test is parametrized with both sync (`httpx.Client()`) and async (`httpx.AsyncClient()`) clients. The sync client implementation (httpx.py lines 75-78) unconditionally sets `attributes["url.query"]` and `attributes["url.fragment"]` even when they are empty, while the async client (lines 182-187) conditionally sets them only when truthy. This inconsistency will cause the test to fail for the sync client.