Skip to content

test/pytest: an A/B whose arms agree measures nothing (#432) - #936

Merged
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:audit/432-ab-arms-must-differ
Sep 10, 2026
Merged

test/pytest: an A/B whose arms agree measures nothing (#432)#936
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:audit/432-ab-arms-must-differ

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

mutation-arm-unobservable — entry 3 on VACUITY_MODES.md's own list of what to add next. Both arms of an A/B produce the identical answer and both are green, because the assertion that would catch it is the one nobody writes.

Measured before building. The layer had eight helpers asserting equality and one asserting inequality — ordering_observable, specific to a forward/reverse pair. The general case was hand-rolled, and that idiom throws both values away: expect.num(int(after != before), 1, ...) reports got 0 want 1 when it fails, so a reader cannot tell arms that were both empty from arms that were both wrong from arms correctly identical. Three defects, one message.

Two failed queries are not two arms — the inverse of #930's trap

query_error() makes each failure unique precisely so two failures cannot compare equal and pass an equality assertion. That uniqueness makes them compare unequal, so an arms-differ assertion passes on a pair of statements that both blew up. The fix for one direction opened the other.

query_error() #1: 'QUERY_ERROR.1.first statement blew up'
query_error() #2: 'QUERY_ERROR.2.second statement blew up'
equal?  False      differ?  True   <- an arms-differ assertion would PASS

differ is declared in test_failed_query_sentinel.py's shape table, so the sentinel sweep covers it and a future assertion cannot be added without an entry.

A helper nobody must use is a convention, so the old spelling is now unavailable

An AST scan refuses int(<comparison>) passed to an expect call — closing the class rather than the three instances. AST and not a line regex, for the reason the pytest.raises scan records: the two paragraphs in this tree that describe the idiom quote it verbatim, so a text sweep flags its own documentation.

The scan found a site my manual count missed, which is the whole argument for it. I grepped for before/after naming and found two. The scan found three — the third spelled int(stated == disk) == 0, the same assertion with the comparison inverted, which no search for != would reach. All three are converted; the corpus is zero across 17 files.

Narrowed, not closed — and the document says so

What has gone is writing the assertion wrongly. What remains is not writing it at all: the layer cannot know which two values in a test are arms, so a test that runs an A/B and asserts nothing about the pair is still vacuous. The refused count therefore stays 28 rather than moving to 29, following the precedent §3.7 set for guard-as-teardown-fixture-still-reports-passed.

Prove by removal

mutation test_layer.py the converted sites
control 25 passed 55 passed
failed-query refusal removed 2 failed 55 passed
never refuses equal arms 2 failed 55 passed
differ does not count 2 failed 55 passed
scan ignores the int(a == b) form 1 failed 55 passed
scan ignores the expect context 1 failed 55 passed

Two of my own arms could not fail, both found by mutating rather than reading

  • The scan's Eq branch was unpinned. My planted offence used only !=, so dropping ast.Eq changed nothing — and that is the spelling the missed site used.
  • The expect-context condition was unpinned. No fixture passed int(a != b) to a call that is not an assertion, so dropping the condition changed nothing. The scan's subject is assertions rather than arithmetic: print(int(a != b)) asserts nothing and flagging it would be a false red.

And one arm passed before the feature existed: it matched a value written in the test body, and pytest prints the failing function's source in the traceback, so it was satisfied by its own fixture rather than by anything the guard produced. The new arms use expect.refusal, which anchors each pattern to an E line, one token per call.

Recorded, not fixed

wrote(cur, want, name) sits outside the sentinel sweep because the derivation keys on the first two parameter names. That happens to be the right answer — its left side is a cursor, so a sentinel cannot arrive there — but a future assertion whose first parameter is not called got would be excluded just as silently and for no good reason. A declared exclusion list with a reason per entry is the fix; it is not in this PR.

Gate

harness_selftest   588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED
driver-free job     9 files, 170 passed, psycopg absent from the venv
full corpus       247 passed with a cluster on pg18a

Based on d0ac4f5d (current main). Independent of #935 — that closes a different mode, and this was replayed onto main rather than stacked so neither review depends on the other.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

)

`mutation-arm-unobservable`, entry 3 on VACUITY_MODES.md's own list of what to add
next: both arms of an A/B produce the identical answer and both are green, because
the assertion that would catch it is the one nobody writes.

MEASURED BEFORE BUILDING. The layer had EIGHT helpers asserting equality and ONE
asserting inequality -- `ordering_observable`, specific to a forward/reverse pair.
The general case was hand-rolled, and that idiom throws BOTH VALUES AWAY:
`expect.num(int(after != before), 1, ...)` reports `got 0 want 1` when it fails, so a
reader cannot tell arms that were both empty from arms that were both wrong from arms
correctly identical. Three defects, one message.

TWO FAILED QUERIES ARE NOT TWO ARMS, and that refusal is the INVERSE of the one commandprompt#930
added. `query_error()` makes each failure unique precisely so two failures cannot
compare EQUAL and pass an equality assertion -- which makes them compare UNEQUAL, so
an arms-differ assertion passes on a pair of statements that both blew up. The fix for
one direction opened the other. Measured: two calls give `QUERY_ERROR.1.<detail>` and
`QUERY_ERROR.2.<detail>`, which are `!=`. `differ` is declared in
test_failed_query_sentinel.py's shape table, so the sentinel sweep covers it and a
future assertion cannot be added without an entry.

A HELPER NOBODY MUST USE IS A CONVENTION, so the old spelling is now unavailable. An
AST scan refuses `int(<comparison>)` passed to an expect call, which closes the class
rather than the instances. AST and not a line regex: the two paragraphs in this tree
that DESCRIBE the idiom quote it verbatim, so a text sweep flags its own
documentation -- the same trap the `pytest.raises` scan records.

THE SCAN FOUND A SITE MY MANUAL COUNT MISSED, which is the whole argument for it. I
grepped for before/after naming and found two sites. The scan found three: the third
was in test_docs_cover_the_corpus.py and spelled `int(stated == disk) == 0` -- the same
assertion with the comparison inverted, which no search for `!=` would reach. All three
are converted, and the corpus is now zero across 17 files.

NARROWED, NOT CLOSED, and the document says so. What has gone is writing the assertion
wrongly. What remains is not writing it at all: the layer cannot know which two values
in a test are arms, so a test that runs an A/B and asserts nothing about the pair is
still vacuous. The refused count therefore stays 28 rather than moving to 29.

Prove by removal, five mutations, each by exact string with a parse assertion:

                                     test_layer.py    the converted sites
    control                          25 passed        55 passed
    failed-query refusal removed      2 failed        55 passed
    never refuses equal arms          2 failed        55 passed
    differ does not count             2 failed        55 passed
    scan ignores the int(a == b) form 1 failed        55 passed
    scan ignores the expect context   1 failed        55 passed

TWO OF MY OWN ARMS COULD NOT FAIL, both found by mutating rather than reading:

  * The scan's Eq branch was unpinned -- my planted offence used only `!=`, so
    dropping `ast.Eq` changed nothing. That is the spelling the missed site used.
  * The expect-context condition was unpinned -- no fixture passed `int(a != b)` to a
    call that is NOT an assertion, so dropping the condition changed nothing. The
    scan's subject is assertions rather than arithmetic: `print(int(a != b))` asserts
    nothing and flagging it would be a false red.

And one arm passed before the feature existed: it matched a value written in the test
body, and pytest prints the failing function's SOURCE in the traceback, so it was
satisfied by its own fixture rather than by anything the guard produced. The new arms
use `expect.refusal`, which anchors each pattern to an `E` line, one token per call.

RECORDED, NOT FIXED: `wrote(cur, want, name)` is outside the sentinel sweep because
the derivation keys on the first two parameter NAMES. That happens to be the right
answer -- its left side is a cursor, so a sentinel cannot arrive there -- but a future
assertion whose first parameter is not called `got` would be excluded just as silently
and for no good reason. A declared exclusion list with a reason per entry is the fix.

Gate:
    harness_selftest   588 checks, 588 passed + 0 failed + 0 unrunnable, PASSED
    driver-free job    10 files, 183 passed, psycopg absent from the venv
    full corpus        262 passed with a cluster on pg18a

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 75bbf640, 13/13. The headline finding is correct and it is a hole my own approval opened, which is the part of this I want on the record most.

The inversion is real

query_error() makes each failure unique so two failures cannot compare equal and pass an equality assertion. Measured on this head, that same uniqueness makes them compare unequal:

'QUERY_ERROR.1.first statement blew up'
'QUERY_ERROR.2.second statement blew up'
equal? False    differ? True   -> an arms-differ assertion would PASS

I approved #930 on the strength of the equality direction and did not ask what the fix made possible. "A fix for one direction opening the other" is the right thing to have written down, and it belongs in the tree rather than in a PR description.

differ closes it, and does not over-close it. Five probes, controls included:

failed query on the left arm    REFUSED  failed-query-is-not-an-arm
failed query on the right arm   REFUSED
BOTH arms failed queries        REFUSED
legitimately different values   GREEN     <- no false positive
identical values                RED       arms-do-not-differ

Declaring it in test_failed_query_sentinel.py's shape table is the right move: it means the sentinel sweep covers it and nobody can add an assertion without an entry, rather than this being a thing you remembered to do.

The scan earns the argument for scanning

The third site is the whole case. It was not != at all:

- expect.num(int(stated_totals(doc) == (...)), 0, "a stated total that disagrees with disk...")
+ expect.differ(stated_totals(doc), (...), "...")

An equality asserted to be zero. No search for != reaches it, and your manual count of two missed it. Driving the scan over the corpus myself: 0 offences across 16 files, and it discriminates:

planted `int(a != b)` in an expect call   -> found
planted `int(a == b)` in an expect call   -> found   <- the spelling a != search misses
bare `print(int(a != b))`                 -> not found  <- arithmetic, not an assertion

Both conditions are load-bearing, and I mutated rather than read:

drop ast.Eq          -> FAILED test_the_inequality_scan_finds_a_planted_offence
drop the is_expect test -> FAILED test_the_inequality_scan_does_not_flag_honest_code

Those are the two arms you found unpinned. They bite now, each by its own name.

Composition, since main moved under you

#935 landed while I was reviewing this, so I composed locally. VACUITY_MODES.md conflicts — yours to resolve — but the file worth checking is the one that did not conflict: pgc_vacuity.py auto-merged, and both PRs add methods to it. Union'ing the doc regions purely to make it parse, the composed tree gives:

test_layer.py + test_writes_wrote_rows.py + test_failed_query_sentinel.py   48 passed
the whole driver-free job                                                   10 files, 183 passed

So the auto-merge is sound. Re-run it yourself after the real resolution rather than taking mine — I union'd the doc, which is a test, not a resolution.

Two notes, neither blocking

The function name is narrower than its subject. _hand_rolled_inequalities also flags expect.num(int(a == b), 1, ...) — an equality assertion. I think flagging it is right, because the vice is int(<Compare>) discarding both values so a failure reads got 0 want 1, and that is true in either direction. But the docstring says so and the name does not, and the name is what the next reader greps for.

"Narrowed, not closed" is the correct call and I am glad you followed the §3.7 precedent rather than moving the count. What remains — nothing tells the layer which two values in a test are arms, so a test that runs an A/B and asserts nothing about the pair is still vacuous — is stated where it will be read. Refused count untouched at 27; I checked rather than trusted.

I will take the _comparisons() exclusion-list question as its own issue, as I said on #935: a declared list is the shape this layer argues against, so the answer is to derive the exclusion and assert the partition.

# Conflicts:
#	test/pytest/VACUITY_MODES.md
@jdatcmd
jdatcmd merged commit cfe1fde into commandprompt:main Sep 10, 2026
13 checks passed
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants