fix(tests): guard local venv from silently under-collecting vs. CI (#287) - #308
Merged
Conversation
) A reused local macOS dev venv can drift from uv.lock (e.g. pgvector 0.4.2 vs. the pinned 0.5.0 — the exact pair test_launcher_pins_match_lock.py caught the day before this issue was filed) without any error: psycopg stays importable either way, so pytest.importorskip("psycopg") never fires, but a version-mismatched postgresql-extra package can still change which tests import cleanly at collection time. That produced the reported 6572-vs-6582 local/CI gap, and PR #284 had to work around it by trusting CI's own verified count directly. scripts/check_venv_lock_parity.py compares every installed postgresql-extra package (activated by psycopg being importable) against the version requirements/ci-postgresql.txt pins for the running interpreter. tests_py/conftest.py calls it eagerly, in the same "guard function at module scope, pytest.exit() on failure" style the file already uses for its two data-safety guards — turning a silent, version-drift-driven change in the collected test count into an immediate, actionable session abort before collection ever starts. Achieved: loud, not impossible (nothing can force a re-sync) — a stale postgresql-extra venv now fails fast with the exact mismatched packages and the fix command, instead of quietly reporting a smaller number a contributor trusts over CI's. Mutation-tested (scripts/mutation_check.sh scope): 100/103 mutants killed; 3 survivors are a provable-equivalent split/rsplit/maxsplit choice, given _PIN_RE's own backslash exclusion (documented at the use site, confirmed by construction and a 200k-case fuzz). Closes #287 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #287: a reused local macOS dev venv can collect fewer tests than CI's
hash-pinned install, silently — no error, no skip reason, just a smaller
number a contributor trusts over CI's. That already shipped a false
advertised-count claim twice.
Root cause identified:
pytest.importorskip("psycopg", ...)correctlyskips postgresql-extra tests when the extra is genuinely absent (by
design). The bug is different: when
psycopg(or a sibling package itpulls in, e.g.
pgvector) IS installed but at a versionuv.locknolonger pins, collection can silently produce a different test set — no
error surfaces.
tests_py/scripts/test_launcher_pins_match_lock.pyindependently caught exactly this drift the day before this issue was
filed:
pgvector0.4.2 vs. 0.5.0,psycopg3.3.3 vs. 3.3.4 — confirmingit is a recurring, not hypothetical, failure mode in this repo.
Which of the two (impossible vs. loud) was achieved: LOUD. Nothing can
force a contributor to re-run
uv sync, so "impossible" isn't reachable.What's now true:
scripts/check_venv_lock_parity.pycompares everyinstalled postgresql-extra package (activated by
psycopgbeingimportable — the same signal that already gates the
pytest.importorskipcalls) against the version
requirements/ci-postgresql.txt(CI's owncanonical, hash-pinned install source) pins for the running interpreter.
tests_py/conftest.pycalls it eagerly at module scope — the same"guard function +
pytest.exit()on failure" convention that file alreadyuses for its two data-safety guards (
_guard_against_populated_db,_guard_against_real_data_roots) — so a version-drifted postgresql-extravenv now refuses to run at all, naming the exact mismatched packages and
the fix command, instead of quietly reporting a smaller collected count.
A package that is genuinely absent (SQLite-only dev install) is left
alone — that's
pytest.importorskip's correct, intentional behavior, notthe bug this closes.
Historical note: at the time of investigation this session, both
environments described in the issue (the exact CI run and the exact stale
local venv) had already moved on — the local
.venvhad since beenresynced to the current lock (now matches CI's 6848 exactly), so the
original 6572-vs-6582 delta could not be reproduced bit-for-bit. The
mechanism was instead reproduced directly:
postgresql_extra_drift()wasexercised against the exact
pgvector0.4.2/0.5.0 pair from the siblinglauncher-pins investigation and correctly fires.
Completion Ledger
parse_pinned_versions— simple pinned linetest_parses_a_simple_pinned_lineparse_pinned_versions— comment/hash/blank lines filtered (via regex fall-through, no separate pre-filter — see in-source note)test_skips_comments_hashes_continuations_and_blanksparse_pinned_versions— marker true / falsetest_marker_gated_line_included_when_marker_true,test_marker_gated_line_excluded_when_marker_falseparse_pinned_versions— two-version python-marker fork keeps only the matching branchtest_python_version_fork_keeps_only_the_matching_branchparse_pinned_versions— PEP 503 name normalizationtest_normalizes_underscores_and_case_like_pep_503parse_pinned_versions— a non-matching line does not truncate the rest (continue, not break)test_a_non_matching_line_is_skipped_not_fatal_to_the_restfind_mismatches— versions agree / differ / absent-is-not-a-mismatch / sorted outputtest_no_mismatch_when_versions_agree,test_mismatch_reports_installed_and_pinned_versions,test_absent_package_is_not_a_mismatch,test_mismatches_are_sorted_by_nameinstalled_version— uninstalled / installedtest_returns_none_for_an_uninstalled_distribution,test_returns_the_real_version_for_an_installed_distributionpostgresql_extra_drift— no-op when psycopg absenttest_none_when_psycopg_is_not_installedpostgresql_extra_drift— no-op when versions agreetest_none_when_installed_matches_the_pinned_filepostgresql_extra_drift— exact message on the real issue #287 pair (pgvector 0.4.2 vs 0.5.0)test_reports_the_exact_drift_that_caused_issue_287postgresql_extra_drift— multiple mismatches, one per linetest_multiple_mismatches_are_newline_joinedpostgresql_extra_drift— reads the pinned file withencoding="utf-8"explicitlytest_reads_the_pinned_file_as_utf8_explicitlytest_real_ci_postgresql_txt_parses_to_a_realistic_package_setmain()CLI — OK / drift exit codes and exact printed texttest_main_prints_ok_and_returns_0_when_no_drift,test_main_prints_the_message_and_returns_1_on_drifttests_py/conftest.pystill imports and calls the guard (wiring itself pinned, not just the logic)test_conftest_imports_and_calls_the_guard22 tests total, all passing (
pytest tests_py/scripts/test_check_venv_lock_parity.py -q→22 passed).Gate output (quoted, run against this exact head)
§4 AST size check (300 lines/file, 40 lines/method) — clean:
tests_py/conftest.pydiff is +19/-0 lines only (no restructuring); the file wasalready 501 lines / had 3 pre-existing functions over the 40-line cap BEFORE this
PR (measured against origin/main, unrelated to this change's own logic) — flagged
below, not silently passed over.
ruff (touched files):
Doc claims / badges:
(issue #294 already removed the suite-size claim from prose/JSON docs — nothing to
update here; the monotone badge floor is CI-generated.)
§12 Mutation testing (
scripts/check_venv_lock_parity.py, scoped run viascripts/mutation_check.sh's own pyproject.toml wiring, invoked directly againstthe shared
.venvrather than throughuv runper this task's "nouv sync"constraint):
All 3 are provable-equivalent mutants, documented at the use site (comment block
directly above
parse_pinned_versions):_PIN_RE's own character classes excludebackslash (
[^\s;\\]+), so.match()— which only needs a conforming PREFIX, not afull-string match — always halts at the first backslash regardless of which
split/rsplit/maxsplit variant produced the string. Verified both by construction and
by a 200k-case property fuzz across all three variants against the real
requirements/ci-postgresql.txt(identical output every time), not just assertedfrom reading the regex. 6 earlier equivalent survivors (a redundant
if not line or line.startswith(...)pre-filter, itself equivalent for the same reason) wereremoved as dead code rather than documented, per §9/§12.1 ("mutation testing
routinely surfaces dead code — remove it, don't test it").
Full local suite (
.venvshared with the live plugin dir, resolves thisworktree's own
mcp_server/scriptssince cwd wins over the venv's stale editablepointer):
No regressions. CI's own canonical count on
origin/mainHEAD at investigationtime (
Test (Python 3.12), run 30551542735) was 6826 before this PR's 22 newtests; this PR's local collected count (6848 = 6826 + 22) matches that baseline
exactly, confirming the shared
.venvcurrently has no drift of its own — theguard added here is a fast-fail net for FUTURE drift, not a fix for a drift
demonstrated live in this PR.
No conflict markers;
.bestpractices.jsonparses — confirmed.Boy-scout check (coding-standards.md §14)
tests_py/conftest.pyis 501 lines (over both therepo's local 300-line cap and the global 500-line cap) with 3 pre-existing
functions over the 40-line cap (
_drop_dead_orphaned_databases,_guard_against_populated_db,_guard_against_real_data_roots) — all measuredagainst
origin/mainbefore this PR's edit, i.e. not introduced here. ThisPR's own touch to that file is additive-only (+19/-0 lines: one import, one new
12-line guard function under the cap, one call site) and does not make the
pre-existing violation materially worse in kind.
tests_py/conftest.pyis a separate, higher-risk undertaking (the file's own comments document a live
data-loss incident, issue test_consolidate binds to the developer's real store: suite hangs at 58%, and can mutate production data #219, from getting isolation ordering wrong) that does
not belong mixed into a release-blocker fix. Done instead as its own PR:
refactor(tests): split conftest.py under the 300-line cap (boy-scout follow-up) #310 (
split-conftest-size-cap-boyscout) — conftest.py 501 -> 281 lines,split into
tests_py/_pg_throwaway_db.py+_pg_safety_guards.py+_domain_mapping_isolation.py, zero behavior change, 204/211 mutants killed(7 documented equivalent), full suite green (6854 passed, 5 skipped).
ruff/mutation/full-suite all clean ontouched files.
Rules compliance (coding-standards.md, evaluated against the version quoted in
this session's system prompt — no
plugin-version-check.shavailable in thischeckout to stamp a tool-reported version string)
check_venv_lock_parity.pydoes one thing: parse pins + compare + report.conftest.pytouch is a single guard call following its own established patternconftest.pyviolation flagged, not fixed here (see Boy-scout)find_mismatches(pinned, installed_version)takes the version-lookup as an injected callable — the seam every test exercises without touching the real environmentCONTRIBUTING.md/pip_constraint_sets.py; no invented numbersHand-offs
None.
Closes #287