test(django): Drop span_streaming parametrize where the arm is inert - #7157
test(django): Drop span_streaming parametrize where the arm is inert#7157ericapisani wants to merge 6 commits into
Conversation
The marker was added in 2019 (6326cb6) with no reason or issue reference, and the test now XPASSes across the supported Django range (verified 5.2.16 and 6.x; the already-read-body handling it exercises is stable Django public-contract behavior). A non-strict xfail that always passes can never fail the build, so the test was dead weight that would have silently xpassed a real regression. Removing the marker restores it as a genuine regression guard.
No test references rest_hello (its test was removed in a 2019-era revert cycle); every other registered URL is exercised by the suite.
Empirical audit of all 61 django_db-marked tests in the django suite: 60 are load-bearing (DB queries, login, or session-cookie loads via SessionMiddleware). The cache tests' markers also stay: stripping them broke a pytest-forked x pytest-django teardown invariant. This test is the only one that is genuinely DB-free (DRF view with no ORM work, no session cookie sent) and safe to unmark.
AST-verified unused: capture_events in test_materialized_user_captured (never called), client in 6 raw-cursor executemany tests (no HTTP). Unrequested fixtures are never built, so removal is behavior-neutral; signatures now truthfully describe each test's dependencies.
These tests are event-only: they capture events, never spans, and the span_streaming argument was only ever fed to trace_lifecycle. Both arms therefore exercised identical code paths and asserted identical payloads, so the parametrize doubled the case count without adding coverage. Affected: 18 event-only tests in test_basic.py, the 3 cookie-scrubbing tests in test_data_scrubbing.py, test_set_db_data_custom_backend, and test_cache_spans_get_span_name (a pure unit test of _get_span_description that never referenced the argument). Tests that inspect spans or transactions keep their parametrize.
Codecov Results 📊✅ 103737 passed | ⏭️ 6677 skipped | Total: 110414 | Pass Rate: 93.95% | Execution Time: 365m 30s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2486 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 90.14% 90.13% -0.01%
==========================================
Files 193 193 —
Lines 25183 25183 —
Branches 9176 9176 —
==========================================
+ Hits 22700 22697 -3
- Misses 2483 2486 +3
- Partials 1429 1432 +3Generated by Codecov Action |
Update all Django tests to use the `capture_items("event")` fixture API
instead of the `capture_events()` fixture. The new API returns
items with a `.payload` attribute rather than event objects directly.
There was a problem hiding this comment.
I think some of these go through different code paths depending on whether span streaming is enabled or not, even if they arrive at the same result
So maybe we can simplify the asserts (remove the if span_streaming branching and just leave one branch), but it might still make sense to parametrize so that we exercise both paths
I'll have a closer look to see if I can identify tests where we'd want to keep the parametrize
sentrivana
left a comment
sentrivana
left a comment
There was a problem hiding this comment.
Found 3 tests that use a different code path in span streaming -- we should keep the parametrize on those. Otherwise lgtm
|
|
||
|
|
||
| @pytest.mark.parametrize("span_streaming", [True, False]) | ||
| def test_request_captured( |
There was a problem hiding this comment.
This one I'd keep parametrized, because it tests that transaction is put on events correctly, and that happens differently in streaming/not streaming
|
|
||
|
|
||
| @pytest.mark.parametrize("span_streaming", [True, False]) | ||
| def test_template_tracing_meta( |
There was a problem hiding this comment.
This one tests outgoing tracing headers, so it should also be parametrized on span_streaming or not
| "endpoint", ["rest_permission_denied_exc", "permission_denied_exc"] | ||
| ) | ||
| @pytest.mark.parametrize("span_streaming", [True, False]) | ||
| def test_does_not_capture_403( |
There was a problem hiding this comment.
I'm not 100% clear if this only aims to tests errors or also spans/transactions, but it seems like both according to the capture_items filter. So we should keep the parametrize, and accept span and transaction in capture_items
2 participants
Description
These tests are event-only: they capture events, never spans, and the
span_streamingargument was only ever fed totrace_lifecycle. Both arms exercised identical code paths and asserted identical payloads, so the parametrize doubled the case count without adding coverage.Tests that inspect spans or transactions keep their parametrize.
391 -> 356 cases.
Refs PY-2641
Refs #6975