Repository navigation
test(api,auth,scheduler,server,email): assert mock expectations in the remaining tests - #1594
Conversation
…e remaining tests Part B of LeanerCloud#35: internal/api test files after handler_config_test.go, plus internal/auth, internal/scheduler, internal/server and internal/email. Every test there that registers a required .On() expectation now verifies it, through t.Cleanup(AssertExpectations) at the mock's construction or in the helper that builds it (helpers used only by these files now take t). A runtime audit (testify instrumented to log every On, Maybe, call and AssertExpectations) found 868 tests whose required expectations were never checked; it now finds none, unit and integration alike. Turning the assertions on failed 93 tests on expectations that never fired. Most were stale stubs and are removed or made .Maybe(), each with the reason: an admin X-API-Key request never validates the bearer session, a session or CSRF check now runs before the row lookup (LeanerCloud#435, LeanerCloud#597), an RI exchange record with no region fails before reserving the spend ceiling, and so on. Where a refusal path should make no call, the call is left unregistered (an unexpected call fails the test) or pinned with AssertNotCalled. Six tests were passing for the wrong reason and now test what they name: - TestHandler_patchPlan_UpdateError failed plan validation, never reaching the UpdatePurchasePlan error it is about. - TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission was refused by CSRF; with a valid token it now pins the permission denial. - TestHandler_runPlannedPurchase_NilExecution was refused because admin:* does not grant execute; it now reaches RunPlannedPurchaseNow. - TestHandler_deleteUser_Error and TestHandler_deleteGroup_DeleteFailed failed UUID validation before the store error. - TestUpdateRIExchangeConfig_PermissionGate sent a body that no longer decodes; it now saves a valid one. No production code changes. Refs LeanerCloud#35 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 132 files, which is 32 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rQ4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rQ3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
Part B of #35. It covers the
internal/apitest files whose names sort afterhandler_config_test.go, plus all ofinternal/auth,internal/scheduler,internal/serverandinternal/email. Part A (theinternal/apifiles up to and includinghandler_config_test.go) is a separate branch. This PR does not edit any part A file or any non-test file.Refs #35
Problem
Many tests in these packages register testify expectations with
.On(...)but never callAssertExpectations. If the code under test stopped calling a collaborator, these tests would still pass. A file-level grep undercounts the problem, because many files assert some of their mocks and not others, and some build their mocks in helpers. So I measured it per test at runtime instead. I ran each package against a temporary copy of testify that logs everyOn,Maybe,Times, matched call andAssertExpectations. This instrumentation lives outside the repository and is not committed. The audit then lists every test that leaves a mock with a required (non-Maybe) expectation unasserted.Fix
Every test in scope that registers a required expectation now verifies it. In most cases this is
t.Cleanup(func() { m.AssertExpectations(t) })right where the mock is built. Where a helper builds the mock, the cleanup goes in the helper.tand register the cleanup themselves. Examples:adminHistoryReq,adminDashboardReq,adminInventoryReq,scopedAuthMock,buildSession{Retry,Cancel}Handler,cancelRouteHandler,adminSession,newRecProvider(scheduler) andnewCompensationAuth.setupDirectExecMocks,expectStoredRecs,newCreateConcurrencyFixture, and part A's ownsetupAdminAuthandsharedNameHandler). The cleanup goes at the part B call sites instead.historyScopeIDs(used byaccount_scope_shared_name_test.go) andnewTokenApprove(used byclaimed_row_check_test.go).newTokenApprovenow registers its session and global-config stubs inapprove(), because only that path reads them. These two helpers can't conflict textually with part A, but please re-check them on the combined tree.Turning the assertions on made 93 tests fail on expectations that never fired. I decided each case on its own:
Stale or over-broad stubs, removed or made
.Maybe(), each with a comment explaining why. Where a refusal path must make no call, the call is left unregistered (an unexpected call fails the test) or pinned withAssertNotCalled.X-API-Keyrequest never validates the bearer session (17handler_test.gotests, plusputPlanandpostPlannedPurchaseAt).RequiresCSRFtests and the oracle fixtures, which now also assertGetExecutionByIDis not called.ReserveApprovedRIExchange. The four-eyes RI fixture now takes the auth path and the outcome, so each case expects exactly its own calls.getHistoryresolves the approver email only for pending rows,getPublicInfonever consults the rate limiter, the scope check runs before the approve-any/retry-any grant, a plain reader never resolves the stale TTL (TestScheduler_ListRecommendationsnow assertsGetGlobalConfigis not called), and a few copy-pasted stubs ininternal/authandinternal/email.Tests that passed for the wrong reason. These now reach the path their names describe:
TestHandler_patchPlan_UpdateErrorfailed plan validation ("plan must have at least one service") and never reached theUpdatePurchasePlanerror it is named for. It now uses a valid plan and asserts the mapped 500.TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermissionwas refused by the CSRF check before the permission check ran. With a valid CSRF token it now asserts the permission denial, which is concealed as a missing execution (sec(api): authenticated callers can still tell a missing execution from an existing one on approve, cancel and revoke #597).TestHandler_runPlannedPurchase_NilExecutionwas refused becauseadmin:*does not grantexecute(test(auth): fail when an API adapter drops a request field #923). It now holds the purchaser verbs and reachesRunPlannedPurchaseNow.TestHandler_deleteUser_ErrorandTestHandler_deleteGroup_DeleteFailedused IDs that are not UUIDs, so they failed validation before reaching the store. They now assert that the store error comes through.TestUpdateRIExchangeConfig_PermissionGate("with update:config") sent a body that no longer decodes, so it passed with a 400. It now sends a valid body and saves.I found no production bug. Every missing call was a stale test stub, or a test whose setup stopped it from reaching the path it targets.
Per-package counts
The
.On(vsAssertExpectationsgrep metric from #35 counts in-scope files with.On(but noAssertExpectationstext:The 22 remaining files are not gaps. Each one holds only
.Maybe()stubs, or is a helper whose mocks are asserted by the code that builds them. Examples:mocks_test.gogrant helpers,newSelfScopeHarness,newPartialSweepScheduler,adminSession, and the integration fixtures, which are all.Maybe(). The runtime audit is the authoritative measure:The same audit over the in-scope
-tags integrationtests ininternal/apiandinternal/scheduleralso reports 0 after the change. After this PR, the same audit still finds 316 such tests in the part Ainternal/apifiles. Part A covers those, so #35 can close once both parts merge.Verification
This evidence comes from unit tests, mocks, and integration tests run on real Postgres in testcontainers. The repo's go-test hook and the race run were done per package on this branch only.
Mutation probes, fail-before/pass-after. Each one temporarily breaks production code so that it skips a collaborator call, runs the named test against the
origin/mainsnapshot and against this branch, then restores the file:getPublicInfostops callingCheckAdminExistsTestHandler_getPublicInfo/with_auth_service_and_no_adminpatchPlanskipsUpdatePurchasePlanTestHandler_patchPlan_UpdateErrorauthorizeSessionApproveallows every sessionTestHandler_approvePurchase_RejectsCreatorWithoutApprovePermissionauthServiceAdapter.Logoutskips the serviceTestAuthServiceAdapter_LogoutLoginskipsRecordSuccessfulLoginTestLogin_WithMFAisInSandboxskipsGetAccountTestSender_SendToEmail_ErrorCombined-tree verification. Tested on a local, never-pushed tree:
queue/platform@594ac3ec= origin/mainae6854b3plus:Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers.go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, one package at a time):internal/api,internal/auth,internal/config,internal/scheduler,internal/serverandinternal/testutilpass.scripts/test-git-secrets-allowlist.shpasses.Out of scope / notes
internal/apifiles are untouched, apart from the two shared helpers named above, which live in part B files.getPublicInfo(/api/info, AuthPublic) is not rate limited by the handler.Could a maintainer mirror #35's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/s.🤖 Generated with Claude Code