TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p3Polish / idea / may never shipPolish / idea / may never ship

   severity/lowMinor harmMinor harm

   urgency/eventuallyNo deadlineNo deadline

   impact/internalTeam-internal onlyTeam-internal only

   effort/sHoursHours

   type/choreMaintenance / non-user-visibleMaintenance / non-user-visible

   on Jun 10, 2026
2. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

   Confirmed still present. The 2026-07-28 full review re-derived this independently and adds the whole-file count for the money-path file plus a sharper failure scenario. Suggest treating internal/purchase/money_path_regression_test.go as its own first-class item rather than one of nineteen, and raising the priority accordingly.

   Where

   • internal/purchase/money_path_regression_test.go - whole file: 29 .On(...) registrations, zero AssertExpectations. The .Once() expectations at the TestSQSRedeliveryDoesNotDoubleExecute and TestMultiAccountPartialSuccessIsAcked setups are the load-bearing ones.

   What

   This is the double-spend guard file for LeanerCloud/cloud-commitments-cli#1012 / LeanerCloud/cloud-commitments-cli#1013 / LeanerCloud/cloud-commitments-cli#1014. It registers ordered .Once() expectations - two GetExecutionByID, two TransitionExecutionStatus with different outcomes - that encode the exact call sequence the test claims to prove. Nothing ever verifies they were consumed.

   Failure scenario

   The tests currently survive on side-channel assertions: purchaseCalls == 1 and savedRoot.Status == "partially_completed". Suppose a refactor made handleExecutePurchase return early before the compare-and-swap, never calling TransitionExecutionStatus at all. The unconsumed .Once() expectations go unreported, and both side-channel assertions can still hold. On the money path, the absence of the atomic claim is precisely the double-spend bug: without the CAS, a redelivered SQS message is free to execute a second purchase.

   So the file that exists to prove "a redelivery cannot double-execute" cannot detect the removal of the mechanism that prevents it. That is a different severity from the general hygiene case: for the other eighteen files a missing AssertExpectations weakens coverage; here it removes the only proof of the invariant the file is named after.

   Suggest splitting this file out at priority/p2 / severity/medium (or handling it first within this issue) and leaving the remaining eighteen at P3, rather than letting the money-path file inherit the batch's "informational" rating.

   Fix direction

   As already recommended here, with the specific placement:

   t.Cleanup(func() {
       mockStore.AssertExpectations(t)
       mockServiceClient.AssertExpectations(t)
   })

   in each test in the file. Mandatory for any test whose proof is "this call happened exactly once".

   Verify the right way round: delete the CAS from handleExecutePurchase, confirm the tests now fail, then restore. If they still pass, the fix is not done.

   Related

   • Two separate findings from the same review touch this issue's surface and are filed as their own issues, since TEST-03: 19 Go test files register .On() expectations without mock.AssertExpectations, incl. money-path regression suite #35's fix would not resolve them:
     • internal/api/handler_coverage_test.go (listed here) also asserts nothing about response bodies, so adding AssertExpectations alone leaves a handler returning a structurally wrong response passing.
     • internal/auth/service_apikeys_test.go:513-521 and service_apikeys_api_test.go:415-423 do call AssertExpectations, but .Maybe() makes it vacuous, so they need the opposite change (dropping .Maybe() and synchronising on a channel).
3. cristim

   transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026
4. IAmJSD commented on Oct 22, 2026

   

   IAmJSD

   on Oct 22, 2026

   Collaborator

   More actions

   Claiming: working on this
5. IAmJSD

   self-assigned this

   on Oct 22, 2026
6. IAmJSD

   added 2 commits that reference this issue on Oct 25, 2026

   test(purchase,email): assert mock expectations in the money-path regr…

   ...

   1a4dd00

   test(purchase): assert mock expectations across the purchase tests (#…

   ...

   f50f1d3
7. IAmJSD commented on Oct 28, 2026

   

   IAmJSD

   on Oct 28, 2026

   Collaborator

   More actions

   Status update (2026-10-28): #1570 merged: internal/purchase tests now assert their mock expectations (a cleanup-based helper in the shared setups), with mutation probes showing dropped claims, history writes and emails now fail tests.

   Left: the same treatment for internal/api (the largest gap), internal/auth, internal/scheduler and internal/server.