Repository navigation
fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore #1740
Description
PR #1735 repaired 77 unfailable assertions on MockConfigStore. One of its two structural causes is not specific to that mock — it is a property of testify itself, and it leaves assertions on every other mock type in the repo equally unfailable.
The cause
testify diffs an empty expectation against the real arguments and counts each as a difference. So a name-only AssertNotCalled(t, "MethodName") can never match a method that takes parameters, and therefore can never fail — regardless of whether the forbidden call happened.
#1735 fixed this for MockConfigStore by shadowing AssertCalled / AssertNotCalled / AssertNumberOfCalls to read a callLog recorded before any branching. Those shadows are on *MockConfigStore only. Every other mock type still has the underlying behaviour.
Scope, as measured during the #1735 review
- 82 name-only assertion sites repo-wide
- of those, roughly 30 on non-
MockConfigStoretypes are still vacuous
Named examples:
| site | mock |
|---|---|
handler_auth_test.go:968 AssertNotCalled(t, "UpdateUserProfile") |
mockAuth |
six sites naming ApproveExecution |
MockPurchaseManager |
| various | MockProviderFactory, MockSESClient, provider clients |
Both figures come from the #1735 review and are a static count of name-only call sites, not a proven count of dangerous ones. A mock with no short-circuit panics loudly on an unregistered call, so the defect can still fail the test — just not via the assertion. Determining the genuinely-vacuous subset needs the same runtime probe #1735 used, run per mock type. Do not quote 30 as established until that is done.
Why it matters despite the panic caveat
The dangerous subset is where an expectation is also registered for the method — then the call is expected, no panic fires, and the AssertNotCalled guarding it silently passes. That is the exact shape #1735 found 77 instances of.
It also matters for new security tests. During this session a grant-ceiling PR was about to assert "the write was refused, nothing reached the store" via a name-only AssertNotCalled on MockAuthService. It would have been unfailable. It was caught only because #1735's author flagged the class mid-flight.
Suggested approach
- Run fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735's runtime probe per mock type to get the real vacuous count, rather than acting on the 82/30 static figures.
- For each mock type with genuinely vacuous sites, either apply fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735's
callLog+ shadow pattern, or correct the assertions to pass explicit matchers. - Prefer a mechanism that makes the mode impossible over fixing instances. fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735 is separately hardening
matching()to fail loudly on an arity mismatch — a third variant of this same family — and the equivalent here would be a lint or test-time check that rejects name-onlyAssertNotCalledagainst a parameterised method.
Verify per assertion by mutation, as #1735 did: break the behaviour the assertion guards and confirm the test now fails. An assertion that still passes under that mutation is still vacuous and has not been repaired.
Out of scope for #1595 deliberately, to keep that PR reviewable.
Activity
cristim commentedon Aug 8, 2026
Concrete instance found while reviewing PR #1725, and it is a money-path guard:
cmd/multi_service_test.go:1349,:1411,:1486use name-onlyAssertNotCalled(t, "PurchaseCommitment")onMockServiceClient. That method takes three parameters, so all three assertions are vacuous by the mechanism this issue describes.What they are supposed to guard: "a dry run must not purchase." That is the assertion standing between
--dry-runand real money, and it cannot currently fail.Two reasons this instance deserves priority over the general sweep:
&& !isDryRunto a refusal guard on the same path. Its own new refusal test correctly usesassert.Empty(results)rather than a vacuousAssertNotCalled, so that PR is fine — but the surrounding dry-run guards are not.MockServiceClientis outside fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735's fix. That PR shadows the assertion helpers onMockConfigStoreonly, so nothing about it repairs these.Suggested handling when this issue is picked up: do
MockServiceClientfirst, and verify by mutation rather than by correcting the matcher count and assuming. Make a dry run actually purchase, and confirm each of the three tests now fails. If any still passes, the repair is incomplete.Note also that a third structural cause exists beyond the two described in this issue: a non-zero but wrong matcher count also never matches, because
matching()special-cases only the empty case and a wrong arity falls through toDiff, where the missing position always scores as a difference. PR #1735 is being amended to fail loudly on an arity mismatch. Any repair here should pass one matcher per parameter, not merely some matchers.fix(test): make the guard see cross-package mocks instead of skipping…
test(api): drive the routed execute handler, not just the permission …
fix(test): repair an assertion that ran before its call, and scope th…
test(api): drive the routed execute handler, not just the permission …
sec(auth): carve execute:ri-exchange out of admin:* and seed the gran…
fix(test): route unsupported shapes through the path that actually runs
fix(azure): retry a purchase whose retryability could not be read
fix(azure): retry a purchase whose retryability could not be read
fix(azure): surface the read error that decides purchase retry-vs-abo…
fix(test): repair 30 unfailable mock assertions and guard the class (#…