Skip to content

fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore #1740

Closed

Description

@cristim

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-MockConfigStore types 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

  1. 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.
  2. 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.
  3. 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.

Activity

  1. cristim commented on Aug 8, 2026

    @cristim
    MemberAuthor

    Concrete instance found while reviewing PR #1725, and it is a money-path guard:

    cmd/multi_service_test.go:1349, :1411, :1486 use name-only AssertNotCalled(t, "PurchaseCommitment") on MockServiceClient. 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-run and real money, and it cannot currently fail.

    Two reasons this instance deserves priority over the general sweep:

    1. It is adjacent to live work. PR fix(cli): apply --max-instances once run-wide, not per service and region #1725 adds && !isDryRun to a refusal guard on the same path. Its own new refusal test correctly uses assert.Empty(results) rather than a vacuous AssertNotCalled, so that PR is fine — but the surrounding dry-run guards are not.
    2. MockServiceClient is outside fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735's fix. That PR shadows the assertion helpers on MockConfigStore only, so nothing about it repairs these.

    Suggested handling when this issue is picked up: do MockServiceClient first, 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 to Diff, 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.

  2. added a commit that references this issue on Sep 27, 2026
    fd83983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions