test infra: backend suite speedup (Phase A1) — Python 3.12 + sysmon coverage

[JSv4]

Summary

Phase A1 only — Python 3.12 + COVERAGE_CORE=sysmon + dropped django_coverage_plugin. Phase B1 (--dist worksteal + auto-tag) was trialled in the first commit and reverted (commit 399d555a7); see the plan doc §10 post-mortem.

What landed (A1)

• compose/{local,production}/django/Dockerfile: python:3.11.15-slim-bookworm → python:3.12.7-slim-bookworm.
• .envs/.test/.django: COVERAGE_CORE=sysmon — switches coverage.py to sys.monitoring-backed instrumentation (SysMonitor tracer class), ~5-10× cheaper than the legacy C trace function used on 3.11.
• setup.cfg: dropped django_coverage_plugin. Coverage 7.x silently falls back to the legacy C trace path whenever any file-tracer plugin is configured, which would defeat sysmon. The project owns 7 Django templates total (4xx/5xx + a couple of admin views) — a trivial coverage slice for a ~150% wall-time tax.
• Codecov is unaffected — same coverage.xml content, only the collection path changes.

What was reverted (B1)

• --dist worksteal and the xdist_group auto-tag in conftest.py. They exposed 39 pre-existing test-isolation bugs in plain TestCase subclasses that loadscope's class-pinning had been hiding (mostly UniqueViolation on the admin user from migration-seeded users colliding with UserFactory).

Measured CI delta

First commit (A1 + B1, with failures): 41 min → 31:53 (−22%). The failures themselves didn't dominate wall-time; the modest gain reflects that coverage cost was a multiplier on the per-test 17 MB fixture-reload base cost (not the base itself). After A1 lands cleanly, expect a similar ~22% reduction without failures.

Next phase (separately tracked)

A follow-up issue will cover Phase B3 (class-once fixture load for WebsocketFixtureBaseTestCase + per-test TRUNCATE-restore) and the test-isolation sweep needed before worksteal can be re-attempted. See plan doc §10 for the detailed analysis.

Test plan

• CI pytest wall-clock is meaningfully lower than the ~41-min baseline (target ~32 min based on the first commit's data).
• Codecov receives a valid coverage.xml and project/patch numbers are within usual noise.
• No new test failures or flakiness.

[claude]

Code Review

Overview

This PR ships two independent CI speedup levers — A1 (Python 3.12 + sysmon coverage) and B1 (worksteal + auto-tag class-bound state) — and is well-motivated by empirical benchmark data. The changes are minimal, clearly scoped, and the plan doc is a strong artifact. A few items worth addressing before merge.

---

Bugs / Correctness

setUpClass-only overrides are not pinned, and that gap is undocumented

The auto-tag logic in conftest.py pins classes that override setUpTestData or set fixtures. However, several TestCase subclasses in the repo override setUpClass without setUpTestData and without fixtures — for example EmbeddingManagerStoreEmbeddingTest, four classes in test_document_index_tool.py, several in test_embedding_manager.py, and PipelineComponentQueriesTestCase.

Under --dist worksteal, tests from those classes can fan out to multiple workers, and each worker independently calls setUpClass. This is correct (workers are separate processes with isolated DBs), and for the classes I inspected the setup is cheap enough that repeated calls are fine. But it should be documented explicitly in the conftest docstring so the next person who hits a flake from an expensive setUpClass-only class knows where to look.

PipelineComponentQueriesTestCase is the one worth a second look — its setUpClass calls importlib.invalidate_caches() and reloads four pipeline modules. Since workers are isolated processes this is safe, but worth noting.

---

Style / Conventions

CLAUDE.md still documents --dist loadscope throughout

CLAUDE.md has multiple references to the old flag:

• Line 23: "Uses pytest-xdist with 4 workers, --dist loadscope keeps class tests together"
• Lines 24, 27, 30, 36: All example commands use --dist loadscope
• Line 292 (Testing Patterns): "Use --dist loadscope to keep tests from the same class on the same worker (respects setUpClass)"

After this PR lands, developers following CLAUDE.md will use loadscope locally while CI uses worksteal. Should be updated to --dist worksteal with a note that the conftest auto-tagging handles class-bound state pinning.

Plan doc credits Claude Code (violates project rule)

Line 3 of docs/refactor_plans/2026-05-23-test-suite-speedup-A1-B1.md:

**Author:** scrudato@umich.edu (drafted with Claude Code)

CLAUDE.md explicitly says: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." This includes docs checked into the repo.

---

Scope Concern

Production Dockerfile is changed in a "test infra" PR

Both compose/local/django/Dockerfile and compose/production/django/Dockerfile are bumped to 3.12.7. The PR title says "test infra" and the primary motivation is CI speed via sysmon — but the production image bump is a wider change. It is the right call (keep local/prod/CI in sync), and the PR body mentions that the linter job has been on 3.12 for weeks. Still, a line in the test plan noting that the production image build was also validated would help reviewers not mentally skip past it.

---

Positive Highlights

• Empirical grounding: The A/B table in section 6 of the plan doc is exactly the right evidence. Treating ratios rather than absolute extrapolations as the signal is a good note.
• Lazy import in conftest: Importing DjangoTestCase inside the function body is the right call — Django settings must be configured before this import can succeed, and the comment explains why.
• django_coverage_plugin removal rationale: The setup.cfg comment explaining the Coverage 7.x fallback behaviour is worth keeping — it answers the obvious "why was this removed?" question for future readers.
• TransactionTestCase deliberately excluded: The docstring and inline comments make the reasoning clear. This is the trickier part of the logic and it is well-explained.
• pytest-xdist version: 3.8.0 is in requirements/local.txt. --dist worksteal has been available since xdist 2.x, so there is no version compatibility risk.

---

Summary

Approve after: (1) removing the Claude Code credit from the plan doc, and (2) updating CLAUDE.md's local-run examples from --dist loadscope to --dist worksteal. The setUpClass-gap documentation is a nice-to-have rather than a blocker — the current behavior is correct, it is just undocumented.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review — PR #1767: Backend Suite Speedup (Phase A1)

Overview

This PR upgrades the Django image from Python 3.11.15 → 3.12.7, enables COVERAGE_CORE=sysmon in the test environment, and drops django_coverage_plugin — together targeting a ~22% reduction in CI wall-time (~41 min → ~32 min) without any change to test logic or Codecov output. Phase B1 (--dist worksteal) was trialled, found 39 pre-existing test-isolation failures, and correctly reverted. The scope is right-sized and the rationale is impeccably documented.

---

✅ What's done well

• Scoping discipline: Phase B1 was reverted when it surfaced ordering-dependency failures. Shipping Phase A1 cleanly and deferring B1 until the isolation bugs are fixed is the right call.
• setup.cfg comment: The multi-line explanation of why django_coverage_plugin is intentionally absent is exactly the type of non-obvious constraint that CLAUDE.md says to document. Future contributors won't silently re-add it and lose the sysmon benefit.
• Plan document: The docs/refactor_plans/2026-05-23-test-suite-speedup-A1-B1.md is exemplary — measured data, honest post-mortem, and a clear roadmap. Particularly valuable: the §10 analysis of why the local A/B over-extrapolated (coverage was a multiplier on fixture-load cost, not independent of it).
• CHANGELOG entry: Detailed, includes file paths, rationale, and follow-up tracking. Matches the project's changelog conventions.

---

🔍 Issues / Suggestions

1. Misleading comment in compose/production/django/Dockerfile (minor)

+# Kept in sync with compose/local/django/Dockerfile. 3.12 is required for the
+# ``sys.monitoring``-backed coverage path used in CI; see that file's header.
ARG PYTHON_VERSION=3.12.7-slim-bookworm

sysmon and --cov are never invoked in the production image. The "required for coverage" justification is technically accurate for CI but reads as if the production runtime depends on it, which it doesn't. Suggest:

# Kept in sync with compose/local/django/Dockerfile — see that file's header
# for the Python 3.12 version rationale.

2. COVERAGE_CORE=sysmon is a silent no-op on Python < 3.12 (low risk, worth noting)

If someone builds the Django image locally against an older base (e.g. by hand from a different tag) and runs pytest --cov, the env var will be ignored by coverage.py without any warning, and they'll silently get the slower C-trace path. Not a blocker for CI (the Dockerfile is pinned), but worth a one-line note in the env comment:

# WARNING: Requires the Django image to be on Python 3.12+; silently falls back
# to the C-trace path on earlier versions. See compose/local/django/Dockerfile.

The existing comment mentions the requirement, so this is very minor.

3. Plan document version discrepancy (cosmetic)

§4 (Phase A design) says to bump to 3.12-slim-bookworm (floating tag), but the actual implementation correctly pins to 3.12.7-slim-bookworm. The plan doc is a design document and not contractual, but it's slightly misleading for anyone comparing the plan to the diff.

4. No validation of Python 3.12 wheel compatibility in requirements/ (tracking concern)

The PR description relies on "CI's linter job has been running on Python 3.12 for weeks" as the wheel-compatibility proof. The linter job likely covers a subset of the full dependency tree (no psycopg2 native compile, no ML libs like numpy/torch/opencv). This is a tracked risk in §7 of the plan doc — just want to confirm: has the test image been rebuilt against 3.12 in CI for this PR, or will that happen on merge? If not yet validated in a real build, noting it explicitly in the PR checklist would add confidence.

5. Template coverage loss is permanent until coverage.py upstream changes (acknowledged, documented)

The 7 Django templates (4xx/5xx + admin views) are low-risk, but dropping their coverage is a one-way door until upstream coverage.py supports sysmon-compatible plugin tracers. The setup.cfg comment does say "Restore if/when..." which is correct. Consider adding a TODO(coverage-plugin) or a GitHub issue reference so this isn't forgotten indefinitely.

---

Security

No security implications. Pure CI/infrastructure change.

Performance

The stated goal is validated with local A/B data. The §6 post-mortem honestly accounts for why the local 4× speedup didn't extrapolate linearly to the full suite (coverage was a multiplier on fixture-load cost, not additive). The projected ~22% reduction on CI is credible.

Test Coverage

No product code changed; no test additions/removals required. The CI workflow itself is the test vehicle.

---

Verdict

Approve with minor suggestions. The scope is correct (A1 only), the documentation is thorough, and the revert of B1 was the right engineering judgment. The production Dockerfile comment (#1 above) is the only change worth making before merge. Everything else is cosmetic or tracking items.

[claude]

Code Review — PR #1767: Backend Test Suite Speedup (Phase A1)

Overview

This PR upgrades the Django container from Python 3.11.15 to 3.12.7, enables COVERAGE_CORE=sysmon, and drops django_coverage_plugin to unlock sys.monitoring-backed coverage instrumentation (~5–10× cheaper than the C-trace path). A companion Phase B1 (--dist worksteal) was trialled, exposed 39 pre-existing test-isolation bugs, and was reverted. The PR ships only the proven, safe subset (Phase A1) and documents the failure analysis in a plan doc.

The approach is conservative and well-evidenced. The empirical data (156% coverage overhead on Python 3.11, measured 22% CI reduction) makes the rationale solid. Below are specific observations.

---

What's working well

• Evidence-first decision-making. The local A/B table (§6 of the plan doc) is exactly the right level of rigor: three runs against the same warm DB, each isolating one variable. The revert of B1 rather than patching around test failures is the correct call.
• B1 post-mortem (§10) is unusually thorough. Calling out where the projection went wrong — over-extrapolating from 3 hot files, conflating coverage multiplier with fixture-load base cost, auto-tag being both too narrow and insufficient — makes the plan doc genuinely useful for whoever picks up B1 later.
• Test fix is correct. assertIn((...), ("text/javascript", "application/javascript")) is the right way to handle the Python 3.12 MIME-type DB change (RFC 9239 updated the canonical type). The comment explaining both forms are RFC-valid is helpful.
• No magic numbers, constants referenced where appropriate. Changes stay in infrastructure files; no product code is touched.

---

Issues and Suggestions

1. Production Dockerfile comment is misleading (minor)

compose/production/django/Dockerfile now reads:

3.12 is required for the sys.monitoring-backed coverage path used in CI; see that file's header.

sys.monitoring coverage is a CI concern, not a production runtime concern. A production user reading this comment might wonder why their production image is tied to a CI instrumentation requirement. Suggest rewording to something like:

Kept in sync with compose/local/django/Dockerfile. Python 3.12 brings meaningful runtime performance improvements (faster startup, reduced memory overhead) and is the same version the CI and local images use.

The version bump itself is correct — keeping local/production/test images on the same major version avoids subtle divergence bugs. The justification just needs to be production-appropriate.

---

2. setup.cfg — [coverage:run] section now has a dangling comment with no key

After the change, the [coverage:run] block has the explanation comment where plugins = django_coverage_plugin used to be, but no plugins = key at all. That's syntactically valid for coverage.py — an absent key means no plugins — but it's worth an explicit:

plugins =
# django_coverage_plugin intentionally omitted: ...

An empty plugins = makes it unambiguously clear to a future reader (or automation that parses the file) that the omission is deliberate, not accidental. As written, someone scanning with grep -n plugins setup.cfg gets zero results, which looks like the option was never set.

---

3. Plan doc version mismatch (cosmetic)

§4 (Phase A1) reads:

Bump ARG PYTHON_VERSION=3.11.15-slim-bookworm → 3.12-slim-bookworm

The actual Dockerfiles pin to 3.12.7-slim-bookworm (a specific patch), which is the right call. The doc should say 3.12.7-slim-bookworm to match, otherwise someone comparing the plan to the implementation will notice the discrepancy.

---

4. Python 3.12 breaking changes — no explicit sign-off

The PR body mentions "CI's linter job has been running on Python 3.12 for weeks, so dependency compatibility is already proven." That's a reasonable confidence signal, but Python 3.12 removed several stdlib items that were deprecated in 3.10/3.11 (asyncio.coroutine, some distutils paths, imp module, stricter datetime semantics). The PR test plan checks CI pass/fail, which effectively validates this, but it would strengthen the PR to add a one-liner confirming the image rebuild succeeded (or noting it as a prerequisite for merge).

---

5. test_storage_classes.py — touching a pre-existing test (FYI, not a blocker)

Per CLAUDE.md convention, pre-existing tests shouldn't be modified without permission. This change is clearly justified (Python 3.12 changed the MIME type returned for .js files) and the fix is correct, but it's worth a note in the PR description so reviewers know it's intentional rather than incidental cleanup.

---

6. --dist loadscope reverted but conftest.py changes aren't shown

The PR body says Phase B1's conftest.py::pytest_collection_modifyitems was reverted. The diff doesn't show a conftest.py change (suggesting it was reverted cleanly before this PR was opened, or it was never in this branch's current HEAD). This is fine — just confirming the current diff is consistent with the claim. No action needed.

---

Performance Considerations

• Measured delta (22%) is consistent and realistic for A1 alone given the 156% overhead figure. The plan doc §10 honestly recalibrates the earlier 75% projection.
• The serial floor (14-min TestStructuredResponseAPI) is correctly identified as the next binding constraint and deferred to B3. Trying to address it in this PR would have blocked a clean merge.
• -n auto remaining in place is fine. The plan doc's §4 A4 note about cgroup misreporting is a useful future consideration.

---

Security

No concerns. All changes are CI infrastructure: Dockerfile base image, environment variable, coverage config, and workflow comments. No product code, no new inputs, no new network paths.

---

Test Coverage

The only test change (test_storage_classes.py) is a correctness fix necessitated by the Python upgrade — both MIME type values are RFC-valid, so the broader assertion is more correct than the original. No coverage regression.

---

Summary

Approve with minor suggestions. The changes are well-reasoned, the scope is appropriately constrained (B1 reverted rather than shipped broken), and the empirical backing is solid. The main things worth addressing before merge:

1. Reword the production Dockerfile comment to not cite a CI concern as the reason.
2. Consider adding an explicit plugins = (empty) in setup.cfg to make the intentional omission legible to grep and future readers.
3. Fix the 3.12-slim-bookworm → 3.12.7-slim-bookworm version string in §4 of the plan doc.

None of these block the core CI improvement. Items 1 and 3 are cosmetic; item 2 is a minor defensibility improvement.