Parametrise the skip-reason emptiness check over the real derivation - #607
Merged
WilfordGrimley merged 1 commit intoJul 30, 2026
Merged
Conversation
`test_no_declared_value_is_empty_or_whitespace` parametrised over
`EXPECTED_SKIP_REASONS` — the hand-written dict 100 lines above it — and
asserted those literals were non-empty and stripped. That is an assertion
about text typed into the test file, which no implementation change can
falsify.
Verified by mutation: setting `EXTRACTOR_FETCH_FAILED_SKIP_REASON = " "` in
image_evidence.py — a whitespace-only reason, invisible to every consumer of
`CardScanLog.skip_reason`, which is exactly what this test names as the
failure mode — left it green. Three other tests caught it; this one could not.
- parametrised over `_declared()` instead, the values actually read out of
the production modules. The same mutation now produces a red
`test_no_declared_value_is_empty_or_whitespace[ ]`.
- added `test_declared_roster_is_not_empty`, because the parametrised shape
has a second way of going vacuous: if `_declared()` ever returns `{}`
(moved directory, regex that stops matching, glob that finds nothing)
pytest generates zero cases and the suite stays green while checking
nothing. Verified by mutation — pointing CARDPICKER_DIR at a nonexistent
path silently skips the parametrised test and now trips this guard.
- `test_every_declared_skip_reason_value_is_unchanged` is strictly subsumed
by `test_every_declared_constant_name_is_accounted_for` (dict equality
implies key-set equality). Kept, because its failure output names the
changed production string directly where the dict comparison buries it in
a value-to-name-set diff — but its docstring now says it is a diagnostic
and adds no mutation coverage, so it stops being counted as a second check.
Coordination: PR #584 also touches this file. Its edits are in
EXPECTED_SKIP_REASONS (lines 58-100) and appended after line 296; these are at
lines 147-169. Disjoint, and this branch is based on origin/master so either
can merge first.
No production code changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
This was referenced Jul 29, 2026
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.
The finding
test_no_declared_value_is_empty_or_whitespaceparametrised overEXPECTED_SKIP_REASONS— the hand-written dict 100 lines above it — and asserted those literals were non-empty and stripped. That is an assertion about text typed into the test file. No implementation change can falsify it.Mutation, verified: set
EXTRACTOR_FETCH_FAILED_SKIP_REASON = " "inimage_evidence.py. A whitespace-only reason is invisible to every consumer ofCardScanLog.skip_reason— precisely the failure mode this test names. It stayed green. Three other tests in the file caught the mutation; this one could not.What this changes
Parametrised over
_declared()— the values actually read out of the production modules. The same mutation now produces a redtest_no_declared_value_is_empty_or_whitespace[ ].Added
test_declared_roster_is_not_empty. The parametrised shape has a second way of going vacuous: if_declared()ever returns{}(moved directory, regex that stops matching, glob that finds nothing), pytest generates zero cases and the suite stays green while checking nothing. Verified by mutation — pointingCARDPICKER_DIRat a nonexistent path silently skips the parametrised test entirely and now trips this guard.Documented the subsumption.
test_every_declared_skip_reason_value_is_unchangedis strictly subsumed bytest_every_declared_constant_name_is_accounted_for(dict equality implies key-set equality). Kept, because its failure output is the two value sets — which names the changed production string directly, where the dict comparison buries it in a value-to-name-set diff. Its docstring now says it is a diagnostic and adds no mutation coverage of its own, so it stops being counted as a second check.Mutation table
EXTRACTOR_FETCH_FAILED_SKIP_REASON = " "_declared()derivation broken (returns{})test_declared_roster_is_not_empty)Coordination with #584
PR #584 also touches this file. Its edits are in
EXPECTED_SKIP_REASONS(lines 58-100) and appended after line 296; these are at lines 147-169. Disjoint, and this branch is based onorigin/master, so either can merge first without conflict.Verification
test_skip_reason_roster.py— 42 passed (was 41).cardpicker/tests/suite green.🤖 Generated with Claude Code
https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN