Repository navigation
fix(test): make 78 unfailable MockConfigStore assertions able to fail - #1735
Conversation
MockConfigStore serves many methods from a hardcoded default without reaching mock.Called: the isExpected short-circuit when no expectation is registered, and the Fn-override fields. testify appends to mock.Mock.Calls from inside mock.Called, so those invocations never landed there, and any assertion reading that slice was structurally incapable of failing. AssertNotCalled walked a slice the call could not have reached and reported success no matter what the code under test did. A second, independent cause compounds it: testify diffs an empty expectation against the real arguments and counts each one as a difference, so the name-only form AssertNotCalled(t, "WithTx") never matches a method that takes parameters. 18 of the affected sites were unfailable both ways, so recording alone would not have made them bite. Every MockConfigStore method now records its own invocation in a callLog before branching, and AssertCalled / AssertNotCalled / AssertNumberOfCalls shadow the promoted testify methods to read that log. Naming a method with no arguments now means "not called at all", which is the only thing that form can usefully assert. Argument matchers, mock.Anything, registered expectations and AssertExpectations are unchanged. This repairs 77 assertion sites across internal/api, internal/purchase and internal/scheduler that could not fail: 46 blocked by the short-circuit, 31 by the empty-expectation diff. Closes #1595
…t fixture Two findings from mutation-testing the 77 repaired assertion sites. t.Helper() marks its own caller, so routing it through a shared markHelper function marked markHelper as the helper and reported every failure at assertions.go rather than at the failing assertion. With 25 assertion sites in one test file, the output could not say which one fired. The type-assertion is now inlined in each of the three shadowed methods. TestProcessMessage_ApproveRejectsTokenMismatch asserts GetCloudAccount is not called, guarding that token validation precedes approver resolution. Its fixture carries no recommendation with a CloudAccountID, so gatherApproverContactEmails iterates an empty slice and the guarded bug could not produce the call: the assertion was mechanically failable but semantically inert. The fixture now mirrors its sibling at TestProcessMessage_ApproveRejectsNonMatchingActor. Hoisting approver resolution above the token check in messages.go now fails the test; without the fixture change it stayed green. Refs #1595
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
Adversarial review, head
|
| gate | result |
|---|---|
go build ./... |
clean |
go vet ./... |
clean |
go test -race -count=1 ./... |
exit 0, 32 packages ok, 0 FAIL |
gocyclo -over 10 -ignore "_test\.go" . |
clean |
golangci-lint run --timeout=10m at the CI-pinned v2.10.1 (installed fresh; the local 2.11.4 is not authoritative) |
0 issues. |
Diff is 417 insertions, 0 deletions across 4 files. No assertion was deleted, weakened, or relocated to make it honest. Proportionality is fine: no new mocking framework, no test-suite rewrite.
F1 (blocking, execution-verified) internal/purchase/scheduled_fire_test.go:52 is still unfailable
This is a MockConfigStore AssertNotCalled site inside the class the PR claims to have completed, and it is not repaired.
store.AssertNotCalled(t, "TransitionExecutionStatus",
mock.Anything, mock.Anything, mock.Anything, mock.Anything) // 4 matchersinternal/mocks/stores.go:246 records five arguments:
m.record("TransitionExecutionStatus", ctx, executionID, fromStatuses, toStatus, actor)matching() special-cases only len(expected) == 0. With a non-zero but wrong matcher count it falls through to mock.Arguments(expected).Diff(c.args), and testify's Diff sets maxArgCount = max(len(expected), len(actual)) and scores position 5 as (Missing). diffs >= 1 always, so the assertion can never match a recorded call. This is a third structural vacuity mode, distinct from the two the PR describes, and the PR's fix does not cover it.
Verified by mutation, not by reading: with FireScheduledDelayedPurchases mutated to issue a CAS on the nothing-is-due branch (the exact bug the assertion states it guards, "No CAS attempted when nothing is due"), the call demonstrably happens and dispatches through the mock, and the test still passes:
=== RUN TestFireScheduledDelayedPurchases_NoDueRows
MUTATION: about to call TransitionExecutionStatus with nothing due
MUTATION: TransitionExecutionStatus returned
--- PASS: TestFireScheduledDelayedPurchases_NoDueRows (0.00s)
Correcting only the assertion (either 5 matchers, or the name-only form) makes it bite, with the mutation unchanged:
scheduled_fire_test.go: mock: TransitionExecutionStatus was called 1 time(s) matching [], expected none. Recorded calls:
TransitionExecutionStatus([context.Background mutation-injected-exec [scheduled] approved <nil>])
--- FAIL
One-line fix. Consequence for the PR's headline claim: this site was unfailable before the PR and is unfailable after it, so the true in-scope count is at least 78, not 77, and 77/77 FAILS-AS-EXPECTED does not hold for it. Whatever enumeration produced 77 did not model the arity mode.
Suggested hardening beyond the one-line fix, since the class is now known: have matching() treat a non-zero matcher count that differs from the recorded arity as a hard error rather than a silent non-match. A supplied-vs-recorded arity mismatch is never a meaningful assertion; failing loudly there closes the mode permanently instead of relying on nobody miscounting again.
Sweep for siblings: exactly one
Two independent methods agree that scheduled_fire_test.go:52 is the complete set:
- Static. Method-to-arity map built from all 109
m.record(...)calls, resolved against all 197MockConfigStoreAssertCalled/AssertNotCalledsites (including thetype MockConfigStore = mocks.MockConfigStorealiases ininternal/api/mocks_test.go:19andinternal/purchase/mocks_test.go:18). The (supplied, recorded) distribution is entirely on the diagonal,(0,*)x51,(1,1)x14,(2,2)x66,(3,3)x19,(4,4)x9,(5,5)x37, with one off-diagonal cell. - Runtime. An arity probe wired into the shadowed helpers, run across the whole
./internal/...suite, emitted exactly one distinct hit, the same line, with zero unresolved records.
What holds up (execution-verified)
Cause (b) is real and correctly diagnosed. Confirmed against the testify v1.11.1 source in go.mod (mock.go:955-985): with an empty args, maxArgCount comes from the actual arguments and each position becomes (Missing), counting as a difference. A name-only AssertNotCalled(t, "WithTx") therefore cannot match any method taking parameters. Reproduced by execution.
All 109 *MockConfigStore methods record before branching. Zero methods missing an m.record(...). The instrumentation itself is complete.
Mutation spot-checks: 4 sites across 3 shapes, all bite, all correctly attributed. For each: mutate production code so the forbidden call happens, confirm the failure comes from the shadowed helper (not a testify panic or an unrelated assertion), then re-run with the three shadowed methods renamed so testify's promoted versions take over, confirming the site was genuinely unfailable pre-fix.
| site | class | mutated behaviour | result |
|---|---|---|---|
handler_purchases_test.go:2622 WithTx |
both causes | deny path opens a tx before the 403 (#1516 shape) | fails; control passes |
handler_purchases_test.go:2623 SavePurchaseExecution |
name-only + Fn escape |
deny path stamps the execution row before the 403 | fails; control passes |
handler_inventory_test.go:177 GetAllPurchaseHistory |
cause (b) | handler falls back to the unscoped read (#701/#498/#866 shape) | fails; control passes |
coverage_extra_test.go:651 GetCloudAccount |
repaired fixture | approver resolution hoisted above the token check | fails; control passes |
t.Helper() attribution is correct. Every failure is attributed to the assertion's own line in the test file, never to assertions.go. Confirmed independently at four different call sites. The inlining fix in the second commit does what it claims.
The coverage_extra_test.go:643 fixture repair is load-bearing, not cosmetic. The decisive evidence is the pair against the same mutation: with the new Recommendations: [{CloudAccountID: &accountID}] fixture the test goes red; with only the fixture reverted it goes green, because gatherApproverContactEmails iterates a nil slice and never reaches GetCloudAccount. The two commits are independently necessary, one fixes visibility and the other reachability.
The shadowing does not break assertions that already worked. AssertNumberOfCalls reads the same log and does not double-count:
handler_config_test.go:1089(SaveServiceConfig, a method that both records and reachesm.Called, so it is the direct no-double-count proof): one extra call givesexpected 2 call(s) ... got 3. Exactly +1.handler_history_test.go:1969and:2031(TransitionExecutionStatus): firing twice givesexpected 1 ... got 2. Exactly +1, not +2.- Registered expectations, argument matchers,
.Maybe()andAssertExpectationsare untouched; they still readmock.Mock.Calls.internal/api2041 passed,internal/purchase+internal/mocks+internal/scheduler432 passed.
Also checked: there are zero name-only AssertCalled sites in the repo, so the loosened name-only semantics cannot silently convert a previously-failing AssertCalled into a passing one. The two structs that embed MockConfigStore (mockSuppressionStore, mockOverrideStore in internal/scheduler) override methods that never reached m.Called either, so their assertion behaviour is unchanged.
F2 (non-blocking, scope) The PR does not state that the fix is MockConfigStore-only
Cause (b) is a testify-wide defect, not a MockConfigStore one. Every name-only AssertNotCalled on any testify mock in this repo is vacuous, and the shadowing here only covers *MockConfigStore.
Repo-wide there are 82 name-only sites, of which roughly 30 sit on other mock types and remain unfailable after this PR, for example:
internal/api/handler_auth_test.go:968mockAuth.AssertNotCalled(t, "UpdateUserProfile")internal/api/handler_purchases_test.go:156,207,331,681,744,745mockPurchase(MockPurchaseManager),ApproveExecution/ApproveAndExecuteinternal/purchase/execution_test.go:663,729,1822mockFactory(MockProviderFactory),CreateAndValidateProviderinternal/email/mute_test.go:60,149ses(MockSESClient),SendEmailinternal/api/handler_analytics_test.go:204,259,414,639,662,internal/api/handler_ri_exchange_test.go:971,providers/aws/services/savingsplans/client_test.go:390,976,1032,1535,cmd/multi_service_test.go:1287,1349,1424
Several of these guard authorization and money paths of the same shape the PR is fixing. This is legitimately out of scope for #1595, but the PR body presents "The real count: 77, not 20" without saying the count is scoped to one mock type, which reads as completeness for the whole defect class. Worth a scope sentence in the body and a follow-up issue.
F3 (non-blocking, accuracy) The severity framing does not hold uniformly across the 77
The premise is "an assertion that cannot fail is a place where a real defect ships green". That is exactly right for the short-circuit and Fn-override classes, where the mock serves a default and the run stays green.
It is not right for the pure cause-(b) class. For methods that reach m.Called unconditionally (TransitionExecutionStatus, GetAllPurchaseHistory, SaveServiceConfig, ...), a forbidden call with no registered expectation makes testify panic: mock: I don't know what to return because the method call was unexpected, aborting the test before the assertion runs. Reaching the assertion at those sites required arming an inert .On(...) in the fixture. So for that subset the pre-fix state was a loud, if misattributed, failure rather than a silent pass, and the PR's real gain is a legible correctly-attributed assertion instead of an incidental panic. That is still worth having; it just is not the same risk. Since the 31 "empty-expectation diff only" sites are the largest single bucket in the table, the distinction is material to how the 77 should be read.
Related minor inaccuracy: the body attributes the SavePurchaseExecution sites to the isExpected short-circuit, but stores.go:235-242 has no isExpected on that method, only the SavePurchaseExecutionFn override. The count is unaffected; the 28/18/31 split labels are imprecise.
F4 (non-blocking, latent) The call log makes mock-internal delegation observable
record() fires once per method entry and outer/inner names differ, so nothing is ever double-counted. But four methods fall through to a sibling, and the fallback now shows up in the log where testify structurally could not see it:
| delegation | new callLog |
testify mock.Mock.Calls |
|---|---|---|
SavePurchaseExecutionTx -> SavePurchaseExecution (Fn set) |
Tx=1, inner=1 | (neither) |
UpdatePurchasePlanTx -> UpdatePurchasePlan |
Tx=1, inner=1 | [UpdatePurchasePlan] |
GetPendingExecutionsTx -> GetPendingExecutions |
Tx=1, inner=1 | [GetPendingExecutions] |
UpdateGlobalConfigAtomic -> GetGlobalConfig + SaveGlobalConfig |
Atomic=1, Get=1, Save=1 | [SaveGlobalConfig] |
Divergence is monotone and only ever 0 -> 1. AssertNotCalled(t, "SavePurchaseExecution") now means "neither the plain writer nor the Tx variant ran", which is strictly stronger for a "no write happened" guard, and a false positive only for a test whose intent is specifically "the Tx variant was used, not the plain one". Nothing trips it today (the Tx delegators have production callers only in internal/api, and those AssertNotCalled sites all sit on 403-before-store paths). A short comment at the four fallbacks in stores.go would stop a future author reading AssertNotCalled(inner) as "the non-Tx path was not used".
Declared out-of-scope items: both confirmed, correctly excluded
scheduler_test.go:972MarkCollectionStarted. Confirmed: the only production caller isinternal/api/handler_recommendations_refresh.go:77, not the scheduler's background-refresh path. The assertion bites mechanically but guards nothing about today's code, and the sentinel-disable behaviour (the feat(settings): configurable recommendations cache-staleness threshold + provider lookback window (closes #301) #308 regression) is genuinely unguarded. Correctly filed as fix(test): scheduler sentinel-disable guard asserts a method the refresh path never calls, so the #308 regression is unguarded cloud-commitments-platform#167 rather than patched here.GetPurchaseHistory. Confirmed: zero production callers repo-wide. Its twoAssertNotCalledsites are tripwires against re-introducing the legacy single-column API, not guards on current behaviour. Accurate as described.
Verdict
The core mechanism is sound, the diagnosis of both named causes is correct, the fix is proportionate and purely additive, attribution is right, and the spot-checked sites genuinely bite with pre-fix controls confirming they could not. The fixture repair is load-bearing. All gates pass at the CI-pinned versions.
One blocking item: F1. It is a one-line assertion fix, but it matters more than its size, because it is a site inside the claimed-complete set that the enumeration did not model, and the PR's central claim is completeness. I would also take the matching() arity guard so the mode cannot recur. F2 and F3 are corrections to the PR description plus a follow-up issue; F4 is a comment.
Reviewer notes: F1, the four mutation spot-checks, the pre-fix controls, the no-double-count checks, the arity sweep and all five gates are execution-verified. F2's site list is static-analysis-derived (receiver types resolved by declaration, not by full type-checking). F3's panic behaviour is execution-verified; its implication for the 31-site bucket is inferred. I did not independently reproduce the exact 28/18/31 partition, so I neither endorse nor dispute those three sub-counts; what I did establish is that the in-scope total is at least 78 and that one member of it is unrepaired.
A third structural cause of unfailable assertions, distinct from the two this PR already repaired and not covered by either fix. scheduled_fire_test.go asserted TransitionExecutionStatus was not called using four matchers. The method takes five arguments. matching() special -cased only len(expected)==0, so a non-zero but wrong count fell through to testify's Diff, which pads the shorter side with "(Missing)" and scores the padded position as a difference. diffs was then non-zero for every recorded call, the assertion matched nothing, and it could not fire whatever the code under test did. Mutating FireScheduledDelayedPurchases to issue a CAS on the nothing-is-due branch -- exactly what the assertion guards -- leaves AssertNotCalled reporting success. The assertion now names the method with no matchers, which is how this mock spells "never called at all, whatever the arguments", and bites on that mutation. matching() now treats a matcher count that differs from the recorded arity as a test bug and reports it through Errorf + FailNow, naming the method, its real arity and the count given. len(expected)==0 keeps its meaning. The check reads the arity off a recorded call, so it is inert when the method was never called -- the case where any matcher count yields the same correct verdict anyway. The helpers also surface it as a failed assertion, so a TestingT whose FailNow returns cannot turn a miscount back into a silent pass. An arity sweep over all 206 MockConfigStore assertion sites in the repo, comparing each non-zero matcher count against the arity the method records, finds this one site and no other. The full suite confirms it: 6692 pass with the check armed. Both new regression tests fail with the check disabled and pass with it armed. The mock-internal fallback delegations gain a note that the delegate records itself, so one Tx call appears in the log under both names. Refs #1595
cristim
changed the title
|
Merging without a CodeRabbit verdict, deliberately, with the reasoning recorded. CodeRabbit has posted nothing on this PR at any head; its quota is per-developer, adaptive, and shared across the open PRs. What stands in its place is a full independent adversarial review, a blocking finding from that review, the fix for it, and an independent sweep confirming the fix is complete. The review found a third structural vacuity mode the PR did not model — a non-zero but wrong matcher count. The completeness claim was re-derived independently. An AST tool with full type info built a method-to-arity map from the Three claims in the original body were overstated and are now corrected:
Two things the implementer caught in its own work, both worth recording: It declined to claim the in-situ mutation left the test passing. It does not — the test fails via a testify panic rather than via the assertion, because that method reaches And its own first regression test passed without the hardening enabled — vacuous in precisely the way this PR exists to fix. It extended the test double to capture the error message so the test now asserts the diagnosis rather than the outcome. It also found the old body's per-package split summed to 73 against a stated 77, could not re-derive it without repeating the original instrumentation run, and removed the figures rather than patching a broken sum. Scope is stated explicitly: #1595 covers Gates at CI-pinned versions: build, vet, |
… 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
…predicate The three existing carve-out tests call requirePermission directly. That proves adminCarvedOuts refuses the pair; it does not prove the endpoint refuses, and those are different claims. #1757 is the standing example of the gap in this same file: a test named "...MUST be required" passes while calling a predicate the real dispatch never consults. Adds coverage that exercises executeExchange itself, the handler behind POST /api/ri-exchange/execute, with a plain admin:* principal. mockStore is stubbed with no expectations so a regression reaches the store and panics rather than quietly returning 200, and the AssertNotCalled passes two matchers because SaveRIExchangeRecord hands two arguments to m.Called (a name-only form could never fail: #1595/#1740). The request body is the real ExchangeExecuteRequestBody shape. The first draft used invented field names, and the mutation run exposed it: with the carve-out removed the handler stopped at "ri_ids is required" rather than proceeding, so the no-state-change assertion was satisfied by body validation instead of by the guard under test -- the semantically-inert fixture shape from #1735. With a valid body the mutation now carries execution past the gate and into the handler proper, so the assertion discriminates. Refs #1644
…predicate The three existing carve-out tests call requirePermission directly. That proves adminCarvedOuts refuses the pair; it does not prove the endpoint refuses, and those are different claims. #1757 is the standing example of the gap in this same file: a test named "...MUST be required" passes while calling a predicate the real dispatch never consults. Adds coverage that exercises executeExchange itself, the handler behind POST /api/ri-exchange/execute, with a plain admin:* principal. mockStore is stubbed with no expectations so a regression reaches the store and panics rather than quietly returning 200, and the AssertNotCalled passes two matchers because SaveRIExchangeRecord hands two arguments to m.Called (a name-only form could never fail: #1595/#1740). The request body is the real ExchangeExecuteRequestBody shape. The first draft used invented field names, and the mutation run exposed it: with the carve-out removed the handler stopped at "ri_ids is required" rather than proceeding, so the no-state-change assertion was satisfied by body validation instead of by the guard under test -- the semantically-inert fixture shape from #1735. With a valid body the mutation now carries execution past the gate and into the handler proper, so the assertion discriminates. Refs #1644
…ting group (#1758) * sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group execute:ri-exchange was absent from adminCarvedOuts, so permissionsAllow short-circuited on the admin:* wildcard and returned true unconditionally. Both routed execute endpoints, POST /api/ri-exchange/execute and POST /api/ri-exchange/azure-instances/exchange, were reachable by any admin with no explicit grant, and the provider/region/MaxPurchaseAmount dimensions were skipped along with the verb. An exchange consumes existing commitments and buys replacements with no rollback path, which is the same rationale that put execute:purchases behind separation of duties in #923. The carve-out alone would have been an outage rather than a partial fix. PR #1737's grant ceiling refuses to add a carved-out verb to any group through the API, and no migration seeded one, so the verb would have been grantable to nobody and both endpoints would 403 for every principal. execute:purchases survives its own carve-out only because 000059/000064 seed Purchaser and backfill admins into it. Migration 000096 does the same here: it seeds a system-managed RI Exchanger group at the next free namespace UUID and backfills every Administrators member, so no admin loses the capability on upgrade. The seeded grant is deliberately unconstrained, because a migration cannot know an operator's accounts, regions or spend ceiling and an over-narrow seed would refuse legitimate exchanges. Verified by execution against permissionsAllow: an unconstrained grant still short-circuits the constraint dimensions, so backfilled members bypass them exactly as admin:* does today. What the carve-out does buy is that the verb can no longer be granted through the API, that new principals need a deliberate grant instead of inheriting it from the wildcard, and that membership is revocable and auditable independently of the admin role. Constraint enforcement for the seeded population remains open on #1644. Coverage runs both directions, because a refusal-only test passes equally well against a handler that refuses everyone: a plain admin:* principal is refused, a holder of an explicit execute:ri-exchange grant is allowed, and a scope control pins that admin:* still grants the neighbouring non-carved verbs including view:ri-exchange. Mutation-verified per test, run alone: removing the pair from adminCarvedOuts fails the refusal test and leaves the other two passing, which is the correct dependency shape. The frontend ADMIN_CARVED_OUTS mirror is updated in the same change; drift there would offer an action in the UI that the backend then refuses. Closes #1644 * test(api): drive the routed execute handler, not just the permission predicate The three existing carve-out tests call requirePermission directly. That proves adminCarvedOuts refuses the pair; it does not prove the endpoint refuses, and those are different claims. #1757 is the standing example of the gap in this same file: a test named "...MUST be required" passes while calling a predicate the real dispatch never consults. Adds coverage that exercises executeExchange itself, the handler behind POST /api/ri-exchange/execute, with a plain admin:* principal. mockStore is stubbed with no expectations so a regression reaches the store and panics rather than quietly returning 200, and the AssertNotCalled passes two matchers because SaveRIExchangeRecord hands two arguments to m.Called (a name-only form could never fail: #1595/#1740). The request body is the real ExchangeExecuteRequestBody shape. The first draft used invented field names, and the mutation run exposed it: with the carve-out removed the handler stopped at "ri_ids is required" rather than proceeding, so the no-state-change assertion was satisfied by body validation instead of by the guard under test -- the semantically-inert fixture shape from #1735. With a valid body the mutation now carries execution past the gate and into the handler proper, so the assertion discriminates. Refs #1644 * fix(frontend): route each carved-out verb through the group that grants it back canAccess()'s loading-race fallback (effectivePermissions not yet loaded) hardcoded every carved-out verb to isPurchaser(), so it agreed with the backend for the three money-spending verbs but not for execute:ri-exchange (issue #1644): admin+Purchaser (not RI Exchanger) wrongly passed, and admin+RI-Exchanger (not Purchaser) wrongly failed. Adds isRIExchanger(), mirroring isPurchaser()'s shape, and routes both through a CARVE_OUT_FALLBACK_CHECK map keyed by carved-out verb instead of a single hardcoded predicate. Also fixes isPurchaser() itself, which iterated the full ADMIN_CARVED_OUTS set (now including execute:ri-exchange) instead of the three money-spending verbs it's actually meant to gate -- holding execute:ri-exchange alone would have wrongly satisfied the "can spend money" predicate the no-Purchaser first-run prompt keys off. Fixes the stale permissions.test.ts assertion that still expected execute:ri-exchange to pass admin:* during the fallback (pre-carve-out behaviour), and adds both-direction coverage for the fix plus regression tests pinning the two carve-outs' groups as disjoint. UX-only gate; the backend enforces on every request regardless of this fallback's answer. * test(migrations): make the 000096 idempotency subtest actually re-run the migration The "backfill is idempotent" subtest called migrations.RunMigrations a second time expecting it to re-exercise the DO block's guards. It does not: m.Up() returns migrate.ErrNoChange once the database is already at the latest version, so the migration body never runs again and the subtest passed regardless of whether the guards worked. Proved by mutation: stripping both idempotency devices from the up migration (the WHERE NOT guard and the DISTINCT dedup on the backfill UPDATE) still left the old test green. Rewrites the subtest to read 000096_seed_ri_exchanger_group.up.sql and execute its SQL directly a second time, the same pattern 000095_purchase_history_account_id_width_test.go already uses for its re-run assertion. Re-verified by the same mutation: with the fix, stripping the guards now fails the subtest (duplicate group_ids entry), and restoring them passes it again. The migration itself was already correctly idempotent; only the test coverage was empty. * test(api): assert the 403 identity, not error-string tokens, in the execute:ri-exchange handler test TestExecuteExchange_PlainAdminIsRefused discriminated a dropped carve-out by checking that the error message did NOT contain "execute"/"ri-exchange". Under mutation the error actually returned is an AWS credential/STS failure from exchange.ExecuteExchange (reached only once the carve-out stops refusing), whose wording has nothing to do with permissions -- the test caught the regression by accident, because that unrelated error string happened not to contain those two tokens. If the wording of that AWS error ever changed, the test would stop discriminating while staying green. Replaces the substring assertions with an identity check: the carve-out denial from requirePermission is a *clientError with code 403 (requireSessionPermission in handler.go), returned unwrapped by executeExchange, so asserting IsClientError + code 403 fails for the actual reason -- refused at the gate vs. failed downstream for something else. Verified by mutation, run in a hard-sandboxed AWS environment (nulled credentials/config files, disabled profile, IMDS disabled) so the mutated test cannot reach live AWS: with adminCarvedOuts stripped of the execute:ri-exchange pair, the new 403-identity assertion is the one that fails (0.00s, no AWS contact); restoring the pair passes again. Also corrects the docstring, which claimed the unstubbed mockStore backstop would panic if the carve-out regressed. It cannot: executeExchange's success path never calls SaveRIExchangeRecord at all (only the scheduled auto-exchange path in pkg/exchange/auto.go does), so AssertNotCalled holds unconditionally here and does not discriminate this test today. Kept as defense-in-depth for a future change that routes this handler through the store, documented as such. Adds t.Setenv("AWS_EC2_METADATA_DISABLED", "true") so the credential resolution failure this test depends on under mutation is fast and deterministic regardless of what AWS credentials happen to be configured in whichever environment re-runs it later, rather than depending on IMDS being unreachable by chance. executeExchange builds its AWS clients from ambient credentials with no injected seam (unlike internal/server's riExchangeClients) -- that structural gap is tracked separately as #1760; this is containment for the test, not a fix to the seam.
…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>
…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 #1595
Scope
This PR covers
MockConfigStoreonly. Cause (b) below is a property of testify itself, not of this mock: the repo has 27 further name-only assertion sites on other mock types (MockAuthService,MockPurchaseManager,MockAnalyticsClient,mockAzureExchangeOpsClientand others) that are still unfailable the same way. They are out of scope here and filed separately. "78, not 20" is a count forMockConfigStore, not a claim of class-wide completeness.The defect
MockConfigStoreserves many methods from a hardcoded default without reachingmock.Called: theisExpectedshort-circuit when no expectation is registered, and theFnoverride fields. testify appends tomock.Mock.Callsfrom insidemock.Called, so those invocations never landed there. Any assertion reading that slice was structurally incapable of failing:AssertNotCalledwalked a slice the call could not have reached.A second, independent cause compounds it. testify diffs an empty expectation against the real arguments and counts each one as a difference, so the name-only form
AssertNotCalled(t, "WithTx")never matches a method that takes parameters. 18 sites were unfailable both ways, so recording alone would not have made them bite.A third cause, found by adversarial review after the first two were repaired: a matcher count that is non-zero but does not equal the method's real arity.
Diffpads the shorter side with(Missing)and scores the padded position as a difference, sodiffsis unconditionally non-zero and the assertion matches nothing. The originalmatching()special-cased onlylen(expected) == 0, so a wrong count fell straight through.The real count: at least 78, not 20
The issue estimated 20. Causes (a) and (b) were enumerated empirically by instrumenting
AssertNotCalled/AssertNumberOfCallsto log every execution that could not fail, then running the whole suite:isExpectedshort-circuit onlySpread across
internal/api,internal/purchaseandinternal/scheduler; the cause-(c) site is ininternal/purchase. (The earlier revision of this section gave a per-package split of 49/17/7, which sums to 73 rather than 77; the per-package figures were not re-derived here, so they are omitted rather than restated.)The instrumentation that found (a) and (b) could not have found (c) — it keyed on the two known bypass shapes — so 78 is a floor, not a proven ceiling for causes nobody has thought of yet. The arity dimension specifically is now closed: a static sweep over all 206
MockConfigStoreassertion sites in the repo, comparing every non-zero matcher count against the arity the method records, finds exactly one mismatch (the site fixed here) and no other. The armed runtime check agrees — the full suite stays green.The fix
Every
MockConfigStoremethod records its own invocation in acallLogbefore branching, andAssertCalled/AssertNotCalled/AssertNumberOfCallsshadow the promoted testify methods to read that log. Naming a method with no arguments now means "not called at all", which is the only thing that form can usefully assert. Argument matchers,mock.Anything, registered expectations andAssertExpectationsare unchanged, and a call that reachesmock.Calledis counted once, not twice.For cause (c),
matching()treats a matcher count that differs from the recorded arity as a bug in the test and reports it viaErrorf+FailNow, naming the method, its real arity and the count given.len(expected) == 0keeps its meaning. The check reads the arity off a recorded call, so it is inert when the method was never called — the case where any matcher count yields the same correct verdict anyway.Proof that each repaired assertion bites
Every site was mutation-tested: the production code was changed so the path under test performs the action the assertion forbids, and the test had to fail with the message from the shadowed helper. All mutations were reverted.
What the mutations actually demonstrate differs by bucket, and the distinction matters:
mock.Called. Pre-fix, the forbidden call happened and nothing observed it: the test shipped green with the bug live. These are the sites where the fix converts a missed bug into a caught one.m.Calledunconditionally. Pre-fix, a forbidden call with no registered expectation hit testify's unknown-call path and panicked — the test failed, loudly but atstores.goand with a message about an unexpected call rather than about the invariant. The gain here is a legible failure attributed to the assertion that owns the invariant, not a bug that would otherwise have shipped. Describing these as "would have shipped green" overstates the fix, which is the same defect class this PR exists to correct.scheduled_fire_test.go:52) is a genuine false pass at the assertion level:AssertNotCalledreturned success with the forbidden call recorded. It sat onTransitionExecutionStatus, an unconditional-m.Calledmethod, so the test was still backstopped by the panic — but the assertion itself guarded nothing, and would have gone silent the moment that method gained anisExpectedguard orFnescape like its siblings.Mutations were the bug class each site exists to catch, for example:
WithTxand 9SavePurchaseExecutionsites in the permission matrixcheckDifferentApproverresolving the creator email before the tier-1 UUID compare - fails all 7GetUserEmailByIDsitesGetGlobalConfigread before theview:config/update:configgateDeleteSuppressionsByExecutionTxhandler_inventoryfalling back to the unscopedGetAllPurchaseHistory(the bug(api/history): handler ignores provider/account_ids/start/end query params from Purchase History + Purchases-page Global filters #701/ux(home): Account filter doesn't affect Home page Savings-over-time graph (data identical across accounts) #498/Main Header global filter not propagated to Inventory & Coverage subpages (QA 2.7+2.14, 2.8+2.15) #866 account-filter shape)FireScheduledDelayedPurchasesissuing a CAS on the nothing-is-due branch - failsscheduled_fire_test.go:52, which it did not beforeThree sites needed repair beyond the mock, all included here:
scheduled_fire_test.go:52AssertNotCalled(t, "TransitionExecutionStatus", ...)passed four matchers to a five-argument method (cause (c) above). Now named without matchers.coverage_extra_test.go:643AssertNotCalled(t, "GetCloudAccount")was mechanically failable but semantically inert: theexec-bad-tokenfixture carried no recommendation with aCloudAccountID, sogatherApproverContactEmailsiterated an empty slice and the guarded ordering bug could not produce the call. The fixture now mirrors its sibling; hoisting approver resolution above the token check fails the test, and did not before.t.Helper()marks its own caller, so routing it through a sharedmarkHelperreported every failure atassertions.goinstead of the failing assertion. Inlined into each shadowed method; failures now point at the test line.internal/mocks/assertions_test.gopins all three causes, including a test asserting that testify's promotedAssertNotCalledstill passes on the same call - the defect this replaces. The two cause-(c) regression tests were verified to fail with the arity check disabled and pass with it armed.Note on the escape hatches:
SavePurchaseExecutionis the one method served by anFnoverride without anisExpectedguard (stores.go:235-242); the other twelveFnfields sit on methods that also short-circuit viaisExpected.Gates
go build ./...,go vet ./...,go test -race -count=1 ./...(6692 passed, 43 packages),gocyclo -over 10 -ignore "_test\.go" .,golangci-lint runat the CI-pinned v2.10.1: all clean.Follow-ups filed separately, not fixed here
MockConfigStoremocks remain unfailable; this PR does not touch them.scheduler_test.go:972assertsMarkCollectionStartedis not called, but that method has no caller on the scheduler's background-refresh path (onlyhandler_recommendations_refresh.go:77). The faithful mutation - makingRecommendationsCacheStaleHours == 0no longer disable the refresh, the PR feat(settings): configurable recommendations cache-staleness threshold + provider lookback window (closes #301) #308 regression - leaves the test green. The sentinel-disable behavior is unguarded.GetPurchaseHistoryhas no production callers at all, so the twoAssertNotCalledsites naming it are tripwires for re-introducing the legacy single-column API rather than guards on current behavior.