From 457ea300d23d65f59549453b5a8a593fe8801c1f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 04:44:13 +0200 Subject: [PATCH 1/4] 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 --- cmd/multi_service_test.go | 6 +- .../api/executed_notification_flow_test.go | 3 +- internal/api/handler_analytics_test.go | 10 +- internal/api/handler_auth_test.go | 2 +- .../api/handler_per_account_perms_test.go | 4 +- internal/api/handler_purchases_test.go | 12 +- internal/api/handler_ri_exchange_test.go | 2 +- internal/auth/service_test.go | 2 +- internal/email/mute_test.go | 4 +- .../mocks/vacuous_assertion_guard_test.go | 488 ++++++++++++++++++ internal/purchase/execution_test.go | 6 +- internal/scheduler/scheduler_test.go | 2 +- .../aws/services/savingsplans/client_test.go | 8 +- 13 files changed, 519 insertions(+), 30 deletions(-) create mode 100644 internal/mocks/vacuous_assertion_guard_test.go diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 2c09d8ddd..d696d319f 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1346,7 +1346,7 @@ func TestProcessPurchaseLoopEmptyRecommendations(t *testing.T) { results := processPurchaseLoop(ctx, []common.Recommendation{}, "us-east-1", false, mockClient, toolCfg) assert.Empty(t, results) - mockClient.AssertNotCalled(t, "PurchaseCommitment") + mockClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) } func TestProcessServicePurchasesUserCancellation(t *testing.T) { @@ -1408,7 +1408,7 @@ func TestProcessServicePurchasesDryRunMultiple(t *testing.T) { assert.Equal(t, recs[i].ResourceType, result.Recommendation.ResourceType) } - mockClient.AssertNotCalled(t, "PurchaseCommitment") + mockClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) } // ==================== New Extracted Function Tests ==================== @@ -1483,7 +1483,7 @@ func TestProcessPurchaseLoopDryRun(t *testing.T) { } // Mock should not be called in dry run mode - mockClient.AssertNotCalled(t, "PurchaseCommitment") + mockClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) } func TestProcessPurchaseLoopActualPurchase(t *testing.T) { diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 2d59b2e44..09af39939 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/email" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -244,7 +245,7 @@ func TestExecutedNotification_SessionApprovePath(t *testing.T) { // Admin approved, so the executor recorded in the body is the admin. assertExecutedNotificationFingerprints(t, notifier, contact, adminEmail, "valid-token") mockPurchase.AssertExpectations(t) - mockPurchase.AssertNotCalled(t, "ApproveExecution") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestExecutedNotification_DirectExecutePath is the regression test for the diff --git a/internal/api/handler_analytics_test.go b/internal/api/handler_analytics_test.go index 17106906c..965f760cc 100644 --- a/internal/api/handler_analytics_test.go +++ b/internal/api/handler_analytics_test.go @@ -201,7 +201,7 @@ func TestHandler_getHistoryAnalytics_InvalidProvider(t *testing.T) { _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"provider": "oracle"}) require.Error(t, err) assert.Contains(t, err.Error(), "invalid provider") - mockClient.AssertNotCalled(t, "QueryHistory") + mockClient.AssertNotCalled(t, "QueryHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestHandler_getHistoryAnalytics_InvalidDateRange(t *testing.T) { @@ -256,7 +256,7 @@ func TestHandler_getHistoryAnalytics_ScopedUser_RequiresAccountID(t *testing.T) _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{}) require.Error(t, err) assert.Contains(t, err.Error(), "account_id is required") - mockClient.AssertNotCalled(t, "QueryHistory") + mockClient.AssertNotCalled(t, "QueryHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestHandler_getHistoryBreakdown_Success(t *testing.T) { @@ -411,7 +411,7 @@ func TestHandler_triggerAnalyticsCollection_NonAdmin(t *testing.T) { _, err := handler.triggerAnalyticsCollection(ctx, req, nil) require.Error(t, err) assert.Contains(t, err.Error(), "admin access required") - mockCollector.AssertNotCalled(t, "Collect") + mockCollector.AssertNotCalled(t, "Collect", mock.Anything) } func TestParseDateRange(t *testing.T) { @@ -636,7 +636,7 @@ func TestHandler_getAnalyticsTrends_ScopedUser_RequiresAccountID(t *testing.T) { _, err := handler.getAnalyticsTrends(ctx, req, map[string]string{}) require.Error(t, err) assert.Contains(t, err.Error(), "account_id is required") - mockSnap.AssertNotCalled(t, "QueryMonthlyTotals") + mockSnap.AssertNotCalled(t, "QueryMonthlyTotals", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_getAnalyticsTrends_ScopedUser_OutsideScope returns not-found when @@ -659,5 +659,5 @@ func TestHandler_getAnalyticsTrends_ScopedUser_OutsideScope(t *testing.T) { _, err := handler.getAnalyticsTrends(ctx, req, map[string]string{"account_id": "other-acct"}) require.Error(t, err) - mockSnap.AssertNotCalled(t, "QueryMonthlyTotals") + mockSnap.AssertNotCalled(t, "QueryMonthlyTotals", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index eccaf9b60..02e87ff36 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -965,7 +965,7 @@ func TestHandler_updateProfile_RejectsInvalidEmail(t *testing.T) { assert.Equal(t, 400, ce.code) assert.Contains(t, ce.message, "email") // Confirm UpdateUserProfile was never reached. - mockAuth.AssertNotCalled(t, "UpdateUserProfile") + mockAuth.AssertNotCalled(t, "UpdateUserProfile", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_updateProfile_AcceptsValidEmail verifies that a well-formed diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index c98ca538f..e016f58a4 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -363,7 +363,7 @@ func TestPerAccountPerms_HistoryAnalytics_CrossAccountRejected(t *testing.T) { "cross-account analytics must return 404; got: %v", err) // The analytics backend must never be called — the scope check fires first. - mockClient.AssertNotCalled(t, "QueryHistory") + mockClient.AssertNotCalled(t, "QueryHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestPerAccountPerms_HistoryAnalytics_AllowedAccountSucceeds is the paired @@ -463,7 +463,7 @@ func TestPerAccountPerms_HistoryBreakdown_CrossAccountRejected(t *testing.T) { assert.True(t, IsNotFoundError(err), "cross-account breakdown must return 404; got: %v", err) - mockClient.AssertNotCalled(t, "QueryBreakdown") + mockClient.AssertNotCalled(t, "QueryBreakdown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // ─── 6. GET /dashboard/summary ─────────────────────────────────────────────── diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 5878c99c2..dfbd6542e 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -153,7 +153,7 @@ func TestHandler_approvePurchase_RejectsMismatchedSession(t *testing.T) { // asserts nothing by construction; a .On(...) entry above would create // a false positive, so we pin the negative by confirming the error is // the authz error, not an approval-manager error. - mockPurchase.AssertNotCalled(t, "ApproveExecution") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_approvePurchase_RejectsMissingContactEmail covers the @@ -204,7 +204,7 @@ func TestHandler_approvePurchase_RejectsMissingContactEmail(t *testing.T) { _, err := handler.approvePurchase(ctx, req, execID, "valid-token") require.Error(t, err) assert.Contains(t, err.Error(), "no per-account contact email") - mockPurchase.AssertNotCalled(t, "ApproveExecution") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestHandler_approvePurchase_RejectsMissingSession(t *testing.T) { @@ -328,7 +328,7 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) require.NoError(t, err) assert.Equal(t, "completed", result.(map[string]string)["status"]) mockPurchase.AssertExpectations(t) - mockPurchase.AssertNotCalled(t, "ApproveExecution") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409 pins the @@ -678,7 +678,7 @@ func TestHandler_approvePurchase_RejectsGlobalNotifyWhenContactSet(t *testing.T) _, err := handler.approvePurchase(ctx, req, execID, "valid-token") require.Error(t, err) assert.Contains(t, err.Error(), "not the authorized approver") - mockPurchase.AssertNotCalled(t, "ApproveExecution") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission is the @@ -741,8 +741,8 @@ func TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission(t *testi require.True(t, ok, "expected a ClientError, got %T: %v", err, err) assert.Equal(t, 403, ce.code, "creator without approve permission must be denied 403") // Purchase manager must never be reached. - mockPurchase.AssertNotCalled(t, "ApproveExecution") - mockPurchase.AssertNotCalled(t, "ApproveAndExecute") + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestRouter_approvePurchaseHandler_RateLimited is a regression test for issue #400. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 11fb15e81..d2000b000 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -968,7 +968,7 @@ func TestListExchangeableAzureRIs_SubscriptionIDOutOfScope(t *testing.T) { // AssertExpectations above passes whether it was called or not; assert it // explicitly to prove the scope gate refuses the request before the // tenant-wide listing is ever fetched. - opsClient.AssertNotCalled(t, "ListExchangeableReservations") + opsClient.AssertNotCalled(t, "ListExchangeableReservations", mock.Anything) } // TestListExchangeableAzureRIs_SubscriptionIDFiltersToOwnRows exercises diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 3ea7d25df..88e330954 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -766,7 +766,7 @@ func TestService_Logout_EmptyToken(t *testing.T) { assert.Contains(t, err.Error(), "token is required") // DeleteSession must not be called - mockStore.AssertNotCalled(t, "DeleteSession") + mockStore.AssertNotCalled(t, "DeleteSession", mock.Anything, mock.Anything) } func TestService_Logout_NilStore(t *testing.T) { diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go index 3eaf9937d..bd412cbf3 100644 --- a/internal/email/mute_test.go +++ b/internal/email/mute_test.go @@ -57,7 +57,7 @@ func TestSendPurchaseApprovalRequest_MutedRecipient_NoSESCall(t *testing.T) { Return(true, nil) t.Cleanup(func() { mc.AssertExpectations(t) - ses.AssertNotCalled(t, "SendEmail") + ses.AssertNotCalled(t, "SendEmail", mock.Anything, mock.Anything) }) s := newSenderWithMute(ses, mc) @@ -146,7 +146,7 @@ func TestSendRIExchangePendingApproval_MutedRecipient_NoSESCall(t *testing.T) { Return(true, nil).Once() t.Cleanup(func() { mc.AssertExpectations(t) - ses.AssertNotCalled(t, "SendEmail") + ses.AssertNotCalled(t, "SendEmail", mock.Anything, mock.Anything) }) s := newSenderWithMute(ses, mc) diff --git a/internal/mocks/vacuous_assertion_guard_test.go b/internal/mocks/vacuous_assertion_guard_test.go new file mode 100644 index 000000000..c81ea9a7e --- /dev/null +++ b/internal/mocks/vacuous_assertion_guard_test.go @@ -0,0 +1,488 @@ +package mocks + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// This guard closes the class of defect behind #1595 and #1740 for code not yet +// written, rather than only the instances that exist today. +// +// testify's AssertCalled / AssertNotCalled diff the matchers they are given +// against the arguments a mock recorded. An empty matcher list is diffed too, +// and each real argument counts as a difference, so a name-only +// AssertNotCalled(t, "Method") never matches a method that passes arguments to +// m.Called: it reports success no matter what the code under test did. The same +// goes for any matcher count that differs from that method's real arity. +// +// A mock may opt out by defining its own AssertCalled / AssertNotCalled -- as +// MockConfigStore does in assertions.go, where the shadowed versions read a +// callLog and name-only deliberately means "not called at all". That opt-out is +// detected, not hardcoded, so a future mock that adopts the same pattern is +// covered automatically. +// +// The check is syntactic and package-scoped: a mock and the tests that use it +// live in the same package throughout this repo. It parses rather than builds, +// so it costs milliseconds. + +// mockType is one testify mock in one package. +type mockType struct { + // calledArity maps method name -> number of arguments the method hands to + // m.Called. This is what testify diffs against, and it is NOT always the + // signature arity: MockSESClient.SendEmail takes three parameters but calls + // m.Called(ctx, input). + calledArity map[string]int + // shadows is true when the type defines its own assertion helpers. + shadows bool +} + +type assertSite struct { + pos token.Position + recv string + assertFn string + method string + matchers int +} + +func TestNoUnfailableMockAssertions(t *testing.T) { + root := repoRoot(t) + + pkgFiles := map[string][]string{} + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", "node_modules", "vendor", ".terraform": + return filepath.SkipDir + } + return nil + } + if strings.HasSuffix(path, ".go") { + dir := filepath.Dir(path) + pkgFiles[dir] = append(pkgFiles[dir], path) + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + + var findings []string + var skipped []string + checked := 0 + for _, dir := range sortedKeys(pkgFiles) { + fset := token.NewFileSet() + var files []*ast.File + for _, p := range pkgFiles[dir] { + f, err := parser.ParseFile(fset, p, nil, 0) + if err != nil { + // A file that does not parse is the compiler's problem, not this + // guard's; skipping it cannot hide a vacuous assertion, because a + // package that does not compile has no passing tests either. + continue + } + files = append(files, f) + } + mocks := collectMocks(files) + if len(mocks) == 0 { + continue + } + for _, f := range files { + for _, site := range collectAssertSites(fset, f) { + recvType, resolved := resolveRecvType(f, site) + m, isMock := mocks[recvType] + if resolved && !isMock { + // Resolved to a concrete type that is not a testify mock. + // Nothing to say about it, and no blind spot either. + continue + } + if !resolved { + // The receiver was not assigned from a mock literal in this + // file. Only report it when the method name belongs to some + // mock in the package and would be unfailable if it were one, + // so an unreviewed blind spot cannot hide behind a pass. + if unresolvedLooksRisky(site, mocks) { + skipped = append(skipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- receiver type could not be resolved; "+ + "review this site by hand", site.pos, site.recv, site.assertFn, site.method)) + } + continue + } + arity, known := m.calledArity[site.method] + if !known || m.shadows { + continue + } + checked++ + if arity == 0 { + continue + } + switch { + case site.matchers == 0: + findings = append(findings, fmt.Sprintf( + "%s: %s.%s(t, %q) passes no matchers, but %s hands %d argument(s) to m.Called. "+ + "testify diffs the empty matcher list against those arguments and counts each as a "+ + "difference, so this assertion can never fail. Pass %d matcher(s) (mock.Anything is fine).", + site.pos, site.recv, site.assertFn, site.method, site.method, arity, arity)) + case site.matchers != arity: + findings = append(findings, fmt.Sprintf( + "%s: %s.%s(t, %q) passes %d matcher(s), but %s hands %d argument(s) to m.Called. "+ + "A count that differs from the real arity can never match, so this assertion can "+ + "never fail. Pass %d matcher(s).", + site.pos, site.recv, site.assertFn, site.method, site.matchers, site.method, arity, arity)) + } + } + } + } + + if checked == 0 { + t.Fatal("guard checked no assertion sites; its mock or receiver detection has broken " + + "and it would report success on a repo full of unfailable assertions") + } + t.Logf("checked %d mock assertion site(s)", checked) + + // An unresolved receiver is not proof of safety. Surface it rather than + // letting the count quietly shrink. + if len(skipped) > 0 { + sort.Strings(skipped) + t.Errorf("%d assertion site(s) could not be checked:\n\n%s", + len(skipped), strings.Join(skipped, "\n\n")) + } + + if len(findings) > 0 { + sort.Strings(findings) + t.Errorf("%d unfailable mock assertion(s):\n\n%s", len(findings), strings.Join(findings, "\n\n")) + } +} + +// collectMocks finds every type in the package that has at least one method +// dispatching through m.Called, which is what makes it a testify mock for the +// purposes of this check. +func collectMocks(files []*ast.File) map[string]*mockType { + mocks := map[string]*mockType{} + for _, f := range files { + for _, d := range f.Decls { + fd, ok := d.(*ast.FuncDecl) + if !ok || fd.Recv == nil || len(fd.Recv.List) != 1 || fd.Body == nil { + continue + } + typeName := recvTypeIdent(fd.Recv.List[0].Type) + if typeName == "" { + continue + } + m := mocks[typeName] + if m == nil { + m = &mockType{calledArity: map[string]int{}} + mocks[typeName] = m + } + if fd.Name.Name == "AssertCalled" || fd.Name.Name == "AssertNotCalled" { + m.shadows = true + } + 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 + } + switch sel.Sel.Name { + case "Called": + m.calledArity[fd.Name.Name] = len(call.Args) + case "MethodCalled": + if len(call.Args) >= 1 { + m.calledArity[fd.Name.Name] = len(call.Args) - 1 + } + } + return true + }) + } + } + // A type with no m.Called anywhere is not a mock. + for name, m := range mocks { + if len(m.calledArity) == 0 { + delete(mocks, name) + } + } + return mocks +} + +func collectAssertSites(fset *token.FileSet, f *ast.File) []assertSite { + var out []assertSite + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if sel.Sel.Name != "AssertCalled" && sel.Sel.Name != "AssertNotCalled" { + return true + } + recv, ok := sel.X.(*ast.Ident) + if !ok || len(call.Args) < 2 { + return true + } + lit, ok := call.Args[1].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + if call.Ellipsis.IsValid() { + // AssertNotCalled(t, "M", args...) spreads a slice whose length is + // not knowable here. Counting the spread expression as one matcher + // would invent a mismatch, so the site is left alone. + return true + } + out = append(out, assertSite{ + pos: fset.Position(call.Pos()), + recv: recv.Name, + assertFn: sel.Sel.Name, + method: strings.Trim(lit.Value, `"`), + matchers: len(call.Args) - 2, + }) + return true + }) + return out +} + +// resolveRecvType maps the assertion's receiver identifier back to the type it +// was assigned from within the file: mockStore := &MockX{}, new(MockX), or +// MockX{}. It reports the type name whether or not that type is a mock, so the +// caller can tell "this is not a mock" (nothing to check) apart from "we could +// not tell what this is" (an unreviewed blind spot). +func resolveRecvType(f *ast.File, site assertSite) (string, bool) { + var found string + ast.Inspect(f, func(n ast.Node) bool { + var lhs, rhs []ast.Expr + switch v := n.(type) { + case *ast.AssignStmt: + lhs, rhs = v.Lhs, v.Rhs + case *ast.ValueSpec: + for _, name := range v.Names { + lhs = append(lhs, name) + } + rhs = v.Values + default: + return true + } + if len(lhs) != len(rhs) { + return true + } + for i, l := range lhs { + id, ok := l.(*ast.Ident) + if !ok || id.Name != site.recv { + continue + } + if name := mockTypeOfExpr(rhs[i]); name != "" { + found = name + } + } + return true + }) + return found, found != "" +} + +// mockTypeOfExpr extracts T from &T{...}, T{...} and new(T). +func mockTypeOfExpr(e ast.Expr) string { + switch v := e.(type) { + case *ast.UnaryExpr: + if v.Op == token.AND { + return mockTypeOfExpr(v.X) + } + case *ast.CompositeLit: + if id, ok := v.Type.(*ast.Ident); ok { + return id.Name + } + case *ast.CallExpr: + if id, ok := v.Fun.(*ast.Ident); ok && id.Name == "new" && len(v.Args) == 1 { + if t, ok := v.Args[0].(*ast.Ident); ok { + return t.Name + } + } + } + return "" +} + +func recvTypeIdent(e ast.Expr) string { + if star, ok := e.(*ast.StarExpr); ok { + e = star.X + } + if id, ok := e.(*ast.Ident); ok { + return id.Name + } + return "" +} + +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("no go.mod found above the working directory") + } + dir = parent + } +} + +func sortedKeys(m map[string][]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// unresolvedLooksRisky reports whether an assertion whose receiver could not be +// resolved names a method that some mock in the package implements with a +// non-zero m.Called arity and without shadowing the helpers. Those are the +// unresolved sites that could be hiding the defect. +func unresolvedLooksRisky(site assertSite, mocks map[string]*mockType) bool { + if site.matchers > 0 { + // A site carrying matchers is only wrong if the count is off, which + // cannot be judged without knowing which mock it is. Ambiguous rather + // than risky, and reporting every one would drown the real findings. + return false + } + for _, m := range mocks { + if m.shadows { + continue + } + if arity, ok := m.calledArity[site.method]; ok && arity > 0 { + return true + } + } + return false +} + +// TestVacuousAssertionGuardDetects exercises the guard's analysis on synthetic +// source covering each case it must separate. Without this, the guard could +// break into reporting nothing and the repo-wide run would still look green -- +// the exact failure mode #1595 was about. +func TestVacuousAssertionGuardDetects(t *testing.T) { + const src = `package p + +type MockThing struct{ mock.Mock } + +func (m *MockThing) Two(a, b int) error { return m.Called(a, b).Error(0) } +func (m *MockThing) None() error { return m.Called().Error(0) } + +type MockShadowed struct{ mock.Mock } + +func (m *MockShadowed) Two(a, b int) error { return m.Called(a, b).Error(0) } +func (m *MockShadowed) AssertNotCalled(t T, s string, a ...interface{}) bool { return true } + +type NotAMock struct{} + +func (n *NotAMock) Two(a, b int) error { return nil } + +func Test(t *testing.T) { + thing := &MockThing{} + thing.AssertNotCalled(t, "Two") // want: vacuous + thing.AssertNotCalled(t, "Two", mock.Anything) // want: wrong count + thing.AssertNotCalled(t, "Two", mock.Anything, mock.Anything) // want: ok + thing.AssertCalled(t, "Two") // want: vacuous + thing.AssertNotCalled(t, "None") // want: ok, arity 0 + + shadowed := new(MockShadowed) + shadowed.AssertNotCalled(t, "Two") // want: ok, type shadows the helpers + + plain := &NotAMock{} + plain.AssertNotCalled(t, "Two") // want: ignored, not a mock + + unknown.AssertNotCalled(t, "Two") // want: reported as unresolved + + spread := &MockThing{} + spread.AssertNotCalled(t, "Two", anyArgs...) // want: ignored, count unknowable +} +` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "synthetic.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + files := []*ast.File{f} + mocks := collectMocks(files) + + if _, ok := mocks["NotAMock"]; ok { + t.Error("NotAMock has no m.Called and must not be treated as a mock") + } + if m, ok := mocks["MockThing"]; !ok { + t.Fatal("MockThing not detected as a mock") + } else { + if got := m.calledArity["Two"]; got != 2 { + t.Errorf("MockThing.Two arity = %d, want 2", got) + } + if got, ok := m.calledArity["None"]; !ok || got != 0 { + t.Errorf("MockThing.None arity = %d (present=%v), want 0", got, ok) + } + if m.shadows { + t.Error("MockThing does not define its own helpers and must not count as shadowing") + } + } + if m, ok := mocks["MockShadowed"]; !ok || !m.shadows { + t.Error("MockShadowed defines AssertNotCalled and must count as shadowing") + } + + type verdict struct { + method string + matchers int + vacuous bool + } + var got []verdict + unresolved := 0 + for _, site := range collectAssertSites(fset, f) { + recvType, resolved := resolveRecvType(f, site) + m, isMock := mocks[recvType] + if resolved && !isMock { + continue + } + if !resolved { + if unresolvedLooksRisky(site, mocks) { + unresolved++ + } + continue + } + arity, known := m.calledArity[site.method] + if !known || m.shadows || arity == 0 { + continue + } + got = append(got, verdict{site.method, site.matchers, site.matchers != arity}) + } + + want := []verdict{ + {"Two", 0, true}, // name-only + {"Two", 1, true}, // wrong count + {"Two", 2, false}, // correct + {"Two", 0, true}, // AssertCalled, name-only + } + if len(got) != len(want) { + t.Fatalf("checked %d site(s), want %d: %+v", len(got), len(want), got) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("site %d = %+v, want %+v", i, got[i], want[i]) + } + } + if unresolved != 1 { + t.Errorf("unresolved risky sites = %d, want 1 (the `unknown` receiver)", unresolved) + } +} diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 502ca53f2..95add7547 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -660,7 +660,7 @@ func TestExecuteForAccount_CredentialFailure_MarksFailed(t *testing.T) { // The provider factory must NEVER be invoked when credentials // fail to resolve. If this fires, something is attempting an // ambient-credential fallback — exactly the bug 9531681a4 closed. - mockFactory.AssertNotCalled(t, "CreateAndValidateProvider") + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) }) } } @@ -726,7 +726,7 @@ func TestExecuteForAccount_CredentialFailure_SaveErrorSurfaced(t *testing.T) { "the original credential failure must not be masked by the save failure") // No provider may ever be constructed when credentials fail to resolve. - mockFactory.AssertNotCalled(t, "CreateAndValidateProvider") + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) } @@ -1819,7 +1819,7 @@ func TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulti // PurchaseCommitment must NEVER be called: assert the factory was never // invoked, which is the clearest proxy for "no cloud API call happened". - mockFactory.AssertNotCalled(t, "CreateAndValidateProvider") + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) t.Cleanup(func() { mockStore.AssertExpectations(t) }) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 317415118..5289b980e 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -627,7 +627,7 @@ func TestScheduler_CollectRecommendations_WithNotification(t *testing.T) { assert.Equal(t, 0, result.Recommendations) // Verify no email was sent - mockEmail.AssertNotCalled(t, "SendNewRecommendationsNotification") + mockEmail.AssertNotCalled(t, "SendNewRecommendationsNotification", mock.Anything, mock.Anything) } // Test that verifies the struct implements expected interface. diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index 6594c37a5..5ed7422e5 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -387,7 +387,7 @@ func TestClient_findOfferingID_RejectsMismatchedPlanType(t *testing.T) { assert.Contains(t, err.Error(), "does not match client scope") // AWS API must not be called — the mismatch should be caught // client-side before any DescribeSavingsPlansOfferings request. - mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) } func TestClient_GetValidResourceTypes(t *testing.T) { @@ -973,7 +973,7 @@ func TestClient_FindOfferingID_AllPaymentOptions(t *testing.T) { if tt.expectError { require.Error(t, err) assert.Contains(t, err.Error(), "unsupported Savings Plans payment option") - mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) } else { assert.NoError(t, err) } @@ -1029,7 +1029,7 @@ func TestClient_FindOfferingID_TermVariations(t *testing.T) { if tt.expectError { require.Error(t, err) assert.Contains(t, err.Error(), "unsupported Savings Plans term") - mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) } else { assert.NoError(t, err) } @@ -1532,7 +1532,7 @@ func TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly(t *testing.T) { } // DescribeSavingsPlansOfferings must NOT be called when CE supplies the ID. - mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) id, err := client.findOfferingID(context.Background(), rec, "test-exec") require.NoError(t, err) From 67cced118952e1ddd900a231ad5321b1155f04b2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:16:51 +0200 Subject: [PATCH 2/4] 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 --- .../mocks/vacuous_assertion_guard_test.go | 222 +++++++++++++++--- 1 file changed, 192 insertions(+), 30 deletions(-) diff --git a/internal/mocks/vacuous_assertion_guard_test.go b/internal/mocks/vacuous_assertion_guard_test.go index c81ea9a7e..6d2721134 100644 --- a/internal/mocks/vacuous_assertion_guard_test.go +++ b/internal/mocks/vacuous_assertion_guard_test.go @@ -28,9 +28,12 @@ import ( // detected, not hardcoded, so a future mock that adopts the same pattern is // covered automatically. // -// The check is syntactic and package-scoped: a mock and the tests that use it -// live in the same package throughout this repo. It parses rather than builds, -// so it costs milliseconds. +// The check is syntactic. Mocks are resolved within the using package first and +// then, for mocks declared elsewhere and aliased in (MockConfigStore is the +// repo's one such case), through a repo-wide index by type name. A receiver it +// still cannot place is reported rather than skipped: a skip that says nothing +// is the same shape of defect the guard exists to catch. It parses rather than +// builds, so it costs milliseconds. // mockType is one testify mock in one package. type mockType struct { @@ -39,8 +42,12 @@ type mockType struct { // signature arity: MockSESClient.SendEmail takes three parameters but calls // m.Called(ctx, input). calledArity map[string]int - // shadows is true when the type defines its own assertion helpers. - shadows bool + // shadowed records which assertion helpers the type defines itself, keyed by + // helper name. Per-helper rather than per-type: a mock that shadows only + // AssertNotCalled leaves AssertCalled going through testify, where the + // name-only form is still unfailable, so exempting the whole type would + // hand back the very hole this guard closes. + shadowed map[string]bool } type assertSite struct { @@ -76,9 +83,15 @@ func TestNoUnfailableMockAssertions(t *testing.T) { t.Fatalf("walk %s: %v", root, err) } - var findings []string - var skipped []string - checked := 0 + // Mocks are usually declared in the package that uses them, but not always: + // MockConfigStore lives in internal/mocks and is aliased into internal/api, + // internal/purchase and internal/scheduler. A per-package map alone does not + // recognize it there, so those sites would be skipped -- and a skip that + // reports nothing is the very defect this guard exists to catch. Index every + // mock in the repo by type name as a fallback for exactly that case. + global := map[string][]*mockType{} + parsed := map[string][]*ast.File{} + fsets := map[string]*token.FileSet{} for _, dir := range sortedKeys(pkgFiles) { fset := token.NewFileSet() var files []*ast.File @@ -92,6 +105,19 @@ func TestNoUnfailableMockAssertions(t *testing.T) { } files = append(files, f) } + parsed[dir] = files + fsets[dir] = fset + for name, m := range collectMocks(files) { + global[name] = append(global[name], m) + } + } + + var findings []string + var skipped []string + checked := 0 + for _, dir := range sortedKeys(pkgFiles) { + fset := fsets[dir] + files := parsed[dir] mocks := collectMocks(files) if len(mocks) == 0 { continue @@ -101,24 +127,43 @@ func TestNoUnfailableMockAssertions(t *testing.T) { recvType, resolved := resolveRecvType(f, site) m, isMock := mocks[recvType] if resolved && !isMock { - // Resolved to a concrete type that is not a testify mock. - // Nothing to say about it, and no blind spot either. - continue + var reason string + m, isMock, reason = resolveCrossPackage(global, recvType, site.method) + if !isMock { + skipped = append(skipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- receiver resolves to %s, which this check "+ + "could not analyze (%s). Review by hand.", + site.pos, site.recv, site.assertFn, site.method, recvType, reason)) + continue + } } if !resolved { // The receiver was not assigned from a mock literal in this // file. Only report it when the method name belongs to some // mock in the package and would be unfailable if it were one, // so an unreviewed blind spot cannot hide behind a pass. - if unresolvedLooksRisky(site, mocks) { + if unresolvedLooksRisky(site, mocks, global) { skipped = append(skipped, fmt.Sprintf( "%s: %s.%s(t, %q) -- receiver type could not be resolved; "+ "review this site by hand", site.pos, site.recv, site.assertFn, site.method)) } continue } + if m.shadowed[site.assertFn] { + // This type implements the helper being used, so it defines + // what the matchers mean. See MockConfigStore in assertions.go. + continue + } arity, known := m.calledArity[site.method] - if !known || m.shadows { + if !known { + // The method exists on a mock but never calls m.Called itself + // (it delegates to a sibling), so there is no arity to check + // against. Report it: "checked nothing" must never be silent, + // which is the same reason checked == 0 is fatal below. + skipped = append(skipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- %s does not call m.Called directly, so its "+ + "argument count cannot be derived. Review by hand.", + site.pos, site.recv, site.assertFn, site.method, site.method)) continue } checked++ @@ -180,11 +225,11 @@ func collectMocks(files []*ast.File) map[string]*mockType { } m := mocks[typeName] if m == nil { - m = &mockType{calledArity: map[string]int{}} + m = &mockType{calledArity: map[string]int{}, shadowed: map[string]bool{}} mocks[typeName] = m } if fd.Name.Name == "AssertCalled" || fd.Name.Name == "AssertNotCalled" { - m.shadows = true + m.shadowed[fd.Name.Name] = true } ast.Inspect(fd.Body, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) @@ -355,24 +400,47 @@ func sortedKeys(m map[string][]string) []string { // resolved names a method that some mock in the package implements with a // non-zero m.Called arity and without shadowing the helpers. Those are the // unresolved sites that could be hiding the defect. -func unresolvedLooksRisky(site assertSite, mocks map[string]*mockType) bool { - if site.matchers > 0 { - // A site carrying matchers is only wrong if the count is off, which - // cannot be judged without knowing which mock it is. Ambiguous rather - // than risky, and reporting every one would drown the real findings. - return false +func unresolvedLooksRisky(site assertSite, mocks map[string]*mockType, global map[string][]*mockType) bool { + // Any matcher count can be wrong, not just zero: a non-zero count that does + // not match the real arity is the third vacuity mode from #1735. Both are + // reported, since neither can be judged without knowing the type. + looks := func(m *mockType) bool { + if m.shadowed[site.assertFn] { + return false + } + arity, ok := m.calledArity[site.method] + return ok && arity > 0 } for _, m := range mocks { - if m.shadows { - continue - } - if arity, ok := m.calledArity[site.method]; ok && arity > 0 { + if looks(m) { return true } } + for _, defs := range global { + for _, m := range defs { + if looks(m) { + return true + } + } + } return false } +// sameShadowing reports whether two same-named mocks define the same set of +// assertion helpers, so the cross-package fallback does not exempt a site on one +// mock's opt-out while the real type has none. +func sameShadowing(a, b *mockType) bool { + if len(a.shadowed) != len(b.shadowed) { + return false + } + for k := range a.shadowed { + if !b.shadowed[k] { + return false + } + } + return true +} + // TestVacuousAssertionGuardDetects exercises the guard's analysis on synthetic // source covering each case it must separate. Without this, the guard could // break into reporting nothing and the repo-wide run would still look green -- @@ -434,12 +502,15 @@ func Test(t *testing.T) { if got, ok := m.calledArity["None"]; !ok || got != 0 { t.Errorf("MockThing.None arity = %d (present=%v), want 0", got, ok) } - if m.shadows { + if len(m.shadowed) != 0 { t.Error("MockThing does not define its own helpers and must not count as shadowing") } } - if m, ok := mocks["MockShadowed"]; !ok || !m.shadows { - t.Error("MockShadowed defines AssertNotCalled and must count as shadowing") + if m, ok := mocks["MockShadowed"]; !ok || !m.shadowed["AssertNotCalled"] { + t.Error("MockShadowed defines AssertNotCalled and must be recorded as shadowing it") + } else if m.shadowed["AssertCalled"] { + t.Error("MockShadowed does not define AssertCalled; shadowing must be per-helper, " + + "or its name-only AssertCalled sites would be exempted while still unfailable") } type verdict struct { @@ -447,6 +518,13 @@ func Test(t *testing.T) { matchers int vacuous bool } + // The synthetic source is a single package, so the repo-wide index used by + // the real run is just this package's mocks. + global := map[string][]*mockType{} + for name, m := range mocks { + global[name] = append(global[name], m) + } + var got []verdict unresolved := 0 for _, site := range collectAssertSites(fset, f) { @@ -456,13 +534,13 @@ func Test(t *testing.T) { continue } if !resolved { - if unresolvedLooksRisky(site, mocks) { + if unresolvedLooksRisky(site, mocks, global) { unresolved++ } continue } arity, known := m.calledArity[site.method] - if !known || m.shadows || arity == 0 { + if m.shadowed[site.assertFn] || !known || arity == 0 { continue } got = append(got, verdict{site.method, site.matchers, site.matchers != arity}) @@ -486,3 +564,87 @@ func Test(t *testing.T) { t.Errorf("unresolved risky sites = %d, want 1 (the `unknown` receiver)", unresolved) } } + +// resolveCrossPackage finds a mock declared outside the package that uses it, +// keyed by type name. Type names are not unique across the repo (MockEmailSender +// and MockHTTPClient each have several unrelated definitions), so a single +// candidate is used directly and multiple candidates are accepted only when they +// agree about the method in question. Anything else is reported rather than +// assumed, because guessing here would silently attribute one mock's arity to +// another. +func resolveCrossPackage(global map[string][]*mockType, typeName, method string) (*mockType, bool, string) { + candidates := global[typeName] + switch len(candidates) { + case 0: + return nil, false, "no mock of that name is declared anywhere in the repo" + case 1: + return candidates[0], true, "" + } + first := candidates[0] + wantArity, wantKnown := first.calledArity[method] + for _, c := range candidates[1:] { + arity, known := c.calledArity[method] + if known != wantKnown || arity != wantArity || !sameShadowing(c, first) { + return nil, false, fmt.Sprintf( + "%d mocks share that name and disagree about %s", len(candidates), method) + } + } + return first, true, "" +} + +// TestResolveCrossPackage pins the fallback used for mocks declared outside the +// package that asserts on them. Before it existed, MockConfigStore was +// unrecognized in internal/api, internal/purchase and internal/scheduler, so +// every site there was silently skipped: absent from the checked count, absent +// from the report. Removing MockConfigStore's shadowed helpers took the guard +// from 6 findings to 56 once this landed, the 50 difference being exactly those +// cross-package sites. +// +// Type names are not unique across this repo (MockHTTPClient has five unrelated +// definitions), so the fallback must refuse to guess rather than attribute one +// mock's arity to another. +func TestResolveCrossPackage(t *testing.T) { + twoArgs := &mockType{calledArity: map[string]int{"Do": 2}, shadowed: map[string]bool{}} + twoArgsAgain := &mockType{calledArity: map[string]int{"Do": 2}, shadowed: map[string]bool{}} + threeArgs := &mockType{calledArity: map[string]int{"Do": 3}, shadowed: map[string]bool{}} + shadowed := &mockType{calledArity: map[string]int{"Do": 2}, shadowed: map[string]bool{"AssertNotCalled": true}} + + tests := []struct { + name string + candidates []*mockType + wantOK bool + }{ + {"single definition is used", []*mockType{twoArgs}, true}, + {"no definition anywhere", nil, false}, + {"duplicates that agree", []*mockType{twoArgs, twoArgsAgain}, true}, + {"duplicates disagreeing on arity", []*mockType{twoArgs, threeArgs}, false}, + {"duplicates disagreeing on shadowing", []*mockType{twoArgs, shadowed}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + global := map[string][]*mockType{"MockThing": tt.candidates} + got, ok, reason := resolveCrossPackage(global, "MockThing", "Do") + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v (reason %q)", ok, tt.wantOK, reason) + } + if ok && got == nil { + t.Error("resolved but returned no mock") + } + if !ok && reason == "" { + t.Error("refused to resolve but gave no reason; the skip would be silent") + } + }) + } +} + +// TestResolveCrossPackage_UnknownMethodStillDisagrees covers the case where one +// candidate implements the method and another does not: that is a disagreement, +// not a match, and must not resolve. +func TestResolveCrossPackage_UnknownMethodStillDisagrees(t *testing.T) { + has := &mockType{calledArity: map[string]int{"Do": 2}, shadowed: map[string]bool{}} + hasNot := &mockType{calledArity: map[string]int{"Other": 1}, shadowed: map[string]bool{}} + global := map[string][]*mockType{"MockThing": {has, hasNot}} + if _, ok, reason := resolveCrossPackage(global, "MockThing", "Do"); ok { + t.Errorf("resolved across candidates that disagree about Do (reason %q)", reason) + } +} From b90c9ebf55da130f902c1bb41d9b0fe4f5e90c7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:19:02 +0200 Subject: [PATCH 3/4] 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) --- .../mocks/vacuous_assertion_guard_test.go | 274 +++++++++++++++--- .../aws/services/savingsplans/client_test.go | 8 +- 2 files changed, 232 insertions(+), 50 deletions(-) diff --git a/internal/mocks/vacuous_assertion_guard_test.go b/internal/mocks/vacuous_assertion_guard_test.go index 6d2721134..7bc46a6f1 100644 --- a/internal/mocks/vacuous_assertion_guard_test.go +++ b/internal/mocks/vacuous_assertion_guard_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "testing" ) @@ -56,6 +57,17 @@ type assertSite struct { assertFn string method string matchers int + // unsupported is non-empty when the site was recognized as a mock + // assertion but has a shape this check cannot analyze. Such sites are + // reported, never dropped: silently ignoring a shape is the same defect + // the guard exists to catch. + unsupported string + // scopes is the chain of function bodies enclosing the site, innermost + // first. Resolution walks it outward like Go's own lexical scoping: a + // t.Cleanup closure that references a mock declared in the enclosing test + // still resolves, while two tests in one file that both declare mockStore + // no longer bleed into each other. + scopes []ast.Node } func TestNoUnfailableMockAssertions(t *testing.T) { @@ -124,7 +136,7 @@ func TestNoUnfailableMockAssertions(t *testing.T) { } for _, f := range files { for _, site := range collectAssertSites(fset, f) { - recvType, resolved := resolveRecvType(f, site) + recvType, resolved := resolveRecvType(site.scopes, site) m, isMock := mocks[recvType] if resolved && !isMock { var reason string @@ -263,52 +275,137 @@ func collectMocks(files []*ast.File) map[string]*mockType { func collectAssertSites(fset *token.FileSet, f *ast.File) []assertSite { var out []assertSite - ast.Inspect(f, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return true - } - if sel.Sel.Name != "AssertCalled" && sel.Sel.Name != "AssertNotCalled" { - return true - } - recv, ok := sel.X.(*ast.Ident) - if !ok || len(call.Args) < 2 { - return true - } - lit, ok := call.Args[1].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - return true - } - if call.Ellipsis.IsValid() { - // AssertNotCalled(t, "M", args...) spreads a slice whose length is - // not knowable here. Counting the spread expression as one matcher - // would invent a mismatch, so the site is left alone. + // Innermost enclosing function body for each site, so receiver resolution + // is scoped rather than file-wide. + var stack []ast.Node + var walk func(n ast.Node) bool + walk = func(n ast.Node) bool { + switch v := n.(type) { + case *ast.FuncDecl: + if v.Body != nil { + stack = append(stack, v.Body) + defer func() { stack = stack[:len(stack)-1] }() + ast.Inspect(v.Body, walk) + return false + } + case *ast.FuncLit: + if v.Body != nil { + stack = append(stack, v.Body) + defer func() { stack = stack[:len(stack)-1] }() + ast.Inspect(v.Body, walk) + return false + } + case *ast.CallExpr: + sel, ok := v.Fun.(*ast.SelectorExpr) + if !ok || (sel.Sel.Name != "AssertCalled" && sel.Sel.Name != "AssertNotCalled") { + return true + } + // Copy innermost-first; stack is mutated as the walk unwinds. + chain := make([]ast.Node, 0, len(stack)) + for i := len(stack) - 1; i >= 0; i-- { + chain = append(chain, stack[i]) + } + site := assertSite{pos: fset.Position(v.Pos()), assertFn: sel.Sel.Name, scopes: chain} + + recv, ok := sel.X.(*ast.Ident) + if !ok { + site.recv = exprString(sel.X) + site.unsupported = "receiver is not a plain identifier (e.g. a field or nested selector)" + out = append(out, site) + return true + } + site.recv = recv.Name + + if len(v.Args) < 2 { + site.unsupported = "fewer than two arguments; not a well-formed mock assertion" + out = append(out, site) + return true + } + lit, ok := v.Args[1].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + site.unsupported = "method name is not a string literal" + out = append(out, site) + return true + } + // strconv.Unquote rather than strings.Trim: Trim mishandles escapes + // and silently accepts a raw-string literal, which is also + // token.STRING but has different contents. + name, err := strconv.Unquote(lit.Value) + if err != nil { + site.unsupported = "method name literal could not be unquoted: " + err.Error() + out = append(out, site) + return true + } + site.method = name + if v.Ellipsis.IsValid() { + // AssertNotCalled(t, "M", args...) spreads a slice whose length + // is not knowable here. Counting the spread expression as one + // matcher would invent a mismatch. + site.unsupported = "matchers are passed as a variadic spread; the count is not statically known" + out = append(out, site) + return true + } + site.matchers = len(v.Args) - 2 + out = append(out, site) return true } - out = append(out, assertSite{ - pos: fset.Position(call.Pos()), - recv: recv.Name, - assertFn: sel.Sel.Name, - method: strings.Trim(lit.Value, `"`), - matchers: len(call.Args) - 2, - }) return true - }) + } + ast.Inspect(f, walk) return out } -// resolveRecvType maps the assertion's receiver identifier back to the type it -// was assigned from within the file: mockStore := &MockX{}, new(MockX), or -// MockX{}. It reports the type name whether or not that type is a mock, so the -// caller can tell "this is not a mock" (nothing to check) apart from "we could -// not tell what this is" (an unreviewed blind spot). -func resolveRecvType(f *ast.File, site assertSite) (string, bool) { +// exprString renders a receiver expression for a report message. +func exprString(e ast.Expr) string { + switch v := e.(type) { + case *ast.Ident: + return v.Name + case *ast.SelectorExpr: + return exprString(v.X) + "." + v.Sel.Name + case *ast.CallExpr: + return exprString(v.Fun) + "()" + case *ast.IndexExpr: + return exprString(v.X) + "[...]" + } + return "" +} + +// resolveRecvType maps the assertion's receiver identifier to the type it was +// assigned from, searching ONLY the innermost function body containing the site. +// A file-wide search would keep the last matching assignment, so two tests in +// one file that both name a variable mockStore would resolve every site to +// whichever type happened to appear last -- comparing matcher counts against the +// wrong arity and either inventing a finding or accepting a real one. This repo +// reuses names like mockStore and mockFactory across many tests in a file, so +// that is a live hazard, not a theoretical one. +// +// Conflicting bindings inside a single scope return unresolved rather than a +// guess: the site then reaches the skipped report, which is the direction this +// check degrades in everywhere else. +func resolveRecvType(scopes []ast.Node, site assertSite) (string, bool) { + for _, scope := range scopes { + name, ok, conflict := bindingInScope(scope, site.recv) + if conflict { + return "", false + } + if ok { + return name, true + } + } + return "", false +} + +// bindingInScope looks for assignments of name directly within one function +// body, without descending into nested function literals (those are their own +// scopes and are searched separately). It reports the bound mock type, whether +// one was found, and whether the scope binds the name to two different types. +func bindingInScope(scope ast.Node, recv string) (string, bool, bool) { var found string - ast.Inspect(f, func(n ast.Node) bool { + conflict := false + ast.Inspect(scope, func(n ast.Node) bool { + if fl, ok := n.(*ast.FuncLit); ok && fl.Body != scope { + return false + } var lhs, rhs []ast.Expr switch v := n.(type) { case *ast.AssignStmt: @@ -326,16 +423,21 @@ func resolveRecvType(f *ast.File, site assertSite) (string, bool) { } for i, l := range lhs { id, ok := l.(*ast.Ident) - if !ok || id.Name != site.recv { + if !ok || id.Name != recv { continue } - if name := mockTypeOfExpr(rhs[i]); name != "" { - found = name + name := mockTypeOfExpr(rhs[i]) + if name == "" { + continue } + if found != "" && found != name { + conflict = true + } + found = name } return true }) - return found, found != "" + return found, found != "", conflict } // mockTypeOfExpr extracts T from &T{...}, T{...} and new(T). @@ -479,7 +581,15 @@ func Test(t *testing.T) { unknown.AssertNotCalled(t, "Two") // want: reported as unresolved spread := &MockThing{} - spread.AssertNotCalled(t, "Two", anyArgs...) // want: ignored, count unknowable + spread.AssertNotCalled(t, "Two", anyArgs...) // want: unsupported, count unknowable + + h.mockThing.AssertNotCalled(t, "Two") // want: unsupported, receiver is a selector + thing.AssertNotCalled(t, methodName) // want: unsupported, method not a literal + + // A second test in the same file binding the same name to another type: the + // site above must not resolve to this one. + other := &MockShadowed{} + other.AssertNotCalled(t, "Two") } ` fset := token.NewFileSet() @@ -526,9 +636,13 @@ func Test(t *testing.T) { } var got []verdict - unresolved := 0 + unresolved, unsupported := 0, 0 for _, site := range collectAssertSites(fset, f) { - recvType, resolved := resolveRecvType(f, site) + if site.unsupported != "" { + unsupported++ + continue + } + recvType, resolved := resolveRecvType(site.scopes, site) m, isMock := mocks[recvType] if resolved && !isMock { continue @@ -545,6 +659,11 @@ func Test(t *testing.T) { } got = append(got, verdict{site.method, site.matchers, site.matchers != arity}) } + // Three shapes the check cannot analyze must be REPORTED, not dropped: + // a variadic spread, a selector receiver, and a non-literal method name. + if unsupported != 3 { + t.Errorf("unsupported shapes reported = %d, want 3 (spread, selector receiver, non-literal method)", unsupported) + } want := []verdict{ {"Two", 0, true}, // name-only @@ -648,3 +767,64 @@ func TestResolveCrossPackage_UnknownMethodStillDisagrees(t *testing.T) { t.Errorf("resolved across candidates that disagree about Do (reason %q)", reason) } } + +// TestResolveRecvTypeScoping pins the scoping rules for receiver resolution. +// Before this, the search covered the whole file and kept the LAST matching +// assignment, so two tests in one file both naming a variable mockStore +// resolved every site to whichever type appeared last — comparing the matcher +// count against the wrong arity, which either invents a finding or accepts a +// genuinely unfailable assertion. Wrong attribution is the failure direction +// that gets a guard deleted rather than fixed. +func TestResolveRecvTypeScoping(t *testing.T) { + const src = `package p + +func TestA(t *testing.T) { + mockStore := &MockAlpha{} + mockStore.AssertNotCalled(t, "Do") +} + +func TestB(t *testing.T) { + mockStore := &MockBeta{} + mockStore.AssertNotCalled(t, "Do") +} + +func TestClosure(t *testing.T) { + ses := &MockGamma{} + t.Cleanup(func() { + ses.AssertNotCalled(t, "Do") + }) +} + +func TestConflict(t *testing.T) { + dup := &MockAlpha{} + dup = &MockBeta{} + dup.AssertNotCalled(t, "Do") +} +` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "scoping.go", src, 0) + if err != nil { + t.Fatalf("parse: %v", err) + } + + got := map[string]string{} + for _, site := range collectAssertSites(fset, f) { + typ, ok := resolveRecvType(site.scopes, site) + if !ok { + typ = "" + } + got[fmt.Sprintf("%s@%d", site.recv, site.pos.Line)] = typ + } + + want := map[string]string{ + "mockStore@5": "MockAlpha", // TestA's own binding, not TestB's + "mockStore@10": "MockBeta", // TestB's own binding, not TestA's + "ses@16": "MockGamma", // closure resolves through the enclosing scope + "dup@23": "", // one scope, two types: refuse to guess + } + for k, w := range want { + if got[k] != w { + t.Errorf("%s resolved to %q, want %q", k, got[k], w) + } + } +} diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index 5ed7422e5..685f59579 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -1531,11 +1531,13 @@ func TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly(t *testing.T) { }, } - // DescribeSavingsPlansOfferings must NOT be called when CE supplies the ID. - mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) - id, err := client.findOfferingID(context.Background(), rec, "test-exec") require.NoError(t, err) assert.Equal(t, "ce-provided-offering-id-abc123", id, "CE-provided OfferingID must be used directly, skipping DescribeSavingsPlansOfferings") + + // DescribeSavingsPlansOfferings must NOT be called when CE supplies the ID. + // Asserted after the call: before it, the mock has recorded nothing and this + // passes for any implementation, whatever the matcher count. + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings", mock.Anything, mock.Anything) } From 14dfddfff59efa7d144f54c7feb07941ee541f81 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 08:27:11 +0200 Subject: [PATCH 4/4] 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) --- internal/mocks/assertions_test.go | 8 +- .../mocks/vacuous_assertion_guard_test.go | 284 +++++++++++------- 2 files changed, 175 insertions(+), 117 deletions(-) diff --git a/internal/mocks/assertions_test.go b/internal/mocks/assertions_test.go index 8b158a016..b257ab9e4 100644 --- a/internal/mocks/assertions_test.go +++ b/internal/mocks/assertions_test.go @@ -78,7 +78,13 @@ func TestAssertNotCalled_NameOnlyFormMatchesCallWithArguments(t *testing.T) { // testify's promoted implementation is the behavior being replaced: it // still passes, because WithTx never reached mock.Called and because an // empty expectation cannot match a two-argument call. - assert.True(t, m.Mock.AssertNotCalled(&recordingT{}, "WithTx")) + // + // Hoisted to a local rather than written as m.Mock.AssertNotCalled(...): + // TestNoUnfailableMockAssertions cannot resolve a selector receiver and + // correctly reports such a site for hand review, which this deliberate + // call into the broken implementation would otherwise trip forever. + promoted := &m.Mock + assert.True(t, promoted.AssertNotCalled(&recordingT{}, "WithTx")) } // TestAssertNotCalled_WrongMatcherCountFailsLoudly pins the third way these diff --git a/internal/mocks/vacuous_assertion_guard_test.go b/internal/mocks/vacuous_assertion_guard_test.go index 7bc46a6f1..02f50537e 100644 --- a/internal/mocks/vacuous_assertion_guard_test.go +++ b/internal/mocks/vacuous_assertion_guard_test.go @@ -35,6 +35,9 @@ import ( // still cannot place is reported rather than skipped: a skip that says nothing // is the same shape of defect the guard exists to catch. It parses rather than // builds, so it costs milliseconds. +// +// Receiver resolution walks enclosing function bodies innermost-outward, which +// approximates Go's lexical scoping rather than implementing it. // mockType is one testify mock in one package. type mockType struct { @@ -136,65 +139,15 @@ func TestNoUnfailableMockAssertions(t *testing.T) { } for _, f := range files { for _, site := range collectAssertSites(fset, f) { - recvType, resolved := resolveRecvType(site.scopes, site) - m, isMock := mocks[recvType] - if resolved && !isMock { - var reason string - m, isMock, reason = resolveCrossPackage(global, recvType, site.method) - if !isMock { - skipped = append(skipped, fmt.Sprintf( - "%s: %s.%s(t, %q) -- receiver resolves to %s, which this check "+ - "could not analyze (%s). Review by hand.", - site.pos, site.recv, site.assertFn, site.method, recvType, reason)) - continue - } - } - if !resolved { - // The receiver was not assigned from a mock literal in this - // file. Only report it when the method name belongs to some - // mock in the package and would be unfailable if it were one, - // so an unreviewed blind spot cannot hide behind a pass. - if unresolvedLooksRisky(site, mocks, global) { - skipped = append(skipped, fmt.Sprintf( - "%s: %s.%s(t, %q) -- receiver type could not be resolved; "+ - "review this site by hand", site.pos, site.recv, site.assertFn, site.method)) - } - continue - } - if m.shadowed[site.assertFn] { - // This type implements the helper being used, so it defines - // what the matchers mean. See MockConfigStore in assertions.go. - continue - } - arity, known := m.calledArity[site.method] - if !known { - // The method exists on a mock but never calls m.Called itself - // (it delegates to a sibling), so there is no arity to check - // against. Report it: "checked nothing" must never be silent, - // which is the same reason checked == 0 is fatal below. - skipped = append(skipped, fmt.Sprintf( - "%s: %s.%s(t, %q) -- %s does not call m.Called directly, so its "+ - "argument count cannot be derived. Review by hand.", - site.pos, site.recv, site.assertFn, site.method, site.method)) - continue - } - checked++ - if arity == 0 { - continue - } - switch { - case site.matchers == 0: - findings = append(findings, fmt.Sprintf( - "%s: %s.%s(t, %q) passes no matchers, but %s hands %d argument(s) to m.Called. "+ - "testify diffs the empty matcher list against those arguments and counts each as a "+ - "difference, so this assertion can never fail. Pass %d matcher(s) (mock.Anything is fine).", - site.pos, site.recv, site.assertFn, site.method, site.method, arity, arity)) - case site.matchers != arity: - findings = append(findings, fmt.Sprintf( - "%s: %s.%s(t, %q) passes %d matcher(s), but %s hands %d argument(s) to m.Called. "+ - "A count that differs from the real arity can never match, so this assertion can "+ - "never fail. Pass %d matcher(s).", - site.pos, site.recv, site.assertFn, site.method, site.matchers, site.method, arity, arity)) + verdict, msg := classifySite(site, mocks, global) + switch verdict { + case verdictSkipped: + skipped = append(skipped, msg) + case verdictFinding: + checked++ + findings = append(findings, msg) + case verdictChecked: + checked++ } } } @@ -317,6 +270,9 @@ func collectAssertSites(fset *token.FileSet, f *ast.File) []assertSite { site.recv = recv.Name if len(v.Args) < 2 { + // Defensive only: testify's signature requires a method name, so + // this shape cannot appear in code that compiles. It is exercised + // from the parse-only synthetic fixture in the self-test. site.unsupported = "fewer than two arguments; not a well-formed mock assertion" out = append(out, site) return true @@ -355,6 +311,91 @@ func collectAssertSites(fset *token.FileSet, f *ast.File) []assertSite { return out } +// siteVerdict is what the guard has to say about a single assertion site. +type siteVerdict int + +const ( + verdictIgnore siteVerdict = iota // nothing to say: not a mock, shadowed, or zero-arity + verdictChecked // analyzed and sound + verdictFinding // an assertion that cannot fail + verdictSkipped // could not be analyzed, and must be reported as such +) + +// classifySite is the ONE dispatch for an assertion site. The repo-wide run and +// the self-test both call it, so the self-test cannot pass against a code path +// that exists only inside the test. +// +// That is not hypothetical: the unsupported-shape reporting below was once +// written in five places and read in none that actually ran, because the +// self-test re-implemented this dispatch with the branch the real loop lacked. +// A test asserting on its own private copy of the logic is the same defect this +// whole guard exists to catch, one level up. +func classifySite(site assertSite, mocks map[string]*mockType, global map[string][]*mockType) (siteVerdict, string) { + if site.unsupported != "" { + return verdictSkipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- %s. Review by hand.", + site.pos, site.recv, site.assertFn, site.method, site.unsupported) + } + + recvType, resolved := resolveRecvType(site.scopes, site) + m, isMock := mocks[recvType] + if resolved && !isMock { + var reason string + m, isMock, reason = resolveCrossPackage(global, recvType, site.method) + if !isMock { + return verdictSkipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- receiver resolves to %s, which this check "+ + "could not analyze (%s). Review by hand.", + site.pos, site.recv, site.assertFn, site.method, recvType, reason) + } + } + if !resolved { + // Only report an unresolved receiver when the method name belongs to + // some mock and would be unfailable if it were one, so an unreviewed + // blind spot cannot hide behind a pass without drowning the report. + if unresolvedLooksRisky(site, mocks, global) { + return verdictSkipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- receiver type could not be resolved; "+ + "review this site by hand", site.pos, site.recv, site.assertFn, site.method) + } + return verdictIgnore, "" + } + if m.shadowed[site.assertFn] { + // This type implements the helper being used, so it defines what the + // matchers mean. See MockConfigStore in assertions.go. + return verdictIgnore, "" + } + arity, known := m.calledArity[site.method] + if !known { + // The method exists on a mock but never calls m.Called itself (it + // delegates to a sibling), so there is no arity to check against. + // "Checked nothing" must never be silent -- the same reason checked == 0 + // is fatal. + return verdictSkipped, fmt.Sprintf( + "%s: %s.%s(t, %q) -- %s does not call m.Called directly, so its "+ + "argument count cannot be derived. Review by hand.", + site.pos, site.recv, site.assertFn, site.method, site.method) + } + if arity == 0 { + return verdictChecked, "" + } + switch { + case site.matchers == 0: + return verdictFinding, fmt.Sprintf( + "%s: %s.%s(t, %q) passes no matchers, but %s hands %d argument(s) to m.Called. "+ + "testify diffs the empty matcher list against those arguments and counts each as a "+ + "difference, so this assertion can never fail. Pass %d matcher(s) (mock.Anything is fine).", + site.pos, site.recv, site.assertFn, site.method, site.method, arity, arity) + case site.matchers != arity: + return verdictFinding, fmt.Sprintf( + "%s: %s.%s(t, %q) passes %d matcher(s), but %s hands %d argument(s) to m.Called. "+ + "A count that differs from the real arity can never match, so this assertion can "+ + "never fail. Pass %d matcher(s).", + site.pos, site.recv, site.assertFn, site.method, site.matchers, site.method, arity, arity) + } + return verdictChecked, "" +} + // exprString renders a receiver expression for a report message. func exprString(e ast.Expr) string { switch v := e.(type) { @@ -371,7 +412,11 @@ func exprString(e ast.Expr) string { } // resolveRecvType maps the assertion's receiver identifier to the type it was -// assigned from, searching ONLY the innermost function body containing the site. +// assigned from, searching the enclosing function bodies from the innermost +// outward. This approximates Go's lexical scoping closely enough for the shapes +// mock assertions are written in; it is not a full implementation of it. Only +// function bodies are walked, so a package-level mock var is not resolved (see +// the limitation noted in the PR description). // A file-wide search would keep the last matching assignment, so two tests in // one file that both name a variable mockStore would resolve every site to // whichever type happened to appear last -- comparing matcher counts against the @@ -384,13 +429,18 @@ func exprString(e ast.Expr) string { // check degrades in everywhere else. func resolveRecvType(scopes []ast.Node, site assertSite) (string, bool) { for _, scope := range scopes { - name, ok, conflict := bindingInScope(scope, site.recv) - if conflict { + name, binds, conflict := bindingInScope(scope, site.recv) + if !binds { + continue // not bound here; look outward + } + // The innermost scope that binds the name wins, exactly as it shadows + // an outer binding at run time. If its type cannot be determined, or it + // binds two types, report unresolved rather than adopting an outer type + // the code would never actually use. + if conflict || name == "" { return "", false } - if ok { - return name, true - } + return name, true } return "", false } @@ -398,9 +448,15 @@ func resolveRecvType(scopes []ast.Node, site assertSite) (string, bool) { // bindingInScope looks for assignments of name directly within one function // body, without descending into nested function literals (those are their own // scopes and are searched separately). It reports the bound mock type, whether -// one was found, and whether the scope binds the name to two different types. +// the scope binds the name AT ALL, and whether it binds it to two different +// types. +// +// "Binds it at all" is deliberately separate from "we could type it": a scope +// that does `mockStore := newStore()` shadows any outer mockStore, so the walk +// must stop there rather than continue outward and adopt an unrelated type. func bindingInScope(scope ast.Node, recv string) (string, bool, bool) { var found string + binds := false conflict := false ast.Inspect(scope, func(n ast.Node) bool { if fl, ok := n.(*ast.FuncLit); ok && fl.Body != scope { @@ -426,6 +482,7 @@ func bindingInScope(scope ast.Node, recv string) (string, bool, bool) { if !ok || id.Name != recv { continue } + binds = true name := mockTypeOfExpr(rhs[i]) if name == "" { continue @@ -437,7 +494,7 @@ func bindingInScope(scope ast.Node, recv string) (string, bool, bool) { } return true }) - return found, found != "", conflict + return found, binds, conflict } // mockTypeOfExpr extracts T from &T{...}, T{...} and new(T). @@ -585,6 +642,7 @@ func Test(t *testing.T) { h.mockThing.AssertNotCalled(t, "Two") // want: unsupported, receiver is a selector thing.AssertNotCalled(t, methodName) // want: unsupported, method not a literal + thing.AssertNotCalled(t) // want: unsupported, fewer than two arguments // A second test in the same file binding the same name to another type: the // site above must not resolve to this one. @@ -623,64 +681,58 @@ func Test(t *testing.T) { "or its name-only AssertCalled sites would be exempted while still unfailable") } - type verdict struct { - method string - matchers int - vacuous bool - } - // The synthetic source is a single package, so the repo-wide index used by - // the real run is just this package's mocks. + // The synthetic source is a single package, so the repo-wide index the real + // run consults is just this package's mocks. global := map[string][]*mockType{} for name, m := range mocks { global[name] = append(global[name], m) } - var got []verdict - unresolved, unsupported := 0, 0 + // Drive the REAL dispatch, not a copy of it. An earlier revision of this + // test re-implemented the main loop's branching, so it asserted on an + // unsupported-shape branch the production path did not have. + counts := map[siteVerdict]int{} + var findings, skips []string for _, site := range collectAssertSites(fset, f) { - if site.unsupported != "" { - unsupported++ - continue - } - recvType, resolved := resolveRecvType(site.scopes, site) - m, isMock := mocks[recvType] - if resolved && !isMock { - continue - } - if !resolved { - if unresolvedLooksRisky(site, mocks, global) { - unresolved++ + v, msg := classifySite(site, mocks, global) + counts[v]++ + switch v { + case verdictFinding: + findings = append(findings, msg) + case verdictSkipped: + skips = append(skips, msg) + } + } + + // Three unfailable assertions: name-only Two, wrong-count Two, name-only + // AssertCalled on Two. The correct-count and zero-arity sites are sound. + if counts[verdictFinding] != 3 { + t.Errorf("findings = %d, want 3:\n%s", counts[verdictFinding], strings.Join(findings, "\n")) + } + + // Every shape the check cannot analyze must be REPORTED, never dropped: + // four unsupported syntactic shapes plus a receiver typed to something that + // is not a mock and a receiver that cannot be resolved at all. + if counts[verdictSkipped] != 6 { + t.Errorf("skipped = %d, want 6:\n%s", counts[verdictSkipped], strings.Join(skips, "\n")) + } + for _, want := range []string{ + "variadic spread", + "not a plain identifier", + "not a string literal", + "fewer than two arguments", + "no mock of that name is declared", + "could not be resolved", + } { + found := false + for _, got := range skips { + if strings.Contains(got, want) { + found = true } - continue } - arity, known := m.calledArity[site.method] - if m.shadowed[site.assertFn] || !known || arity == 0 { - continue + if !found { + t.Errorf("no skipped entry mentions %q; got:\n%s", want, strings.Join(skips, "\n")) } - got = append(got, verdict{site.method, site.matchers, site.matchers != arity}) - } - // Three shapes the check cannot analyze must be REPORTED, not dropped: - // a variadic spread, a selector receiver, and a non-literal method name. - if unsupported != 3 { - t.Errorf("unsupported shapes reported = %d, want 3 (spread, selector receiver, non-literal method)", unsupported) - } - - want := []verdict{ - {"Two", 0, true}, // name-only - {"Two", 1, true}, // wrong count - {"Two", 2, false}, // correct - {"Two", 0, true}, // AssertCalled, name-only - } - if len(got) != len(want) { - t.Fatalf("checked %d site(s), want %d: %+v", len(got), len(want), got) - } - for i := range want { - if got[i] != want[i] { - t.Errorf("site %d = %+v, want %+v", i, got[i], want[i]) - } - } - if unresolved != 1 { - t.Errorf("unresolved risky sites = %d, want 1 (the `unknown` receiver)", unresolved) } }