You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore #1740
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-onlyAssertNotCalled(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.
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.
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-only AssertNotCalled against 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.
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
MockConfigStoreby shadowingAssertCalled/AssertNotCalled/AssertNumberOfCallsto read acallLogrecorded before any branching. Those shadows are on*MockConfigStoreonly. Every other mock type still has the underlying behaviour.Scope, as measured during the #1735 review
MockConfigStoretypes are still vacuousNamed examples:
handler_auth_test.go:968AssertNotCalled(t, "UpdateUserProfile")mockAuthApproveExecutionMockPurchaseManagerMockProviderFactory,MockSESClient, provider clientsBoth 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
AssertNotCalledguarding 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
AssertNotCalledonMockAuthService. It would have been unfailable. It was caught only because #1735's author flagged the class mid-flight.Suggested approach
callLog+ shadow pattern, or correct the assertions to pass explicit matchers.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.