fix(test): repair 30 unfailable mock assertions and guard the class - #1750
Conversation
testify diffs the matchers an assertion is given against the arguments a mock recorded, and diffs an empty matcher list too, counting each real argument as a difference. A name-only AssertNotCalled(t, "Method") can therefore never match a method that passes arguments to m.Called: it reports success whatever the code under test did. PR #1735 fixed this for MockConfigStore by shadowing the helpers; every other mock still had it. Repaired 30 sites across 11 mock types by passing each method's real m.Called arity as matchers. The three highest-stakes are the dry-run guards in cmd/multi_service_test.go, which assert that --dry-run does not reach PurchaseCommitment; they could not fail. Every site was mutation-verified in both directions: with the forbidden call recorded on the mock, the name-only form passes (23+4 of 27) and the repaired form fails (27 of 27). The three dry-run guards were verified against a real production mutation making processPurchaseLoop purchase on the dry-run branch: the assertion fires only once it carries three matchers. Of the 30, exactly one had a registered expectation and so silently passed; the other 29 were backstopped by testify's unexpected-call panic, which fails the test loudly but at the mock rather than at the invariant. That backstop is accidental: it disappears the moment anyone registers an expectation for the method. TestNoUnfailableMockAssertions closes the class for code not yet written. It parses every package, derives each mock method's m.Called arity, and rejects any AssertCalled / AssertNotCalled whose matcher count cannot match. Mocks opt out by defining their own helpers, which is detected rather than hardcoded, so MockConfigStore's callLog shadows keep their name-only meaning and a future mock adopting the pattern is covered automatically. Sites whose receiver cannot be resolved are reported, not skipped, so a blind spot cannot hide behind a pass. TestVacuousAssertionGuardDetects exercises that analysis on synthetic source: name-only, wrong count, correct count, zero-arity method, a shadowed type, a non-mock, an unresolvable receiver and a variadic spread whose length is not knowable. Without it the guard could break into reporting nothing and the repo-wide run would still look green, which is the failure mode this whole class is about. The four providers/aws sites were found only by the guard: the repo is a Go workspace, and a type-checked sweep of the root module cannot see the other five modules. Closes #1740
cristim
added
triaged
📝 WalkthroughWalkthroughThe tests now provide argument matchers for negative mock assertions. A new AST-based guard scans repository tests for vacuous or unanalyzable assertions and includes synthetic coverage for resolution and arity cases. ChangesMock assertion hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Adversarial review, head
|
| gate | result |
|---|---|
root go build ./... / go vet ./... |
clean |
root go test -race -count=1 ./... |
exit 0, 32 packages ok, 0 FAIL |
providers/aws go test -race -count=1 ./... |
exit 0, 12 packages ok, 0 FAIL |
gocyclo -over 10 -ignore "_test\.go" . |
clean |
golangci-lint v2.10.1, providers/aws |
272 findings, set-identical base vs head (below) |
CI at this head: 19 checks, all completed / success, no nulls. Per #1751 I did not treat that as evidence for providers/* — I ran that module myself.
1. The money path: verified against a real production mutation
Rather than injecting a call, I mutated production so a dry run actually hits the purchase API — the exact bug these three assertions exist to catch. In cmd/multi_service.go, inside the if isDryRun branch:
result = createDryRunResult(rec, region, j+1, cfg)
// MUTATION: a "dry run" that nevertheless hits the purchase API.
_, _ = serviceClient.PurchaseCommitment(ctx, rec, common.PurchaseOptions{})With one permissive expectation registered so the assertion rather than testify's panic decides the outcome (disclosed; TestProcessPurchaseLoopDryRun registers none of its own):
| assertion form | result under the identical production mutation |
|---|---|
head, AssertNotCalled(t, "PurchaseCommitment", mock.Anything ×3) |
FAIL — Expected "PurchaseCommitment" to not have been called with: [mock.Anything mock.Anything mock.Anything] but actually it was. |
pre-PR, AssertNotCalled(t, "PurchaseCommitment") |
ok — passes silently |
PurchaseCommitment calls m.Called(ctx, rec, opts) = 3, and the fixed sites pass 3. The claim holds against the real thing, not just the injected one.
2. The count, and the honesty of the 1-vs-29 split
I did not re-implement the sweep — I ran the PR's own guard against pre-PR main, which is a stronger check because it validates the guard and the count in one move:
guard on origin/main: checked 125 sites, 30 unfailable mock assertion(s) EXIT=1
guard on 457ea300d: checked 125 sites, 0 findings EXIT=0
Exactly 30, independently. The findings include the four in providers/aws/services/savingsplans/client_test.go (390, 976, 1032, 1535), which is also the first self-correction verified: the walk really does reach the other modules.
On the 1-vs-29 split, my first pass contradicted the claim, so I went and read the code rather than trusting either result. A regex for .On("<method>" inside the enclosing test found 2 sites — and neither was :971. Both directions turned out wrong, for instructive reasons:
handler_ri_exchange_test.go:971is genuinely silent, and my regex missed it because the expectation is registered by a helper:ownsAzureSource(opsClient)registersListExchangeableReservationswith.Maybe(). The test's own comment says so.- The two
savingsplanshits are false positives. The.On(...)is registered only underif !tt.expectError, while theAssertNotCalledruns only underif tt.expectError— mutually exclusive branches, with a freshmockSPper subtest. In the subtest where the assertion runs, nothing is registered, som.Calledwould panic.
So the split really is 1 genuinely silent, 29 panic-backstopped. The author's refusal to write this up as "30 caught bugs" is accurate, not just modest.
Spot-checking the backstop, four of the 29 mocks reach m.Called unconditionally with no isExpected short-circuit: MockPurchaseManager.ApproveExecution, MockSESClient.SendEmail, MockProviderFactory.CreateAndValidateProvider, MockSavingsPlansClient.DescribeSavingsPlansOfferings.
3. Arity from m.Called, not the signature
Verified on the two shapes that produced the false findings: MockSESClient.SendEmail takes three parameters and calls m.Called(ctx, input); DescribeSavingsPlansOfferings likewise. Both fixed sites pass 2 matchers, matching m.Called. Deriving from the signature would have flagged correct assertions — the failure mode that gets a guard deleted — and it does not.
Related: no mock in the repo uses a variadic m.Called(args...) spread, so the guard's len(call.Args) cannot be miscounted into a false finding today. It does not guard against that shape appearing later, but nothing is currently exposed. (I sanity-checked the search pattern against a line I knew matched, per the git grep -E trap.)
4. The injection technique is faithful for what the assertion reads
m.Called appends *newCall(m, methodName, callerInfo, arguments, returnArgs) to m.Calls (mock.go:558), and methodWasCalled consults only Method and Arguments. Appending a mock.Call{Method:…, Arguments:…} therefore reproduces exactly the state the assertion inspects; the untouched fields are never read on this path. It cannot detect a defect in the mock's own dispatch, but that is not the property under test — and the money path, where it matters most, I verified with a real production mutation instead.
5. The guard, attacked directly
End-to-end red. Injected a fresh name-only assertion on a same-package mock:
mockPurchase := new(MockPurchaseManager)
mockPurchase.AssertNotCalled(t, "ApproveExecution")→ FAIL: 1 unfailable mock assertion(s). It fires.
checked == 0 is a hard failure. Broke mock detection and re-ran: FAIL … guard checked no assertion sites; its mock or receiver detection has broken. Confirmed by execution, not by reading.
Unresolved receivers are reported, not skipped — t.Errorf on a non-empty skipped list. Narrowed by unresolvedLooksRisky (see F4).
Variadic on the assertion side is deliberately skipped (call.Ellipsis.IsValid()), documented, and exercised in the self-test. It is a blind spot by construction rather than a miscount, which is the right trade.
Six modules. repoRoot walks up to the nearest go.mod (the repo root, since internal/ has none) and filepath.WalkDirs from there — a filesystem walk, not packages.Load. Demonstrated by the four providers/aws findings above.
Worth stating as a positive: because it walks the filesystem rather than relying on the test runner, the guard is immune to the #1751 CI gap. It analyzes providers/*, pkg and tests/e2e even though CI has never run their tests. That is precisely how the four savingsplans sites surfaced.
F1 (the finding worth acting on) The guard cannot see cross-package mocks, so MockConfigStore's exemption is accidental rather than the designed opt-out
collectMocks builds its mock map per directory. MockConfigStore is declared in internal/mocks and aliased into internal/api, internal/purchase and internal/scheduler (type MockConfigStore = mocks.MockConfigStore — the repo's only cross-package alias). In those packages the guard never finds its methods, so resolveRecvType succeeds, isMock is false, and the site takes the resolved && !isMock → continue path: silently skipped, not counted in checked, not reported in skipped.
Proven by execution. I renamed MockConfigStore's three shadowed helpers away, so the designed opt-out no longer applies, and re-ran:
name-only sites in internal/api + internal/purchase: 50
guard findings with the shadow removed: 6 (checked 125 -> 140)
Only the six in-package sites in internal/mocks are seen. The ~44 cross-package ones stay invisible.
This is harmless today — MockConfigStore genuinely shadows, and its name-only form deliberately means "not called at all". But the guard's own comment says the opt-out "is detected, not hardcoded, so a future mock that adopts the same pattern is covered automatically", and for the repo's only cross-package mock that detection never runs. Two ways it bites later:
- If
assertions.go's shadowing is ever dropped or narrowed, ~44 name-only sites acrossinternal/api,internal/purchaseandinternal/scheduler— many of them money-path guards, per fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735 — silently lose coverage while the guard stays green. - Any future non-shadowing mock placed in
internal/mocksand aliased outward is unchecked from birth.
Cheapest proportionate fix: when a receiver resolves to a type name that is not in the package's mock map, route it to the skipped report instead of continue, so the blind spot is loud rather than silent. (Building the mock map repo-wide would also work and is more precise, but it is a larger change and the report-it version already removes the silence.)
F2 (minor, intentional) Partial shadowing exempts both helpers
collectMocks sets shadows = true if the type defines either AssertCalled or AssertNotCalled. A mock shadowing only one leaves the other going through testify — still vacuous in the name-only form — while the whole type is exempt. The self-test's MockShadowed defines only AssertNotCalled and asserts it "must count as shadowing", so this is deliberate; I raise it only because the opt-out is coarser than "this type handles its own assertions", and nothing records that.
F3 (minor) Methods with no direct m.Called are silently skipped
arity, known := m.calledArity[site.method]; if !known || m.shadows { continue }. A mock method that delegates to a sibling instead of calling m.Called itself has no derivable arity, so assertions naming it are neither checked nor reported. Narrow, but it is the same "silently checks nothing" shape that the checked == 0 fatal exists to prevent, one level down.
F4 (note) Unresolved sites carrying matchers are dropped
unresolvedLooksRisky returns false whenever site.matchers > 0, so an unresolved receiver with a wrong non-zero count is not reported. The code documents the reasoning ("ambiguous rather than risky"), and it is a defensible trade — but that variant is exactly the third vacuity mode found on #1735, so it is worth knowing it is out of scope for the unresolved path.
The providers/aws lint claim: verified as a set diff
Totals can hide an offsetting swap, so I compared the normalized finding sets, not the counts:
base: 272 findings head: 272 findings
only in BASE (fixed): (none)
only in HEAD (new): (none)
sets byte-identical: YES
savingsplans/client_test.go: 7 findings before, 7 after
Zero new, zero fixed, no swap. The claim holds.
Tooling note, since it cost me two false-clean runs and is the same class as the git grep -E / \s trap: golangci-lint v2 has no --out-format flag, and a concurrent invocation exits with Error: parallel golangci-lint is running. Both fail with exit 3 and zero findings, which reads exactly like a clean run. Any script capturing golangci-lint output must check for exit 3 and for a non-empty findings block before recording "0 issues".
Verdict
The substance is sound and the claims are accurate, including the one most likely to have been inflated: the 1-vs-29 split survives scrutiny, and my own first-pass regex was wrong in both directions before I read the code. The money-path assertions genuinely bite against a real production mutation and genuinely did not before. The guard fires end-to-end, fails hard when it checks nothing, and is immune to the #1751 CI gap by construction.
Nothing blocking. F1 is worth taking before this lands, not because it is exploitable today but because the guard currently reports success over ~44 unexamined sites on the repo's most important mock, and the comment above it claims otherwise. Making that case loud is a small change to one branch.
Execution-verified: the money-path mutation and its control, the guard run against pre-PR main (30) and head (0), the cross-package blind-spot experiment, the end-to-end red probe, the checked == 0 fatal, both test suites, gocyclo, and the lint set diff. Reading-derived: the 1-vs-29 adjudication (regex first, then read at each cited line), the injection-fidelity argument from mock.go:558, and F2/F3/F4.
… them Review finding F1. collectMocks is per-directory, and MockConfigStore is declared in internal/mocks and aliased into internal/api, internal/purchase and internal/scheduler -- the repo's only cross-package mock. In those packages it was not recognized as a mock at all, so every site fell into the "resolved to a non-mock" branch and was silently continued: absent from the checked count, absent from the report. Harmless while MockConfigStore's shadowed helpers apply, but the guard's own comment claimed the opt-out was detected rather than hardcoded, and for that one mock the detection never ran. Removing those helpers took the old guard from 6 findings to 6; it should have been 56. A check that cannot fire inside the check that exists to stop checks from not firing. The suggested cheap fix -- report the skip instead of swallowing it -- was measured first and rejected: it produces 183 findings, all MockConfigStore, which would leave the guard permanently red and therefore ignored. Instead the mocks are indexed repo-wide by type name and consulted when a receiver's type is not declared in the using package. Type names are not unique here (MockHTTPClient has five unrelated definitions, 24 names are multi-package), so a single candidate is used directly, multiple candidates only when they agree about the method in question, and anything else is reported rather than guessed. Guessing would attribute one mock's arity to another, which is the failure direction that gets a guard deleted. With both shadowed helpers renamed away the guard now reports 56 across checked 323 -- 6 in-package as before plus the 50 cross-package sites in internal/api and internal/purchase that were invisible. Also from the same review: F2: shadowing is now recorded per helper rather than per type. A mock that defines only AssertNotCalled left AssertCalled going through testify, where the name-only form is still unfailable, while the whole type was exempt. Renaming only MockConfigStore.AssertCalled now surfaces a name-only AssertCalled site that the old logic exempted. F3: a method that never calls m.Called itself has no derivable arity, and assertions naming it were dropped without a word. They are reported now, for the same reason checked == 0 is fatal. F4: an unresolved receiver is reported whatever its matcher count. A non-zero count that does not match the real arity is the third vacuity mode from #1735, so restricting the report to name-only sites left that mode out. F3 and F4 were both measured at zero sites before being changed, so neither adds noise today. Refs #1740
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/mocks/vacuous_assertion_guard_test.go (5)
110-121: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the per-directory mock map instead of calling
collectMockstwice.
collectMocksruns once per directory in the indexing loop and again in the checking loop. Each call re-walks every declaration body in the package. Store the result in the first pass and read it in the second.♻️ Proposed refactor
global := map[string][]*mockType{} parsed := map[string][]*ast.File{} fsets := map[string]*token.FileSet{} + dirMocks := map[string]map[string]*mockType{} for _, dir := range sortedKeys(pkgFiles) { ... parsed[dir] = files fsets[dir] = fset - for name, m := range collectMocks(files) { + dirMocks[dir] = collectMocks(files) + for name, m := range dirMocks[dir] { global[name] = append(global[name], m) } }Then in the checking loop:
- mocks := collectMocks(files) + mocks := dirMocks[dir]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/vacuous_assertion_guard_test.go` around lines 110 - 121, Store each directory’s result from the initial collectMocks call in a per-directory map, then retrieve that stored mock map in the later checking loop instead of invoking collectMocks again. Update the indexing and checking loops while preserving their existing behavior.
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this file to meet the 500-line limit.
The file has 650 lines. Move the analysis helpers (
mockType,assertSite,collectMocks,collectAssertSites,resolveRecvType,mockTypeOfExpr,recvTypeIdent,resolveCrossPackage,sameShadowing,unresolvedLooksRisky) into one file and keep theTest*functions in another.As per coding guidelines: "Follow Domain-Driven Design with bounded contexts, keep files under 500 lines, and use typed interfaces for public APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/vacuous_assertion_guard_test.go` around lines 1 - 13, Split the 650-line test file into two files under 500 lines: move mockType, assertSite, collectMocks, collectAssertSites, resolveRecvType, mockTypeOfExpr, recvTypeIdent, resolveCrossPackage, sameShadowing, and unresolvedLooksRisky into an analysis-helper file, while keeping all Test* functions in the test file. Preserve package scope and existing behavior.Source: Coding guidelines
372-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
repoRootstops at the firstgo.mod, which can narrow the scan without a signal.The function returns the nearest ancestor directory that contains a
go.mod. The repository uses more than one module. If a module boundary is ever added betweeninternal/mocksand the repository root, the walk covers only that submodule. The scan still finds sites, so thechecked == 0guard at Line 191 does not fire and the narrowing stays silent.Anchor the root on a repository-level marker as well, for example the directory that contains
.gitorgo.work, and fail when the two disagree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/vacuous_assertion_guard_test.go` around lines 372 - 388, Update repoRoot to identify the repository root using a repository-level marker such as .git or go.work in addition to go.mod, rather than returning the first module directory. Continue walking upward to find the repository-level root, and fail explicitly if the discovered markers disagree or no valid repository root exists.
530-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe synthetic harness does not exercise the cross-package and skip paths of the real loop.
The real loop at Lines 129-139 sends a
resolved && !isMocksite toresolveCrossPackageand reports it as skipped when resolution fails. The harness here returnscontinuefor the same condition. The real loop also reports methods with no derivable arity at Lines 158-168; the harness folds!knowninto a plaincontinueat Line 543. As a result the synthetic source cannot catch a regression in either report path.Extract the per-site decision into one function that both
TestNoUnfailableMockAssertionsand this test call. That keeps the synthetic coverage aligned with the code that runs against the repository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/vacuous_assertion_guard_test.go` around lines 530 - 547, Extract the per-site verdict logic from the real loop into a shared function, including cross-package resolution and skipped-site reporting for resolved non-mocks, plus reporting when called arity is unknown. Update both TestNoUnfailableMockAssertions and this synthetic harness to call that function instead of duplicating decisions, preserving existing mock and unresolved-risk handling.
234-252: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind
Calleddetection to the method receiver, and detect conflicting arities.Two gaps in the arity derivation:
- The selector base is not checked. Any call named
CalledorMethodCalledon any expression setscalledArity, including a call on a different object or a call inside a nested closure such as aRun(func(...){ ... })body. ComparerecvTypeIdent(fd.Recv.List[0].Type)against the receiver identifier name to accept onlym.Called(...)for that method.- If a method contains more than one
Calledcall with different argument counts, the last one silently wins. The guard then demands a matcher count that cannot match the other branch. Record the conflict and report the method as underivable, the same way Lines 158-168 handle a missing arity.Both cases make the guard prescribe a wrong matcher count, which re-creates the unfailable assertion it exists to prevent.
♻️ Proposed hardening
typeName := recvTypeIdent(fd.Recv.List[0].Type) if typeName == "" { continue } + var recvName string + if len(fd.Recv.List[0].Names) == 1 { + recvName = fd.Recv.List[0].Names[0].Name + } ... ast.Inspect(fd.Body, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) if !ok { return true } sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return true } + base, ok := sel.X.(*ast.Ident) + if !ok || recvName == "" || base.Name != recvName { + return true + } switch sel.Sel.Name { case "Called": - m.calledArity[fd.Name.Name] = len(call.Args) + record(m, fd.Name.Name, len(call.Args)) case "MethodCalled": if len(call.Args) >= 1 { - m.calledArity[fd.Name.Name] = len(call.Args) - 1 + record(m, fd.Name.Name, len(call.Args)-1) } } return true })Where
recorddeletes the entry and marks the method underivable when a second, different arity appears.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/vacuous_assertion_guard_test.go` around lines 234 - 252, Restrict the AST handling in the arity scan to calls whose selector base matches the receiver identifier for the current method, using recvTypeIdent(fd.Recv.List[0].Type), so nested or unrelated Called/MethodCalled calls are ignored. Update the arity recording logic to detect differing arities within one method, delete the recorded entry, and mark that method underivable as the existing missing-arity handling does, rather than letting the last call win.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/mocks/vacuous_assertion_guard_test.go`:
- Around line 309-339: Update resolveRecvType to inspect only the innermost
*ast.FuncDecl or *ast.FuncLit containing site.pos, rather than traversing the
entire file. Within that function scope, resolve assignments for site.recv; if
multiple bindings for the name produce different mock types, return unresolved
(empty type and false) so the site is reported as skipped instead of using a
guessed type.
- Around line 278-297: Update the assertion-site collection logic around the
receiver, argument-count, and method-literal checks so unsupported or malformed
shapes are recorded with a marker and included in skipped rather than silently
ignored; preserve collection for supported selectors and ensure nested
receivers, nonliteral method names, and insufficient arguments are reported.
Replace strings.Trim on lit.Value with strconv.Unquote, accepting only
successfully decoded quoted string literals and handling escapes and raw-string
literals correctly.
In `@providers/aws/services/savingsplans/client_test.go`:
- Around line 1534-1535: Move the mockSP.AssertNotCalled assertion to after the
client.findOfferingID call under test, preserving its existing matcher arguments
so it verifies DescribeSavingsPlansOfferings was not invoked by the
implementation.
---
Nitpick comments:
In `@internal/mocks/vacuous_assertion_guard_test.go`:
- Around line 110-121: Store each directory’s result from the initial
collectMocks call in a per-directory map, then retrieve that stored mock map in
the later checking loop instead of invoking collectMocks again. Update the
indexing and checking loops while preserving their existing behavior.
- Around line 1-13: Split the 650-line test file into two files under 500 lines:
move mockType, assertSite, collectMocks, collectAssertSites, resolveRecvType,
mockTypeOfExpr, recvTypeIdent, resolveCrossPackage, sameShadowing, and
unresolvedLooksRisky into an analysis-helper file, while keeping all Test*
functions in the test file. Preserve package scope and existing behavior.
- Around line 372-388: Update repoRoot to identify the repository root using a
repository-level marker such as .git or go.work in addition to go.mod, rather
than returning the first module directory. Continue walking upward to find the
repository-level root, and fail explicitly if the discovered markers disagree or
no valid repository root exists.
- Around line 530-547: Extract the per-site verdict logic from the real loop
into a shared function, including cross-package resolution and skipped-site
reporting for resolved non-mocks, plus reporting when called arity is unknown.
Update both TestNoUnfailableMockAssertions and this synthetic harness to call
that function instead of duplicating decisions, preserving existing mock and
unresolved-risk handling.
- Around line 234-252: Restrict the AST handling in the arity scan to calls
whose selector base matches the receiver identifier for the current method,
using recvTypeIdent(fd.Recv.List[0].Type), so nested or unrelated
Called/MethodCalled calls are ignored. Update the arity recording logic to
detect differing arities within one method, delete the recorded entry, and mark
that method underivable as the existing missing-arity handling does, rather than
letting the last call win.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff87aa34-3a3b-4b4f-a9ac-a02a58dfac53
📒 Files selected for processing (13)
cmd/multi_service_test.gointernal/api/executed_notification_flow_test.gointernal/api/handler_analytics_test.gointernal/api/handler_auth_test.gointernal/api/handler_per_account_perms_test.gointernal/api/handler_purchases_test.gointernal/api/handler_ri_exchange_test.gointernal/auth/service_test.gointernal/email/mute_test.gointernal/mocks/vacuous_assertion_guard_test.gointernal/purchase/execution_test.gointernal/scheduler/scheduler_test.goproviders/aws/services/savingsplans/client_test.go
Delta review,
|
…e guard Three CodeRabbit findings, all Major, all reproduced before fixing. An assertion positioned before the call under test is a FOURTH vacuity mode. savingsplans/client_test.go asserted DescribeSavingsPlansOfferings was not called, above the findOfferingID call it guards. The mock had recorded nothing at that point, so it passed for any implementation regardless of matcher count -- inside the PR that removes unfailable assertions. Proved by mutating the production short-circuit so the CE-provided offering ID no longer suppresses the lookup, with a permissive expectation registered so the assertion rather than testify's panic decides: the original ordering passes, the moved assertion fails at its own line. The verification method used for the other 29 sites was blind to this by construction -- it injected a recorded call immediately BEFORE the assertion, which is the ordering under test. Swept all 30 sites with an AST detector for the shape; this was the only genuine instance. The others it flagged are t.Cleanup bodies, which run after the test returns, and calls appearing as arguments to sibling assertions. Detecting the mode generally is not cheap: the prototype produced 10 findings with 9 false positives, because there is no syntactic notion of "the call under test". A check at that ratio gets deleted rather than fixed, so it is filed as #1761 and this PR's claim is narrowed to the matcher-count modes. resolveRecvType searched the whole file and kept the last matching assignment, so two tests in one file both naming a variable mockStore would resolve every site to whichever type appeared last, comparing matcher counts against the wrong arity. It now walks the enclosing function scopes outward the way Go's lexical scoping does -- which also keeps a t.Cleanup closure referencing an outer mock resolvable -- and returns unresolved when one scope binds a name to two different types, rather than guessing. Wrong attribution is the failure direction that gets a guard deleted. No file in the repo currently binds one name to two mock types, so nothing was mis-attributed; the fix closes a latent hazard. Three assertion shapes were dropped without a word, contradicting the guard's own stated rule that what it cannot place is reported: a receiver that is not a plain identifier, a method name that is not a string literal, and a malformed call. All three now reach the skipped report. Unquoting uses strconv.Unquote rather than strings.Trim, which mishandles escapes and silently accepts a raw-string literal. The multi-package mock figure in the PR body was wrong: it counted every func (m *T) receiver name rather than only types dispatching through m.Called. Restated with its counting method, since three methods give three answers and the safety argument rests instead on MockConfigStore having exactly one declaring package. Refs #1740 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Delta review at
|
| shape | claimed | what the repo-wide run actually does |
|---|---|---|
p.AssertNotCalled(t, "Two", anyArgs...) |
"Variadic spread is left alone" (PR body) | false finding: "passes no matchers, but Two hands 2 argument(s) to m.Called … can never fail" |
h.mockProbe.AssertNotCalled(t, "Two") |
reported into skipped |
silently dropped — not checked, not skipped, not reported |
p.AssertNotCalled(t, methodName) |
reported with its reason | reported, but with the wrong reason: p.AssertNotCalled(t, "") -- does not call m.Called directly |
m.AssertNotCalled(t) (<2 args) |
reported | unreported; also has no instance in the synthetic fixture, so unsupported != 3 does not guard it either |
Raw output:
probepkg/probe_test.go:22:2: p.AssertNotCalled(t, "") -- does not call m.Called directly...
probepkg/probe_test.go:12:2: p.AssertNotCalled(t, "Two") passes no matchers, but Two hands 2 argument(s)...
(the selector-receiver site at probe_test.go:17 appears in neither list)
The spread case is the one that matters. The early return sets site.method but not site.matchers, so matchers stays 0 and the site lands in the strongest finding class. That is the opposite of the code's own comment at :341-343 — it does not count the spread as one matcher, it counts it as zero, which is worse — and it contradicts the PR body bullet "Variadic spread is left alone". It does not fire today only because no such site exists: git grep -nE "Assert(Not)?Called\(.*\.\.\.\)" matches only the guard's own comment and its synthetic fixture. The first person to write one gets a confident, wrong finding.
The selector-receiver blind spot is live in this repo right now. internal/mocks/assertions_test.go:81:
assert.True(t, m.Mock.AssertNotCalled(&recordingT{}, "WithTx"))That is the one real selector receiver in the tree (verified with git grep -nE "[A-Za-z0-9_]+\.[A-Za-z0-9_]+\.Assert(Not)?Called\(", sanity-checked against a line known to match). It is being silently dropped — the exact defect the delta says it closed.
Suggested fix, at the top of the site loop in TestNoUnfailableMockAssertions:
if site.unsupported != "" {
skipped = append(skipped, fmt.Sprintf(
"%s: %s.%s -- %s. Review by hand.",
site.pos, site.recv, site.assertFn, site.unsupported))
continue
}plus a <2 args case in the fixture and unsupported != 4. Note this is not free: wiring it in newly reports assertions_test.go:81, so that site needs a decision in the same change (the report is arguably correct — that call is deliberately unfailable, it is demonstrating testify's behaviour — but the guard will be red until it is handled).
F2 (minor, latent) — the outward walk over-reaches on a shadowed name bound from a non-literal
resolveRecvType / bindingInScope (:385, :402).
bindingInScope conflates "this scope does not bind the name" with "this scope binds it, but not to something I recognise as a mock literal" — mockTypeOfExpr returns "" and the loop continues. The walk then proceeds outward and resolves against the enclosing binding, which Go's scoping says is shadowed.
Demonstrated against the committed collectAssertSites/resolveRecvType:
func TestOuter(t *testing.T) {
m := &MockAlpha{}
t.Run("sub", func(t *testing.T) {
m := newBeta() // shadows; guard cannot classify the RHS
m.AssertNotCalled(t, "Do") // resolved to "MockAlpha", resolved=true
})
}SHADOW site m@7 -> "MockAlpha" resolved=true
This is wrong attribution — the failure direction the commit message correctly identifies as the one that gets a guard deleted — reintroduced in a narrower form. It needs the arity to differ to produce a wrong verdict, so it is latent, not live.
It is also precisely the case TestResolveRecvTypeScoping does not pin: the test has four cases and none of them is a shadowing case. Inner-wins is asserted nowhere.
Fix: have bindingInScope return "name is assigned anywhere in this scope" separately from "bound to a recognisable mock type", and stop the walk (unresolved) when the former is true and the latter is not.
F3 (minor, accuracy) — "the way Go's own lexical scoping works" overstates it
The scope chain is function bodies, not blocks, so bindingInScope searches across sibling if/for/bare blocks that Go treats as separate scopes:
func TestOuter(t *testing.T) {
{ m := &MockAlpha{}; _ = m }
m := &MockBeta{}
m.AssertNotCalled(t, "Do") // Go: MockBeta. Guard: conflict -> unresolved
}BLOCK site m@9 -> "" resolved=false
The direction is safe (unresolved → skipped → t.Errorf, i.e. noise not mis-attribution), but the doc comment at :373-384 and the PR body bullet both claim fidelity to Go's scoping that the implementation does not have. Worth softening to "enclosing function scopes" rather than claiming lexical-scope equivalence.
F4 (minor, latent) — package-level mock vars silently stopped resolving
The chain contains only function bodies, so a package-level var pkgMock = &MockAlpha{} — which the old file-wide search found — no longer resolves:
PKGLEVEL site pkgMock@6 -> "" resolved=false
It degrades into unresolvedLooksRisky, so a future site of this shape reddens the guard with "receiver type could not be resolved" instead of the real finding. Zero instances today. Flagging as a known limitation, not asking for a fix.
Verified clean
Item 3, the ordering fix — re-derived exactly as claimed. Mutated the production short-circuit in providers/aws/services/savingsplans/client.go:425 to if false && spDetails.OfferingID != "", registered a permissive .Maybe() expectation so the assertion rather than a testify panic decides, and ran both orderings:
| ordering | result under identical mutation |
|---|---|
| assertion after the call (this PR) | FAIL at client_test.go:1548 — "Expected DescribeSavingsPlansOfferings to not have been called with: [mock.Anything mock.Anything] but actually it was" |
| assertion before the call (original) | PASS |
The completeness claim holds, and I checked it with a positive control — the point the original verification method was blind to by construction. Wrote an independent AST sweep over all 12 touched files: for every AssertCalled/AssertNotCalled statement, collect the statements that follow it in its own block and in every enclosing block up to the containing function literal (stopping at a FuncLit boundary, since a t.Cleanup body runs after the test), then filter out testify assertions and teardown.
- At
67cced118: 4 flagged of 125 scanned, includingsavingsplans/client_test.go:1535withid, err := client.findOfferingID(...)sitting after it. The detector demonstrably finds the shape. - At
b90c9ebf5: 3 flagged of 125. The savingsplans site is gone; the other three (handler_purchases_test.go:1147,:2557,:2559) I read individually — in each, the call under test (handler.getPlannedPurchasesat:1141,handler.cancelPurchaseat:2552) precedes the assertion and only result-inspection follows. All benign.
So: the only genuine instance among the touched files, confirmed by a method proven in both directions rather than only the negative one.
MockConfigStore is declared in exactly one package. One struct declaration (internal/mocks/stores.go:40); the three other hits are type MockConfigStore = mocks.MockConfigStore aliases in internal/api, internal/purchase, internal/scheduler. Aliases declare no methods, so collectMocks contributes nothing for them and global["MockConfigStore"] has exactly one candidate. The cross-package safety argument stands.
TestResolveRecvTypeScoping genuinely discriminates. Re-ran its fixture with resolveRecvType([]ast.Node{f}, site) (file-wide): mockStore@5 and mockStore@10 both become <unresolved>, so the test fails on two of its four expectations. Not a vacuous test. Its gap is coverage (F2), not discrimination.
No coverage regression from the scope change. checked = 125 at both 67cced118 and b90c9ebf5, both green. The narrowing did not silently shrink what the guard analyses on today's repo.
The two judgement calls are sound. Declining the ordering check at 9/10 false positives is the right call, and LeanerCloud/cloud-commitments-platform#176 is the right place for it. The "What this guard does NOT cover" section states the limitation accurately and narrows the claim to the matcher-count modes — it does not claim the vacuity class is closed. One correction needed there: the bullet "Variadic spread is left alone" is now false per F1. Dropping the multi-package count in favour of a stated-method figure resting on the MockConfigStore fact is fine, and that fact is verified above.
Gates at b90c9ebf5
| gate | result |
|---|---|
gofmt -l . |
clean (0 files) |
go build ./... (root) |
exit 0 |
go vet ./... (root) |
exit 0 |
go test -race -count=1 ./... (root) |
exit 0 — 28 ok, 11 no-test-files, 0 FAIL |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, empty |
golangci-lint run --timeout=10m @ v2.10.1 |
exit 0 and a genuine 0 issues. line (installed to a private GOBIN; local 2.11.4 not used) |
providers/aws: build + vet + go test -race -count=1 ./... |
exit 0 — 12 packages ok, 0 FAIL |
| guard alone | checked 125, all three guard tests PASS |
Every gate is green. F1 is a correctness defect in the guard's own reporting, not a build failure — it is invisible to CI by construction, which is why it needs fixing before merge rather than after.
site.unsupported was written in five places and read in exactly one -- inside
TestVacuousAssertionGuardDetects. The repo-wide run went straight from
collectAssertSites to resolveRecvType and never consulted it, so no reason
string ever reached the skipped report on the path that runs.
The self-test passed because it re-implemented the main loop's dispatch, and
its copy had the unsupported branch the real one lacked. A test asserting on
its own private copy of the logic is the defect this guard exists to catch,
one level up.
Verified on an isolated module -- scratch go.mod, one package, so repoRoot()
resolves to it -- with the guard otherwise unmodified. Before:
variadic spread FALSE FINDING ("passes no matchers, but Two hands 2")
selector receiver silently dropped, neither checked nor reported
non-literal method reported with the wrong reason
A false finding on correct code is the worst of the three: that is what gets a
guard deleted rather than fixed.
Both halves are fixed, because either alone leaves the blind spot. The
per-site dispatch is now one function, classifySite, called by the repo-wide
run and by the self-test, so the self-test can no longer pass against a branch
the production path does not have. All three shapes now report their own
reason, and the spread produces no finding.
The fourth shape, fewer than two arguments, cannot occur in code that
compiles: testify's signature requires the method name, and the reviewer's
probe failed to build until it was removed. It stays as a defensive branch,
exercised only from the parse-only synthetic fixture, and is now marked as
such.
Routing the reports through the real path surfaced one live site the dead code
had been swallowing: assertions_test.go called testify's promoted
implementation as m.Mock.AssertNotCalled(...), a selector receiver the guard
cannot resolve. Hoisted to a local so the deliberate call into the broken
implementation stays, without tripping the report forever.
Also from the same review:
The outward scope walk over-reached. A scope binding a name from a non-literal
(mockStore := newStore()) yielded no type, so the walk continued outward and
could adopt an unrelated outer type that the code would never use. The
innermost scope that binds the name now wins whether or not its type can be
determined, matching what shadowing does at run time; an undeterminable
binding returns unresolved.
"the way Go's own lexical scoping works" overstated it. The walk approximates
Go's scoping for the shapes mock assertions are written in; it does not
implement it. Wording corrected.
Refs #1740
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merging. Closes #1740. Merging after four independent review rounds, without a fifth — and the reason is worth stating. Every prior round on this PR found something, so the base rate argues for another. Two things outweigh it: the F1 fix is structural rather than another patch (one dispatch implementation with two callers, so the divergence class is eliminated rather than corrected), and this is CR's latest verdict is stale at What this PR does: repairs 30 structurally-unfailable testify assertions — matcher counts that can never match, so the assertion cannot fire — and adds an AST guard that walks every package in all six workspace modules to prevent the class returning. The accounting is honest and worth preserving: 30 repaired, exactly 1 genuinely silent. The other 29 were panic-backstopped — a forbidden call blew up rather than passing quietly. The author declined to write it up as 30 caught bugs. An independent reviewer's first pass contradicted that split, then found both directions of its own regex were wrong: The money path was verified against a real production mutation — making the dry-run branch actually call Four defects were found in the fix itself, across the review rounds: The guard could not see cross-package mocks. A vacuous assertion inside the PR that removes vacuous assertions.
And the fourth is the one this PR exists to prevent, one level up. Both halves were fixed, because either alone leaves the hole: a single The proof that fix works is that the guard went red on its first run after it. Routing reports through the real path immediately surfaced a live site the dead code had been swallowing — One shape turned out not to exist. The A property worth recording: this guard is immune to #1751. It walks the filesystem rather than relying on Known limitations are in the body rather than discovered later: package-level mock vars are not resolved (function bodies only, zero instances today); assertion ordering is not covered, declined with the measurement that a prototype gave 10 findings and 9 false positives, filed as LeanerCloud/cloud-commitments-platform#176; and the Scope stated honestly: the guard closes the matcher-count modes, not assertion ordering. The body carries a "What this guard does NOT cover" section. Gates: gofmt, build, vet, |
…1750) * fix(test): repair 30 unfailable mock assertions and guard the class testify diffs the matchers an assertion is given against the arguments a mock recorded, and diffs an empty matcher list too, counting each real argument as a difference. A name-only AssertNotCalled(t, "Method") can therefore never match a method that passes arguments to m.Called: it reports success whatever the code under test did. PR #1735 fixed this for MockConfigStore by shadowing the helpers; every other mock still had it. Repaired 30 sites across 11 mock types by passing each method's real m.Called arity as matchers. The three highest-stakes are the dry-run guards in cmd/multi_service_test.go, which assert that --dry-run does not reach PurchaseCommitment; they could not fail. Every site was mutation-verified in both directions: with the forbidden call recorded on the mock, the name-only form passes (23+4 of 27) and the repaired form fails (27 of 27). The three dry-run guards were verified against a real production mutation making processPurchaseLoop purchase on the dry-run branch: the assertion fires only once it carries three matchers. Of the 30, exactly one had a registered expectation and so silently passed; the other 29 were backstopped by testify's unexpected-call panic, which fails the test loudly but at the mock rather than at the invariant. That backstop is accidental: it disappears the moment anyone registers an expectation for the method. TestNoUnfailableMockAssertions closes the class for code not yet written. It parses every package, derives each mock method's m.Called arity, and rejects any AssertCalled / AssertNotCalled whose matcher count cannot match. Mocks opt out by defining their own helpers, which is detected rather than hardcoded, so MockConfigStore's callLog shadows keep their name-only meaning and a future mock adopting the pattern is covered automatically. Sites whose receiver cannot be resolved are reported, not skipped, so a blind spot cannot hide behind a pass. TestVacuousAssertionGuardDetects exercises that analysis on synthetic source: name-only, wrong count, correct count, zero-arity method, a shadowed type, a non-mock, an unresolvable receiver and a variadic spread whose length is not knowable. Without it the guard could break into reporting nothing and the repo-wide run would still look green, which is the failure mode this whole class is about. The four providers/aws sites were found only by the guard: the repo is a Go workspace, and a type-checked sweep of the root module cannot see the other five modules. Closes #1740 * fix(test): make the guard see cross-package mocks instead of skipping them Review finding F1. collectMocks is per-directory, and MockConfigStore is declared in internal/mocks and aliased into internal/api, internal/purchase and internal/scheduler -- the repo's only cross-package mock. In those packages it was not recognized as a mock at all, so every site fell into the "resolved to a non-mock" branch and was silently continued: absent from the checked count, absent from the report. Harmless while MockConfigStore's shadowed helpers apply, but the guard's own comment claimed the opt-out was detected rather than hardcoded, and for that one mock the detection never ran. Removing those helpers took the old guard from 6 findings to 6; it should have been 56. A check that cannot fire inside the check that exists to stop checks from not firing. The suggested cheap fix -- report the skip instead of swallowing it -- was measured first and rejected: it produces 183 findings, all MockConfigStore, which would leave the guard permanently red and therefore ignored. Instead the mocks are indexed repo-wide by type name and consulted when a receiver's type is not declared in the using package. Type names are not unique here (MockHTTPClient has five unrelated definitions, 24 names are multi-package), so a single candidate is used directly, multiple candidates only when they agree about the method in question, and anything else is reported rather than guessed. Guessing would attribute one mock's arity to another, which is the failure direction that gets a guard deleted. With both shadowed helpers renamed away the guard now reports 56 across checked 323 -- 6 in-package as before plus the 50 cross-package sites in internal/api and internal/purchase that were invisible. Also from the same review: F2: shadowing is now recorded per helper rather than per type. A mock that defines only AssertNotCalled left AssertCalled going through testify, where the name-only form is still unfailable, while the whole type was exempt. Renaming only MockConfigStore.AssertCalled now surfaces a name-only AssertCalled site that the old logic exempted. F3: a method that never calls m.Called itself has no derivable arity, and assertions naming it were dropped without a word. They are reported now, for the same reason checked == 0 is fatal. F4: an unresolved receiver is reported whatever its matcher count. A non-zero count that does not match the real arity is the third vacuity mode from #1735, so restricting the report to name-only sites left that mode out. F3 and F4 were both measured at zero sites before being changed, so neither adds noise today. Refs #1740 * fix(test): repair an assertion that ran before its call, and scope the guard Three CodeRabbit findings, all Major, all reproduced before fixing. An assertion positioned before the call under test is a FOURTH vacuity mode. savingsplans/client_test.go asserted DescribeSavingsPlansOfferings was not called, above the findOfferingID call it guards. The mock had recorded nothing at that point, so it passed for any implementation regardless of matcher count -- inside the PR that removes unfailable assertions. Proved by mutating the production short-circuit so the CE-provided offering ID no longer suppresses the lookup, with a permissive expectation registered so the assertion rather than testify's panic decides: the original ordering passes, the moved assertion fails at its own line. The verification method used for the other 29 sites was blind to this by construction -- it injected a recorded call immediately BEFORE the assertion, which is the ordering under test. Swept all 30 sites with an AST detector for the shape; this was the only genuine instance. The others it flagged are t.Cleanup bodies, which run after the test returns, and calls appearing as arguments to sibling assertions. Detecting the mode generally is not cheap: the prototype produced 10 findings with 9 false positives, because there is no syntactic notion of "the call under test". A check at that ratio gets deleted rather than fixed, so it is filed as #1761 and this PR's claim is narrowed to the matcher-count modes. resolveRecvType searched the whole file and kept the last matching assignment, so two tests in one file both naming a variable mockStore would resolve every site to whichever type appeared last, comparing matcher counts against the wrong arity. It now walks the enclosing function scopes outward the way Go's lexical scoping does -- which also keeps a t.Cleanup closure referencing an outer mock resolvable -- and returns unresolved when one scope binds a name to two different types, rather than guessing. Wrong attribution is the failure direction that gets a guard deleted. No file in the repo currently binds one name to two mock types, so nothing was mis-attributed; the fix closes a latent hazard. Three assertion shapes were dropped without a word, contradicting the guard's own stated rule that what it cannot place is reported: a receiver that is not a plain identifier, a method name that is not a string literal, and a malformed call. All three now reach the skipped report. Unquoting uses strconv.Unquote rather than strings.Trim, which mishandles escapes and silently accepts a raw-string literal. The multi-package mock figure in the PR body was wrong: it counted every func (m *T) receiver name rather than only types dispatching through m.Called. Restated with its counting method, since three methods give three answers and the safety argument rests instead on MockConfigStore having exactly one declaring package. Refs #1740 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test): route unsupported shapes through the path that actually runs site.unsupported was written in five places and read in exactly one -- inside TestVacuousAssertionGuardDetects. The repo-wide run went straight from collectAssertSites to resolveRecvType and never consulted it, so no reason string ever reached the skipped report on the path that runs. The self-test passed because it re-implemented the main loop's dispatch, and its copy had the unsupported branch the real one lacked. A test asserting on its own private copy of the logic is the defect this guard exists to catch, one level up. Verified on an isolated module -- scratch go.mod, one package, so repoRoot() resolves to it -- with the guard otherwise unmodified. Before: variadic spread FALSE FINDING ("passes no matchers, but Two hands 2") selector receiver silently dropped, neither checked nor reported non-literal method reported with the wrong reason A false finding on correct code is the worst of the three: that is what gets a guard deleted rather than fixed. Both halves are fixed, because either alone leaves the blind spot. The per-site dispatch is now one function, classifySite, called by the repo-wide run and by the self-test, so the self-test can no longer pass against a branch the production path does not have. All three shapes now report their own reason, and the spread produces no finding. The fourth shape, fewer than two arguments, cannot occur in code that compiles: testify's signature requires the method name, and the reviewer's probe failed to build until it was removed. It stays as a defensive branch, exercised only from the parse-only synthetic fixture, and is now marked as such. Routing the reports through the real path surfaced one live site the dead code had been swallowing: assertions_test.go called testify's promoted implementation as m.Mock.AssertNotCalled(...), a selector receiver the guard cannot resolve. Hoisted to a local so the deliberate call into the broken implementation stays, without tripping the report forever. Also from the same review: The outward scope walk over-reached. A scope binding a name from a non-literal (mockStore := newStore()) yielded no type, so the walk continued outward and could adopt an unrelated outer type that the code would never use. The innermost scope that binds the name now wins whether or not its type can be determined, matching what shadowing does at run time; an undeterminable binding returns unresolved. "the way Go's own lexical scoping works" overstated it. The walk approximates Go's scoping for the shapes mock assertions are written in; it does not implement it. Wording corrected. Refs #1740 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #1740
The defect
testify diffs the matchers an assertion is given against the arguments a mock recorded — and it diffs an empty matcher list too, counting each real argument as a difference. So a name-only
AssertNotCalled(t, "Method")never matches a method that passes arguments tom.Called, and reports success whatever the code under test did.PR #1735 fixed this for
MockConfigStoreby shadowing the helpers withcallLog-backed versions. Those shadows are on*MockConfigStoreonly; every other mock in the repo still had the underlying behaviour.The real count: 30, measured rather than estimated
The issue quoted 82 name-only sites / roughly 30 vacuous, explicitly flagged as a static count of call sites rather than of dangerous ones, and asked for the real number. Measured three ways:
AssertCalled/AssertNotCalledsites, repo-wide, all six modulesThe 30 matches the issue's estimate, but not for the reason the estimate assumed — see the honesty note below. The other 54 name-only sites are legitimate: the guard added here passes on this branch, which is precisely the statement that none of them can be unfailable (they are on
MockConfigStore's shadowed helpers or on zero-argument methods).Distribution of the 30:
internal/api16,providers/aws4,internal/purchase3,cmd3,internal/email2,internal/auth1,internal/scheduler1.Reconciliation: 84 name-only on
main− 30 repaired = 54, plus 6 name-only forms inside the new guard's synthetic test fixture = 60 matches on this branch.The four
providers/awssites were invisible to a type-checked sweep of the root module. This repo is a Go workspace (go.work:.,pkg,providers/aws,providers/azure,providers/gcp,tests/e2e), andpackages.Load("./...")from the root sees only the root module. The guard added here walks the filesystem instead, so it covers every module.Money path first
cmd/multi_service_test.go:1349,:1411,:1486assert that--dry-runnever reachesPurchaseCommitment— the check standing between a dry run and real money.MockServiceClient.PurchaseCommitmenthands 3 arguments tom.Called; all three assertions passed none, so none could fail.Verified against a real production mutation (
processPurchaseLoopissuing a purchase on the dry-run branch), with a permissive expectation registered so the assertion rather than testify's panic decides the outcome:Honesty note: what the panic backstop means
Of the 30 sites, only 1 (
handler_ri_exchange_test.go:971) had a registered expectation for the asserted method and therefore passed silently. For the other 29, the mock reachesm.Calledunconditionally with no expectation registered, so the forbidden call hits testify's unexpected-call path and panics — the test fails loudly, but atmocks_test.goand with a message about an unexpected call rather than about the invariant.Determined by a runtime probe: testify was replaced with an instrumented copy that logged, at every
AssertNotCalled, the matcher count, the recorded arity and whether an expectation existed. Cross-checked statically — none of the 30 mock methods has anisExpectedshort-circuit orFnescape that would let the call return quietly.So the honest framing is: 1 caught bug that would otherwise have shipped, and 29 assertions that claimed to check something they could not check. The backstop is accidental, not designed — it disappears the moment anyone registers an expectation for that method, which is exactly the shape #1735 found 77 instances of. Overstating this would be the same defect class the PR is about.
Per-site mutation verification, both directions
Not verified in aggregate. For each of the 27 non-money-path sites, the forbidden call was injected directly into the mock's log immediately before the assertion — the exact state the mock would be in had the guarded bug fired, with nothing else changed, so the assertion is isolated from the panic:
Plus the 3 money-path sites against a real production mutation, above. 30/30.
Preventing the mode rather than fixing instances
TestNoUnfailableMockAssertions(internal/mocks/vacuous_assertion_guard_test.go) parses every package in every module, derives each mock method'sm.Calledarity, and rejects anyAssertCalled/AssertNotCalledwhose matcher count cannot match — both name-only and any wrong count (the third variant #1735 hardenedmatching()against).Deliberate design points:
m.Called, not the signature.MockSESClient.SendEmailtakes three parameters but callsm.Called(ctx, input). An earlier version of this analysis used signature arity and produced two false findings on exactly that method; testify diffs against whatm.Calledreceives.AssertCalled/AssertNotCalledis exempt, soMockConfigStore's 53 name-only sites keep theircallLogmeaning and any future mock adopting the pattern is covered without editing an allowlist.MockConfigStore(declared ininternal/mocks, aliased intointernal/api,internal/purchaseandinternal/scheduler) is actually analysed there rather than silently passed over. Names are not unique across the repo —MockHTTPClientis declared in five separate packages andMockEmailSenderin four — so multiple candidates resolve only when they agree about the method and about which helpers they shadow; anything else is reported rather than guessed. The two facts the safety argument rests on are that same-name collisions are common enough to matter, and thatMockConfigStoreis declared in exactly one package (internal/mocks), so its fallback is unambiguous. (An earlier revision said "24 of 142 mock type names are multi-package". That counted everyfunc (m *T)receiver name rather than only types that dispatch throughm.Called. Counting the way the guard defines a mock — at least one method callingm.Called, grouped by package directory — gives 12 of 39. A third count using a different grouping gives 13 of 49. The precise figure is method-dependent, so it is stated here with its method rather than quoted bare.)mockStoreto whichever type appeared last, comparing matcher counts against the wrong arity. Conflicting bindings inside one scope return unresolved rather than a guess.go test ./..., so it analysesproviders/*,pkgandtests/e2eeven though CI does not currently run those modules' tests. That is exactly how the fourproviders/awssites surfaced: a type-checked sweep of the root module could not see them, and neither could CI. It keeps working regardless of how ci: unit and integration test jobs only run the root module — providers/*, pkg and tests/e2e are never tested #1751 is resolved.AssertNotCalled(t, "M", args...)has no statically knowable count; counting the spread expression as one matcher would invent a mismatch.checked == 0is a hard failure, so the guard cannot silently stop finding anything.It parses rather than builds: ~0.4s, no new dependencies.
What this guard does NOT cover
AssertNotCalledhas a fourth vacuity mode the guard does not detect: an assertion positioned before the call under test. The mock has recorded nothing when it runs, so it passes for any implementation whatever its matcher count. One such site existed in this PR (savingsplans/client_test.go:1535) and is fixed here.The guard checks matcher count against arity; it has no notion of "the call under test", and building one is not cheap. A prototype detector (innermost-scope aware, skipping
t.Cleanupbodies whose contents run after the test) produced 10 findings across the touched files, 9 of them false positives — assertions inside subtests, and calls appearing only as arguments to other assertions. A check with that ratio gets deleted rather than fixed, which is the failure direction this PR is built to avoid.So the claim in this PR is narrowed accordingly: it closes the matcher-count modes (name-only, and any count that cannot match), not assertion ordering. The ordering mode is filed as #1761 with the prototype's results and two candidate approaches that might make it tractable.
Known limitations, recorded deliberately
var pkgMock = &MockAlpha{}at package scope degrades into the unresolved path and would be reported as "receiver type could not be resolved" rather than analysed. Zero instances today. The older file-wide search happened to resolve these; the scoped walk trades that for correctness on the far more common same-name-in-two-tests case. Noted so it does not surprise anyone later.Review findings addressed (head
14dfddfff)F1 (blocking, fourth review).
site.unsupportedwas written in five places and read in exactly one — inside the self-test. The repo-wide run never consulted it, so no reason string reachedskippedon the path that actually runs. The self-test passed because it re-implemented the main loop's dispatch, and its copy had the branch the real one lacked. A test asserting on its own private copy of the logic is the defect this guard exists to catch, one level up.Verified on an isolated module (scratch
go.mod, one package, guard otherwise unmodified). Before:p.AssertNotCalled(t, "Two", anyArgs...)h.mockProbe.AssertNotCalled(t, "Two")p.AssertNotCalled(t, methodName)After, on the same probe — three reports, each with its own reason, and zero findings:
Both halves are fixed, since either alone leaves the blind spot: the per-site dispatch is now a single
classifySitecalled by both the repo-wide run and the self-test, so the self-test can no longer pass against a branch production does not have.Routing reports through the real path immediately surfaced one live site the dead code had been swallowing:
assertions_test.gocalled testify's promoted implementation asm.Mock.AssertNotCalled(...), a selector receiver. Hoisted to a local so the deliberate call into the broken implementation stays without tripping the report forever.F2. The outward scope walk over-reached: a scope binding a name from a non-literal (
mockStore := newStore()) yielded no type, so the walk continued outward and could adopt an unrelated outer type. The innermost binding scope now wins whether or not its type is determinable, matching what shadowing does at run time.F3. "the way Go's own lexical scoping works" overstated it — the walk approximates Go's scoping for the shapes mock assertions take. Wording corrected.
Earlier review findings (head
67cced118)collectMockswas per-directory, so the one cross-package mock was unrecognized in the three packages that alias it: those sites hit the "resolved to a non-mock" branch and were silently skipped — not counted, not reported. RenamingMockConfigStore's shadowed helpers took the old guard from 6 findings to 6, when it should have been 56. The suggested cheap fix (report the skip) was measured first and rejected: it yields 183 findings, allMockConfigStore, which would leave the guard permanently red and therefore ignored. The repo-wide index above fixes it properly; with both helpers renamed the guard now reports 56 acrosschecked 323— the 6 in-package plus the 50 cross-package sites that were invisible.AssertNotCalledleftAssertCalledgoing through testify while the whole type was exempt. Renaming onlyMockConfigStore.AssertCallednow surfaces a name-onlyAssertCalledsite the old logic exempted.m.Calleditself has no derivable arity; those assertions were dropped without a word and are now reported, for the same reasonchecked == 0is fatal.F3 and F4 were each measured at zero sites before being changed, so neither adds noise today.
TestResolveCrossPackagepins the ambiguity rules (single candidate, none, agreeing duplicates, duplicates disagreeing on arity or on shadowing).TestVacuousAssertionGuardDetectsexercises the analysis on synthetic source covering name-only, wrong count, correct count, a zero-arity method, a shadowed type, a non-mock, an unresolvable receiver and a variadic spread. It was written after an earlier revision of the guard reported a resolved-but-non-mock receiver as an unreviewed blind spot, and each case was confirmed to fail with the corresponding logic disabled. Verified end-to-end by injecting a fresh name-only assertion intointernal/auth/service_test.go, which the guard flagged.Gates
Root module:
go build ./...,go vet ./...,go test -race -count=1 ./...(6734 passed, 43 packages),gocyclo -over 10 -ignore "_test\.go" .(exit 0, empty),golangci-lint runat the CI-pinned v2.10.1 (exit 0, "0 issues").providers/awsmodule: build, vet,go test -race(1212 passed, 12 packages).golangci-lintonproviders/awsis red onmain(272 issues by golangci's own summary, mostlygodot140 /misspell67 /gocritic43) and is identically red with and without this change — 272 both ways with an identical per-linter breakdown, zero new. CI lints the root module only, so this is pre-existing debt this PR neither adds to nor masks.Correction: an earlier revision of this section said 276. That came from grepping for lines ending in a parenthesised word, which also matched four prose lines; golangci's own summary reports 272. The comparison was run the same way on both sides, so "zero new" was unaffected.
Out of scope, flagged not fixed
go test ./...from the root only, which undergo.workcovers the root module alone — soproviders/*,pkgandtests/e2etests are not run by that job, thoughgovulncheckdoes loop over every module. The guard added here lives in the root module and scans all of them, so it is enforced regardless. Worth a separate issue.Summary by CodeRabbit