Repository navigation
Conversation
…ession suite money_path_regression_test.go registered ordered .Once() expectations (two TransitionExecutionStatus claims with different outcomes, two GetExecutionByID reads) that nothing ever verified were consumed. Its redelivery and partial-success guards survived only on side-channel counters, so a change that dropped the CAS claim while still running the purchase once went unnoticed. Register t.Cleanup(AssertExpectations) for every mock with required expectations in the file, and in the three email/coverage_test.go tests that lacked it. Two SendPurchaseConfirmation expectations never fired and are removed: processPurchaseRecommendations never sends the confirmation (its callers do), and executeForAccount returns AUDIT LOSS before notifying when the per-account row fails to save. Refs LeanerCloud#35 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First slice for #35: the money-path regression suite (which the maintainer's comment asked to treat first) plus
internal/email/coverage_test.go. Theinternal/apifiles are left for follow-up commits. They are listed below.Refs #35
Re-count on current main (c8bdd46)
The full list of 19 files came from a review doc that isn't in the repo. I rebuilt it by running the review's scan ("
.On(but noAssertExpectationsin the file") against the tree as of 2026-06-06, then addingmoney_path_regression_test.go. That gives exactly 19 files.AssertExpectationsat all. The other 8 now have some.money_path_regression_test.gogained someAssertExpectationscalls, butTestSQSRedeliveryDoesNotDoubleExecuteandTestMultiAccountPartialSuccessIsAckedstill never checked theirs. To count these gaps I used a scope-aware AST scan: a test function counts when it registers a non-.Maybe().On(...)on a local mock that is neverAssertExpectations'd in its scope. That scan found 161 such test functions across 17 of the 19 files.handler_accounts_router_test.goandapi/mocks_test.gohave none, because their.Oncalls are in helpers or are.Maybe(). After this PR, 152 remain, all ininternal/api.What changed
internal/purchase/money_path_regression_test.go: every test (and thecaptureIdempotencyTokenshelper) now registerst.Cleanup(func() { m.AssertExpectations(t) })for each mock with required expectations. This covers the ordered.Once()claim and read pairs inTestSQSRedeliveryDoesNotDoubleExecuteand the claim inTestMultiAccountPartialSuccessIsAcked/TestExecuteForAccount_CommittedButUnsavedIsAcked.internal/email/coverage_test.go: the same change in the 3 tests that lacked it.Expectations that never fired (test fixes, no production change)
captureIdempotencyTokens(used byTestRetryReusesIdempotencyTokenandTestLegacyRowFallsBackToExecutionID) registeredSendPurchaseConfirmation. The code under test,processPurchaseRecommendations, never sends it; its callers do. I removed the expectation, so any call now fails the test.TestExecuteForAccount_CommittedButUnsavedIsAckedregisteredSendPurchaseConfirmation.executeForAccountreturnsAUDIT LOSSbeforesendPurchaseNotificationwhen the per-account row fails to save, and the other account never commits, so no confirmation goes out on this run. I removed the expectation and added a comment explaining why.Verification
All of this is unit/mock evidence; nothing touches a real cloud. Tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause the local default AWS profile is invalid.Mutation probe (fail-before/pass-after). This is the maintainer's scenario. I patched
claimAndExecuteto drop theTransitionExecutionStatusPinnedCAS and use an in-memory dedupe instead, so the purchase still runs exactly once and thepurchaseCalls == 1/partially_completedside-channel assertions still hold:ok(the missing CAS goes undetected)FAIL: TransitionExecutionStatus(...),6 out of 8 expectation(s) were met(Redelivery) and6 out of 7(the other two)Simply deleting the CAS (so both deliveries execute) was already caught by the existing
purchaseCalls == 1assertion. The probe above covers the case that counter misses.Other checks:
go test -count=1 ./internal/purchase/ ./internal/email/ ./internal/mocks/: okgo test -short -race -count=1 ./internal/purchase/ ./internal/email/: okgo vet ./internal/purchase/ ./internal/email/: cleangofmt -l: clean--allow-parallel-runners) on both packages: 0 issues_test.gofiles changed, so not applicablescripts/setup-git-secrets.shin a throwaway repo with an isolated HOME and staged the two changed files plus a planted fake key. The planted key was flagged; with it removed the scan is clean.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Remaining for #35 (follow-up commits/PR)
Test functions still missing
AssertExpectations, by file:Some
internal/apiexpectations are registered inside helpers that take no*testing.T(setupRouterForDispatch,setupAdminAuth,setupStandardUserAuth,setupScopedAuth,adminAnalyticsReq,federationHandler). Asserting those means giving the helper at. Together they have about 140 call sites, some in files outside the 19 (handler_accounts_controlchar_test.go,handler_accounts_external_id_test.go,exchange_helpers_test.go). That should be its own commit.Out of scope (noted, not changed)
internal/mocks(likeTestNoUnfailableMockAssertions) would only be practical once those are cleared, so I didn't add one here.handler_coverage_test.goasserting nothing about response bodies, and.Maybe()makingAssertExpectationsvacuous ininternal/auth/service_apikeys*_test.go.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/s.🤖 Generated with Claude Code