Skip to content

chore(test): fix the 2026-07-28 review's mock, CI-doc, known-issues and e2e findings - #1270

Merged
IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-353
Oct 25, 2026
Merged

IAmJSD merged 5 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-353

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

This is the first slice of #353, the platform part of the 2026-07-28 test-integrity and docs-accuracy review. It fixes four findings in full or in part. The other two are either already fixed or deferred; the table below has the reasons.

Refs #353

Per-finding status (re-verified against main at a14dc8f)

Finding Status Notes
MEDIUM: frontend/src/__tests__/api.test.ts asserts inside a catch with no expect.assertions() Deferred (frontend) Still reproduces at lines 367-379. The UI owner owns frontend/, so this PR doesn't touch it. See "Frontend follow-up" below.
MEDIUM: MockConfigStore mixes two unregistered-method policies, so AssertNotCalled is vacuous for one group Already fixed (the failure scenario), remaining refactor deferred 4695793 ("make 78 unfailable MockConfigStore assertions able to fail") makes every method record itself in callLog before it branches on isExpected. MockConfigStore now shadows AssertCalled/AssertNotCalled/AssertNumberOfCalls to read that log, so AssertNotCalled fails for a default-served call too. TestAssertNotCalled_SeesDefaultServedCall in internal/mocks/assertions_test.go pins this. b5005b4 adds vacuous_assertion_guard_test.go for the rest of the class. The 2026-09-28 re-verification only checked that the two isExpected groups still exist. They do, but the split no longer changes what an assertion can catch. The other half of the fix direction (make defaults opt-in so a bare mock panics) would change the behavior of about 1,030 MockConfigStore constructions in 132 test files. That overlaps almost every in-flight test branch, so it's left out of this slice.
MEDIUM: internal/mocks type-assertion failures panic instead of failing the test Fixed here (mechanism and all but one site). Threading t through every call site is deferred Still reproduced on main: 83 sites (78 in stores.go, 3 in secretsmanager.go, 1 each in ses.go and sns.go). Details below.
MEDIUM: docs/ci-required-checks.md asks for a required check on a path-filtered workflow Fixed here Still reproduced: frontend-build.yml had paths: ["frontend/**", ".github/workflows/frontend-build.yml"].
LOW: known-issues.md names an ARM role the template doesn't define Fixed here Still reproduced: the doc said "CUDly Reservation and Savings Plan Purchaser" (on branch fix/731-arm-roles). arm/CUDly-CrossSubscription/template.json:41 says "CUDly Reservation Purchaser (custom)".
LOW: tests/e2e TestVersionEndpoint only asserts a non-empty body Fixed here Still reproduced: it decoded into map[string]any and checked len(body) != 0.
Cross-repo ci_cd_sanity_tests accountJSON panic Already fixed in cloud-commitments-go LeanerCloud/cloud-commitments-go#565 ("fail azure account helper via t.Fatalf instead of panic") is merged. This PR uses the same shape: t is threaded in and the helper fails with Errorf+FailNow, which is what t.Fatalf does.

In-flight overlap check. I ran git merge-tree of this branch against open PRs #1099, #1103, #1111, #1116, #1141, #1200, #1209, #1217, #1220, #1240, #1241, #1242 and #1245 (the open PRs that touch internal/mocks/stores.go are #1099, #1141, #1200, #1209, #1217, #1220, #1240 and #1242). I also checked the pushed fork branches fix/issue-47, -54, -64, -76, -153, -169, -243, -310, -326, -337 and -532. All merge cleanly. #1217 conflicted on the first pass because it deletes GetPendingExecutionsTx, so I left that one site as it was, which makes it merge cleanly too. None of them touch frontend-build.yml, docs/ci-required-checks.md, known-issues.md or tests/e2e/.

Root cause and fix

internal/mocks: a mistyped .Return(...) panics

Each typed mock method did v, ok := args.Get(0).(*T); if !ok { panic(...) }. A panic ends the whole test binary, not just the test:

The mocks had no *testing.T they could fail instead.

Fix:

  • internal/mocks/report.go (new). testReporter holds the test registered through the mock's Test(t) method. MockConfigStore, MockAuthStore, MockSecretsManagerClient, MockSESClient and MockSNSClient each shadow testify's Mock.Test: the shadow calls it, so testify still fails unexpected calls through t, and then records t for the mock's own checks.
  • Type-assertion sites. 82 of the 83 sites now call m.reporter.wrongReturnType("<Method>", "<type>", got). With a test registered, that fails the test (Errorf plus FailNow, like testify's unexpected-call path and t.Fatalf) under a message that names the method. With no test registered, it still panics, but the message now names the method. A zero-value mock has nowhere else to report to.
  • GetPendingExecutionsTx keeps its old panic: open PR chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 deletes that method, and changing the line would conflict with it.
  • Placement. wrongReturnType sits in stores.go next to isExpected instead of in report.go. That keeps stores.go using fmt: feat(api): scope-aware SQL totals for the History summary cards #1200 and perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240 each add a new panic(fmt.Sprintf(...)) site there, and dropping the import would break the build once they merge.

This is the mechanism, not the full rollout: a test gets the clean failure only once it calls store.Test(t). Three call sites already do (internal/purchase/approvals_test.go:688 and coverage_extra_test.go:459,523), and those now cover mistyped returns as well as unexpected calls. Registering t at the other ~1,030 construction sites is deferred (see "Out of scope").

frontend-build.yml / docs/ci-required-checks.md: a required check that never reports

A workflow with a paths: filter reports no status at all on a PR outside those paths. Following the runbook (making Build frontend required) would leave every backend-, Terraform- or docs-only PR stuck at "Expected - Waiting for status to be reported". Build frontend is not required today: the protect-default-branch ruleset requires CI Success and Run pre-commit hooks. So this is a trap waiting in the runbook, not a live outage.

Fix: drop the paths: filter so the single Build frontend job runs on every PR to main.

  • Detect step. A new first step, Detect frontend changes, diffs the test-merge commit against its first parent (fetch-depth: 2). It sets frontend=false only when no changed path matches ^(frontend/|\.github/workflows/frontend-build\.yml$). Every heavy step (Node setup, npm ci, lint, typecheck, build, jest) is gated on frontend == 'true', so a non-frontend PR passes in seconds with a log line saying why.
  • Fail-closed. If HEAD is not a merge commit, or the diff fails, it warns and builds anyway.
  • Why not a companion job. I didn't add a second job or workflow named Build frontend, the issue's suggested companion. On a mixed PR both would report under the same name, and the requirement shouldn't be satisfiable by the stand-in.
  • Docs. The doc gains a "Why the workflow has no paths: filter" section, so nobody restores the filter as an optimization. The Step 5 smoke test now also checks the docs-only case, which the old smoke test could never catch.

known-issues.md: wrong role name, stale branch

The #731 entry is rewritten and moved to Resolved > Azure, as the issue asks:

  • it names the template's actual role, "CUDly Reservation Purchaser (custom)";
  • it drops the fix/731-arm-roles branch reference;
  • it uses the template's action casing (calculatePrice/action, reservationOrders/write).

The old entry told operators to run a bare az deployment sub create. The newer #1545 entry under Outstanding says to revoke the tenant-wide grant first and then redeploy, because Azure refuses to narrow assignableScopes while assignments exist there. The moved entry now points at those steps and no longer contradicts them.

tests/e2e: /version assertion named no field

TestVersionEndpoint now requires version, git_sha and build_time (the internal/api.VersionResponse fields) to be non-empty strings. It also requires version == "test", the value docker-compose.test.yml sets on cudly-app. The issue suggested asserting commit, but the handler has no such key: the commit SHA is git_sha. The compose stack builds without a GIT_COMMIT build arg, so git_sha is the "unknown" sentinel there, and only its presence and type are asserted.

Verification

All of this is local unit, fixture and stub evidence. Nothing here ran on GitHub Actions or against the real compose stack.

Toolchain: Go 1.26.9 with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

Build, vet, lint:

  • go build ./..., go vet ./... and go vet -tags integration ./...: pass.
  • In tests/e2e: go vet -tags e2e ./... passes.
  • golangci-lint v2.10.1 run --allow-parallel-runners ./internal/mocks/: 0 issues.
  • The same in tests/e2e with --build-tags e2e: 0 issues.
  • gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: none over 10.
  • actionlint .github/workflows/frontend-build.yml (with shellcheck): pass.
  • pre-commit zizmor, markdownlint, trailing-whitespace and end-of-file-fixer on the changed files: pass.

Unit tests:

  • go test ./internal/mocks/ -count=1: ok. This includes the repo-wide guards (vacuous_assertion_guard_test.go).
  • go test ./internal/purchase/ -count=1: ok. It holds the existing store.Test(t) callers.

internal/mocks fail-before/pass-after. internal/mocks/report_test.go is new:

  • TestMockConfigStore_WrongReturnTypeFailsRegisteredTest: one default-serving method and one always-dispatching method.
  • TestMock_WrongReturnTypePanicsWithoutRegisteredTest: the fallback still panics and names the method.
  • TestMock_TestStillRegistersWithTestify: the shadowing Test still hands t to testify.
  • TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod: parses the package and drives every wrongReturnType site (82 subtests) through reflection with a mistyped .Return. Each must fail the registered test, stop the goroutine like t.Fatal, not panic, and report under its own method name.

Before the fix, with the test file copied onto origin/main in a scratch worktree, all four failed:

--- FAIL: TestMockConfigStore_WrongReturnTypeFailsRegisteredTest
    Error: Expected nil, but got: "mock: expected *config.GlobalConfig, got mocks.wrongType"   (it panicked)
--- FAIL: TestMock_WrongReturnTypePanicsWithoutRegisteredTest   (message didn't name the method)
--- FAIL: TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod
    found only 0 wrongReturnType call sites

After the fix, all pass, with 82 subtests in the sweep.

Mutation probes. Each one was applied by hand, run, and reverted:

Mutation Caught by
Put the old panic(...) back in GetGlobalConfig TestMockConfigStore_WrongReturnTypeFailsRegisteredTest/GetGlobalConfig, TestMock_WrongReturnTypePanicsWithoutRegisteredTest
Copy-paste the wrong method name ("GetGlobalConfig") into the GetServiceConfig site .../GetServiceConfig, TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod/MockConfigStore.GetServiceConfig
MockSNSClient.Test stops setting its reporter TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod/MockSNSClient.Publish
MockAuthStore.Test stops calling m.Mock.Test TestMock_TestStillRegistersWithTestify
fail() calls Errorf without FailNow TestMockConfigStore_... and every TestMocks_EveryWrongReturnTypeSite... subtest

Combined-tree check: this branch merged with #1217, #1200 and #1240 → go vet ./internal/mocks/ and go test ./internal/mocks/: ok.

tests/e2e fail-before/pass-after. This used a stub server: a throwaway Go net/http server serving a healthy /health and a chosen /version body. I ran API_URL=http://<stub> go test -tags e2e -run '^TestVersionEndpoint$' with the old test file (from origin/main) and the new one:

/version body old test new test
{"version":"test","git_sha":"unknown","build_time":"2026-10-23T00:00:00Z"} (the real shape) ok ok
{"ok":true} ok FAIL ("version" must be a non-empty string)
{"error":"not configured"} ok FAIL
{"version":"dev","git_sha":"unknown","build_time":"unknown"} (VERSION not reaching the app) ok FAIL (version = dev, want "test")
{"version":"test","build_time":"x"} (git_sha dropped) ok FAIL ("git_sha" must be a non-empty string)

I didn't run the real docker-compose e2e stack locally. CI's e2e-tests job runs it.

frontend-build.yml detect step. This is fixture evidence. I extracted the step's run: script from the YAML and ran it in throwaway git repos holding a --no-ff merge commit, which mimics refs/pull/N/merge:

Case Output
frontend/src/a.ts and internal/x.go changed frontend=true
internal/x.go and docs/y.md changed frontend=false, with the skip message
only .github/workflows/frontend-build.yml changed frontend=true
lookalikes frontend-old/a.ts and frontend-build.yml.bak frontend=false
HEAD not a merge commit frontend=true, with a warning (fail-closed)
git clone --depth 2 of the frontend and backend merges, as fetch-depth: 2 produces frontend=true / frontend=false

The real pull_request run happens when this PR is opened.

known-issues.md: I checked the role name and action casing against arm/CUDly-CrossSubscription/template.json (roleName at line 41, and the permissions.actions list).

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-337 ff3420c
fix/issue-169 7d75eb5
fix/issue-342 66b221f
fix/issue-310 5e11c21
fix/issue-326 5f0ea59
fix/issue-243 365e959
fix/issue-343 6c94036
fix/issue-312 d2848a8
fix/issue-316 acd60dd
fix/issue-339 75ea9b9
fix/issue-339-commitment-type 73bdb19
fix/issue-353 5d25fcd
fix/issue-313 3156bc4
fix/issue-357 53d9e58
fix/issue-368 0ba727d
fix/issue-419 61648c9
fix/issue-111 b04c372
fix/issue-686 0ad90b7
fix/issue-674 793095d
fix/issue-1055 4ec96f1
fix/issue-559 dfa9525
fix/issue-558 034eed4
fix/issue-874 1720539
fix/issue-666 1e120da
fix/issue-668 fb26345
fix/issue-568 ce04173
fix/issue-311 e54f250
fix/issue-177 0cbb88a
fix/issue-151 c21377c
fix/issue-166 41fb77b
fix/issue-27 2d67622
fix/issue-323 69288ae
fix/issue-59 36463e8
fix/issue-43 35e4069
fix/issue-158 6f76f8a
fix/issue-104 bb00491
fix/issue-55 971ef1b
fix/issue-361 89ac220
fix/issue-120 b458719
fix/issue-36 42bac6e
fix/issue-525 57c97dd
fix/issue-712 e5bcda0
fix/issue-72 6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

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:

  • Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
    • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
    • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
    • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
  • What the combined run caught, each fixed on its own branch before this PR opened:
    • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
    • fix/issue-316 rejecting legacy-keyed plans;
    • fix/issue-120 versus main's global-filter test.
  • Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Nothing was applied to any cloud.

Out of scope / deferred

  • Registering t at every MockConfigStore/MockAuthStore call site. That means store.Test(t) or a NewMockConfigStore(t) constructor, across about 1,030 constructions in 132 test files. It overlaps nearly every in-flight test branch, so it should follow once those land. Until then, tests that don't register t keep the panic, though it now names the method.
  • The "one policy" refactor of isExpected defaults (finding 2). Same reason. Since 4695793 it no longer affects assertion strength.
  • New panic sites in open PRs. feat(api): scope-aware SQL totals for the History summary cards #1200 (*config.HistorySummaryGroups) and perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240 (config.RecommendationChunk) each add one new panic(fmt.Sprintf("mock: expected ...")) site to stores.go. They should switch to m.reporter.wrongReturnType(...) after this merges. I didn't add a source guard that bans panic( in internal/mocks, because it would fail the combined tree with those two PRs. A guard can follow once they've switched.
  • GetPendingExecutionsTx keeps its panic because chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 deletes the method.
  • The admin steps in docs/ci-required-checks.md use the classic branch-protection API. GET .../branches/main/protection returns 404 on this repo, because required checks are enforced by the protect-default-branch ruleset. Following Step 3 would create a second, classic protection rule next to the ruleset. That's a separate docs fix and is not made here.
  • The Secrets Manager, SES and SNS mocks in internal/mocks have no callers anywhere in the repo (internal/email and internal/secrets define their own). I converted them for consistency but didn't delete them.

Frontend follow-up for the UI owner

frontend/src/__tests__/api.test.ts, test('includes status code in error') (lines 367-379), asserts only inside a catch. If apiRequest stopped rejecting on a 500, zero assertions would run and Jest would pass. Suggested replacement:

await expect(apiRequest('/test')).rejects.toMatchObject({ status: 500 });

You could add expect.assertions(1) instead. Mutation check: make apiRequest resolve null on a 5xx whose body fails to parse. The current test passes; the replacement fails.

Labels

A maintainer, please mirror the issue's triage labels: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/m.

🤖 Generated with Claude Code

…nd e2e findings

- internal/mocks: a mistyped .Return value no longer panics when the test
  registered itself with the mock's Test method; it fails that test
  (Errorf + FailNow) under the method's name. 82 of the 83 type-assertion sites
  in MockConfigStore, MockAuthStore and the Secrets Manager, SES and SNS
  mocks go through testReporter (GetPendingExecutionsTx is left alone:
  open PR LeanerCloud#1217 deletes it). Without a registered test the mock still
  panics, now naming the method.
- frontend-build.yml: drop the paths filter so `Build frontend` reports on
  every PR (a path-filtered required check blocks every PR outside its
  paths); a fail-closed first step skips the build when nothing under
  frontend/ changed. docs/ci-required-checks.md explains why.
- known-issues.md: the LeanerCloud#731 entry named a role the ARM template does not
  define and a stale branch; correct the name and move it to Resolved,
  pointing re-deploys at the #1545 revoke-first steps.
- tests/e2e: TestVersionEndpoint asserts version, git_sha and build_time
  instead of any non-empty JSON object.

Refs LeanerCloud#353

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/m Days labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 10 billable files and costs up to $2.50.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 26004a6 and 070198c.

Files selected for processing: 10
  • .github/workflows/frontend-build.yml
  • docs/ci-required-checks.md
  • internal/mocks/report.go
  • internal/mocks/report_test.go
  • internal/mocks/secretsmanager.go
  • internal/mocks/ses.go
  • internal/mocks/sns.go
  • internal/mocks/stores.go
  • known-issues.md
  • tests/e2e/e2e_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 4805cde22baf0bcfa3e834c81377001e7bd09e6a

Blocking:

  • none

Non-blocking:

Attacked:

  • Mocks: checked that testReporter is mutex-guarded, that the shadowing Test still calls mock.Mock.Test (so testify's unexpected-call path still fails through t), and that fail matches testify's Errorf plus FailNow.
  • Ran go build ./..., go vet ./internal/mocks/ (with and without -tags integration), tests/e2e go vet -tags e2e, go test -race -short -count=1 ./internal/mocks/ ./internal/purchase/ (ok), and golangci-lint v2.10.1 on internal/mocks (both tag sets) and tests/e2e (e2e tag): 0 issues. gocyclo on the changed non-test files is silent. The sweep test runs 82 subtests.
  • Fail-before: I restored stores.go, ses.go, sns.go and secretsmanager.go from the merge base and removed report.go. TestMockConfigStore_WrongReturnTypeFailsRegisteredTest, TestMock_WrongReturnTypePanicsWithoutRegisteredTest and TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod all fail. I restored the branch afterwards.
  • The head is 21 commits behind main. I merged origin/main locally (clean auto-merge of stores.go), and build, vet and go test -race ./internal/mocks/ ./internal/purchase/ still pass. Two panic sites remain in the merged tree (the two above).
  • Workflow: actionlint is clean. I extracted the Detect frontend changes script and ran it on git clone --depth 2 copies of --no-ff merge fixtures: frontend/a.ts gives true, docs/y.md gives false, and frontend-old/a.ts gives false. The working-directory: . override of the job default is correct. I checked the ARM role name against arm/CUDly-CrossSubscription/template.json:41 and the action casing at lines 48-54.

Body-stale:

Reviewer: rB4. 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.

IAmJSD and others added 2 commits October 25, 2026 18:29
Bring the branch up to date with main (7cde88e), which includes LeanerCloud#1200's
HistorySummaryAggregate mock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#1200 added HistorySummaryAggregate with the old panic on a mistyped
.Return, which ends the whole test binary. Route it through
wrongReturnType like the other sites so a registered test fails under
the method's name instead.

Refs LeanerCloud#353

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

Updated the branch. New head: cefef1f5 (cefef1f5d7c0fd8a493500345f4872a96a8e787f).

  • Merged origin/main (7cde88e) with a merge commit. There were no conflicts.
  • feat(api): scope-aware SQL totals for the History summary cards #1200 (now merged) added MockConfigStore.HistorySummaryAggregate with the old panic(fmt.Sprintf("mock: expected *config.HistorySummaryGroups, got %T", ...)) on a mistyped .Return. It now calls m.reporter.wrongReturnType("HistorySummaryAggregate", "*config.HistorySummaryGroups", args.Get(0)) like the other 82 sites. TestMocks_EveryWrongReturnTypeSiteReportsItsOwnMethod finds the new site automatically and its MockConfigStore.HistorySummaryAggregate subtest passes.
  • The only panic left in internal/mocks is GetPendingExecutionsTx. It stays on purpose, because chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 (still open) deletes that method.

Checks on the merged tree:

  • go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet: clean.
  • golangci-lint v2.10.1 on ./internal/mocks/..., with and without --build-tags integration: 0 issues. gocyclo on the changed non-test files: silent.
  • go test -race -short -count=1 ./internal/mocks/: ok.
  • secrets scan of the changed files: clean, and the planted key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS.

Pick up LeanerCloud#1418, which fixes main's red purchase integration job
(TestPostCASSave_ConcurrentStatusChangeSurvives) that this PR's CI hit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged origin/main again (26004a6) to pick up #1418. The previous head's CI failed only Integration Tests (purchase), in TestPostCASSave_ConcurrentStatusChangeSurvives/safeFail stamp after a late recovery: the mock panicked on an unexpected SendPurchaseFailedNotification. That was main's red test from #1111 + #1245, which #1418 fixes, and nothing in this PR caused it. There were no conflicts and no new panic( sites in internal/mocks. New head: 070198c9 (070198c97b1738d1cd0190465406c5e156c036f0). On the merged tree: build and vet (plain, integration, e2e) are clean, golangci-lint on internal/mocks reports 0 issues, go test -race -short ./internal/mocks/ is ok, and the secrets scan is clean.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 070198c97b1738d1cd0190465406c5e156c036f0

Blocking:

  • none

Non-blocking:

  • .github/workflows/frontend-build.yml (Detect step): the skip path (frontend=false) hasn't run on GitHub Actions yet. This PR touches the workflow file, so its own run took the full-build path. The first backend-only PR after the merge should be checked to confirm it reports Build frontend as a fast pass.
  • .github/workflows/frontend-build.yml: if grep itself errors (exit 2), the result is treated as "no frontend changes" and the build is skipped. That's practically unreachable with a fixed literal pattern, but it is the one path that fails open rather than closed.
  • internal/mocks/report.go: FailNow is only valid on the test goroutine. A mistyped .Return hit from a goroutine spawned by the code under test now Goexits that goroutine and doesn't crash the binary. That's the same behavior as testify's own unexpected-call path, so it's acceptable, but worth knowing when you debug a hang.
  • The protection is still opt-in: only the three store.Test(t) callers get the clean failure. That's disclosed and deferred, and Refs #353 is right for a partial slice.

Attacked:

  • Mock mechanism. I put the old panic(...) back in GetGlobalConfig (fail-before): TestMockConfigStore_WrongReturnTypeFailsRegisteredTest and TestMock_WrongReturnTypePanicsWithoutRegisteredTest fail; restoring the file makes them pass. I checked that every shadowing Test still calls m.Mock.Test(t), that the fatalf path does Errorf+FailNow, and that the no-t fallback still panics with the method name.
  • Workflow. I extracted the Detect step's script and ran it against --no-ff fixture merges: docs-only gives frontend=false, frontend/ gives true, and no merge parent gives true with a warning. I also ran actionlint on the file (clean). The trigger is still pull_request, permissions are contents: read and persist-credentials: false, and no untrusted ${{ }} is interpolated into run.
  • e2e: VERSION: test is a runtime env on cudly-app in docker-compose.test.yml, and getVersion reads env VERSION. So version == "test" is a real assertion, and the PR's E2E Tests check passed with it.
  • Ran go build ./..., go vet (plain and -tags integration) on internal/mocks and internal/purchase, go vet -tags e2e in tests/e2e, and go test -race -short -count=1 ./internal/mocks/ ./internal/purchase/: all ok. The head is 9 commits behind main. I did a scratch merge --no-commit origin/main: it merges cleanly and mocks/purchase still pass. Current main has removed GetPendingExecutionsTx, so the merged tree has no panic( left in internal/mocks/stores.go.
  • I checked the role name in known-issues.md against arm/CUDly-CrossSubscription/template.json:41.

Body-stale:

Reviewer: rB9. 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.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 070198c97b1738d1cd0190465406c5e156c036f0 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 25, 2026 20:26
@IAmJSD
IAmJSD merged commit cdba8ce into LeanerCloud:main Oct 25, 2026
37 checks passed
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 070198c9, green CI, and the combined-tree verification above. Squash-merged at exactly 070198c97b1738d1cd0190465406c5e156c036f0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant