fix(tests): stop the suite reading ambient environment config - #74
Open
ProtocolWarden wants to merge 2 commits into
Open
fix(tests): stop the suite reading ambient environment config#74ProtocolWarden wants to merge 2 commits into
ProtocolWarden wants to merge 2 commits into
Conversation
REPOGRAPH_BOUNDARY_ARTIFACT_FILE leaked from the caller's shell into every test. Two that assert the "no artifact configured" path were contradicted by it: test_reconcile.py::TestAC1SingleSourceOfTruth::test_no_artifact_no_scrub_targets test_boundary_detectors.py::TestB2Required::test_b2_flags_missing_required_boundary_source The exposure was two modules, not the one reported. Worth having checked: the second lives in a file that already knows about the variable — it calls monkeypatch.setenv at line 71 — and still had a test that inherited it. Backwards in the way that matters. The variable is legitimately exported by anyone who runs the audit locally or pushes through .hooks/pre-push, and is absent in CI. So a developer with a WORKING setup saw a red suite while CI stayed green — the failure mode that teaches people to ignore their own results. Fixed with an autouse fixture in tests/conftest.py that clears the variable for every test, rather than a delenv at the two call sites. The defect is that the suite reads ambient config at all; patching the two known victims leaves the next artifact-sensitive test to rediscover it. Tests that WANT the variable still set it explicitly with monkeypatch.setenv, which is unaffected. New tests/test_env_isolation.py pins the fixture — without it a later refactor could drop the fixture and the only symptom would be a suite that passes in CI and fails on the machines of the people most likely to run it. It asserts the isolation list against boundary._ARTIFACT_FILE_ENV rather than a string literal, so renaming the variable in the detector fails the test instead of silently emptying the isolation. (First draft asserted only on os.environ and tripped our own T8 — a test file importing nothing from any src package. Fair catch: it was testing Python, not Custodian.) Verified both directions: 1242 passed, 5 skipped with the variable set and with it unset, identical. Audit clean apart from the pre-existing W2 (core.hooksPath unset in this clone; CI sets it as the audit job's first step). Pre-existing at origin/main — not caused by #72, which observed it and deliberately left it alone to stay focused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ftest CI caught what my local run could not: `from tests.conftest import _AMBIENT_ENV_VARS` failed collection with `No module named 'tests'`. tests/ has no __init__.py, so that name resolves only when the repo root is on sys.path. `python -m pytest` puts it there; the bare `pytest -q` that CI runs does not. I verified through the looser entry point, which hid the breakage — the same mistake in kind as the bug this branch fixes: checking through a path CI does not use. Fixed by inverting the dependency rather than adding tests/__init__.py or importing `conftest` bare. conftest now derives _AMBIENT_ENV_VARS from boundary._ARTIFACT_FILE_ENV, and the test imports only from custodian. There is no test->conftest import left to be fragile, and the single source of truth moved to the module that actually owns the name: renaming it in the detector cannot leave the isolation silently covering nothing. Verified through the CI invocation this time — bare `pytest -q` with the variable set and unset, plus `python -m pytest`: 1241 passed, 5 skipped, identical across all three. Audit clean apart from the pre-existing W2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
REPOGRAPH_BOUNDARY_ARTIFACT_FILEleaked from the caller's shell into every test. Two that assert the "no artifact configured" path were directly contradicted by it:The exposure was two modules, not the one reported. Worth having checked for: the second lives in a file that already knows about the variable — it calls
monkeypatch.setenvat line 71 — and still had a test that inherited it.Why it matters
Backwards in the way that matters. The variable is legitimately exported by anyone who runs the audit locally or pushes through
.hooks/pre-push, and is absent in CI. So a developer with a working setup saw a red suite while CI stayed green — the failure mode that teaches people to ignore their own test results.The fix
An autouse fixture in
tests/conftest.pyclearing the variable for every test, rather than adelenvat the two call sites. The defect is that the suite reads ambient config at all; patching the two known victims leaves the next artifact-sensitive test to rediscover it.Tests that want the variable still set it explicitly with
monkeypatch.setenv— unaffected, and covered by a test.tests/test_env_isolation.pypins the fixture. Without it a later refactor could drop the fixture and the only symptom would be a suite that passes in CI and fails on the machines of the people most likely to run it. It asserts the isolation list againstboundary._ARTIFACT_FILE_ENVrather than a string literal, so renaming the variable in the detector fails the test instead of silently emptying the isolation.(First draft asserted only on
os.environand tripped our own T8 — "test file imports nothing from any src package". Fair catch by our own detector: it was testing Python, not Custodian. Rewritten to assert against the constant the detector actually reads.)Verification
W2(core.hooksPathunset locally; CI sets it as the audit job's first step)Baseline before this PR was 1238 passed / 5 skipped; the four added tests account for the difference.
Pre-existing at
origin/main— not caused by #72, which observed it and deliberately left it alone to stay focused.🤖 Generated with Claude Code