Skip to content

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

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

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