test(scope,executor): two tests took their precondition from the machine (#742) - #743
Merged
Merged
Conversation
…ine (#742) Both passed in CI and failed for any contributor with a configured deployment -- the people most likely to run the suite locally. That is the expensive kind of failure: it teaches you the local suite is unreliable, against a CONTRIBUTING.md that asks for four green gates before review and says the suite is fast enough to run freely. It already cost one round-trip in this repo. **The mechanism is sharper than "the environment leaks in".** CI's empty credential store was silently SUPPLYING a precondition one of the tests needed. `current_credential_fingerprint` is imported independently at three sites -- `commands/scope.py`, `execution/guards.py` (rails 20 and 22), and `execution/executor.py`. `test_confirming_replaces_a_stale_fingerprint_rather_than_ carrying_it_forward` patches only the executor's. With a credential configured, guards' copy resolves a genuine fingerprint, finds it disagrees with the row's "stale000...", and RAIL 20 VETOES THE ENTRY -- so `execute` never reaches the confirm step and the fingerprint stays stale. On CI the unpatched call returns None ("no evidence to disagree with"), no veto fires, and the test passes. The None that made it work came from the machine, not the test. Each test now states its own assumption, in the pattern its own file already uses for every sibling: - `test_scope.py` patches `keel.commands.scope.current_credential_fingerprint` to None, as the three tests above it already patch that same target. What is under test is what the command WRITES when nothing resolves; the "nothing resolves" half belongs in the fixture. - `test_executor.py` patches BOTH sites, deliberately to DIFFERENT values: guards to None so rail 20 has nothing to disagree with and the entry reaches the confirm step, executor to "fresh..." so the overwrite under test is observable. Written into the test, because a reader will otherwise "fix" the asymmetry. **A suite-wide autouse fixture was tried first and is the wrong shape.** Recorded because the idea is tempting: stubbing `_from_keychain` made `test_the_broken_keychain_test_is_not_vacuous` vacuous (it calls that function directly to prove it reaches keyring); pinning `default_env_path` broke five `test_paper_equities_profile.py` tests that legitimately chdir and write their own `.env`; and a narrower `read_secret` wrapper broke twelve, because `test_credential_identity.py` deliberately exercises real `.env`/keychain resolution with exactly those names. This suite tests credential resolution ON PURPOSE, in several places, so a blanket "nothing resolves" fixture fights the tests that exist to exercise it. Verified in the condition that was broken -- the full suite from the repo root with a real `.env` present: 6151 passed, 3 skipped. ruff and mypy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EXz13qp1UM3pBa6BqsRqvC
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.
Closes #742.
Two tests passed in CI and failed for any contributor with a configured deployment — the people most likely to run the suite locally.
The mechanism is sharper than "the environment leaks in"
CI's empty credential store was silently supplying a precondition one of the tests needed.
current_credential_fingerprintis imported independently at three sites —commands/scope.py,execution/guards.py(rails 20 and 22), andexecution/executor.py. The executor test patches only its own. With a credential configured:On CI the unpatched call returns
None— "no evidence to disagree with" — no veto fires, and it passes. TheNonethat made the test work came from the machine, not from the test.The fix
Each test states its own assumption, in the pattern its own file already uses for every sibling test:
test_scope.pypatcheskeel.commands.scope.current_credential_fingerprint→None, exactly as the three tests above it already patch that same target. What's under test is what the command writes when nothing resolves; the "nothing resolves" half belongs in the fixture.test_executor.pypatches both sites, deliberately to different values:guards→Noneso rail 20 has nothing to disagree with and the entry reaches the confirm step;executor→"fresh…"so the overwrite under test is observable. The asymmetry is written into the test, because a reader would otherwise "fix" it.A suite-wide fixture was tried first, and is the wrong shape
Recorded because the idea is tempting and the reasons it fails aren't obvious. Three attempts, three collisions:
_from_keychaintest_the_broken_keychain_test_is_not_vacuousvacuous — it calls that function directly to prove it reacheskeyring.get_passworddefault_env_pathtest_paper_equities_profile.pylegitimatelychdirs and writes its own.envforload_alpaca_secrets, which doesn't useread_secretat allread_secretfor identifier namestest_credential_identity.pydeliberately exercises real.env/keychain resolution using exactly those namesThis suite tests credential resolution on purpose, in several places. A blanket "nothing resolves anywhere" fixture fights the tests that exist to exercise the thing it suppresses. That's why the fix is local — and why the issue's original diagnosis (which blamed
read_secret's three tiers and proposed the fixture) is corrected in a comment there rather than left to mislead.Verification
Run in the condition that was broken — the full suite from the repo root with a real
.envpresent:ruff checkclean ·mypyclean (441 files). Tests only; no library code, no config, no shipped parameter.Left open deliberately
Three import sites resolving ambient credentials independently is a shape that will catch someone again. That's a design question, not a test bug, and it deserves its own issue rather than being smuggled into this one.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EXz13qp1UM3pBa6BqsRqvC