chore(test): medium/low findings from the 2026-07-28 full review [platform part] #353

Split from LeanerCloud/cloud-commitments-cli#1601 (cross-component issue, monorepo split)

This issue carries the findings about internal/mocks/, frontend/, docs/ci-required-checks.md, known-issues.md, and tests/e2e/ from the original test-integrity and docs-accuracy review. The docs/cli/ finding stays on the original CLI issue (still applicable there), and the ci_cd_sanity_tests portion of the mock-panic finding is split to cloud-commitments-go (see the cross-repo item below). The README.md phantom-flags finding is dropped entirely: the CLI repo's README.md was completely rewritten as part of the split and no longer contains the example that named the non-existent flags.

Re-verified against this repo's current tree (2026-09-28); every item below still reproduces at (possibly moved) current line numbers.

MEDIUM

• frontend/src/__tests__/api.test.ts puts an assertion inside a catch with no expect.assertions()

  • Where: frontend/src/__tests__/api.test.ts:367-378, test('includes status code in error'):

    try { await apiRequest('/test'); } catch (error) { expect((error as {status?: number}).status).toBe(500); }

    Still no expect.assertions(1) and no unreachable-fail() after the await.
  • What: if apiRequest stopped throwing on a 500 whose body fails to parse (resolving null instead), the catch block never executes, zero assertions run, and Jest reports a pass.
  • Failure scenario: the test exists to prove "5xx surfaces as a rejection carrying .status"; it cannot detect the regression where 5xx stops rejecting at all.
  • Fix direction: await expect(apiRequest('/test')).rejects.toMatchObject({ status: 500 }), or add expect.assertions(1).

• MockConfigStore mixes two incompatible unregistered-method policies

  • Where: internal/mocks/stores.go - some methods short-circuit via isExpected (e.g. GetGlobalConfig, GetPurchasePlan, GetStuckRampSteps, LockPurchasePlanTx, OccupiedRampStepsInRangeTx, CancelExecutionAtomic, TransitionExecutionStatus), confirmed still gated by isExpected(&m.Mock, ...), while the rest (e.g. SaveGlobalConfig, SaveServiceConfig, ListServiceConfigs, ListPurchasePlans, SavePurchaseExecution) call m.Called unconditionally.
  • What: for the second group, an unregistered call panics inside testify, which makes AssertNotCalled genuinely load-bearing. For the first group it silently returns a default, which makes AssertNotCalled vacuous. Nothing at the call site distinguishes the two.
  • Failure scenario: a reviewer sees two adjacent AssertNotCalled lines, one real and one vacuous, and reasonably concludes both are guarding. That is how the vacuity stays invisible across the hundreds of AssertNotCalled call sites repo-wide.
  • Fix direction: pick one policy for the whole mock. If defaults must stay, make them opt-in and explicit (WithDefaultWithTx()) so a bare mock panics on unregistered calls and the call site shows which policy is in force.

• internal/mocks type-assertion failures panic instead of t.Fatal

  • Where: internal/mocks/stores.go (still 25+ panic(fmt.Sprintf("mock: expected %T, got %T", ...)) sites, e.g. in GetGlobalConfig, UpdateGlobalConfigAtomic, GetServiceConfig, ListServiceConfigs, GetPurchasePlan, GetStuckRampSteps, LockPurchasePlanTx, OccupiedRampStepsInRangeTx, ListPurchasePlans, TransitionExecutionStatus); also internal/mocks/secretsmanager.go, internal/mocks/ses.go, internal/mocks/sns.go.
  • What: a mis-typed .Return(...) aborts the goroutine via panic rather than failing the test cleanly.
  • Failure scenario: the panic bypasses t.Cleanup, so a Postgres testcontainer started by a neighbouring integration test is never terminated and leaks on the CI runner. The Go test output shows a raw stack trace instead of naming the offending test and method, turning a two-line fix into a bisect.
  • Fix direction: thread *testing.T into the mock (a t field set by a constructor) and use t.Fatalf plus t.Helper(). Matches the repo's feedback_no_panic_in_test_setup convention.

• docs/ci-required-checks.md prescribes a required check that would block every non-frontend PR

  • Where: docs/ci-required-checks.md still instructs an admin to add Build frontend to required_status_checks.contexts (Steps 2 and 3) with no mention of the path filter; .github/workflows/frontend-build.yml:24-26 still triggers only on: pull_request: paths: ["frontend/**", ".github/workflows/frontend-build.yml"].
  • What: the doc's job-name claim is correct, but a path-filtered workflow does not report a status at all on PRs that touch no matching path.
  • Failure scenario: an admin follows the runbook verbatim. Every backend-only, Terraform-only or docs-only PR then sits permanently at "Expected - Waiting for status to be reported" and cannot be merged, with no failing check to point at. The runbook's own Step 5 smoke test (a TS error under frontend/) passes and would not surface this, because that PR does touch frontend/.
  • Fix direction: add a paths-less companion job that reports Build frontend as a no-op skip (the standard "required check on a path-filtered workflow" workaround) and document the reason in the doc, so the next person does not remove it as redundant.

LOW

• known-issues.md names an ARM role that does not exist in the template

  • Where: known-issues.md:139-142 still says arm/CUDly-CrossSubscription/template.json "has been updated (fix/731-arm-roles) to add a custom role "CUDly Reservation and Savings Plan Purchaser"", versus arm/CUDly-CrossSubscription/template.json:41, where roleName is still "CUDly Reservation Purchaser (custom)".
  • What: the substance is on main (the template does enumerate the required actions), so only the role name and the "on branch fix/731-arm-roles" framing are stale.
  • Failure scenario: an operator verifying the remediation searches the Azure portal or az role definition list for the documented role name, finds nothing, and concludes the fix was never deployed. They then re-run the ARM deployment or file a duplicate 403 report.
  • Fix direction: correct the role name in known-issues.md, drop the branch reference, and move the entry to the Resolved section per the repo's known-issues convention.

• tests/e2e /version test asserts only that the body is non-empty

  • Where: tests/e2e/e2e_test.go:107-129 (TestVersionEndpoint) still decodes into map[string]any and asserts only len(body) != 0.
  • What: the test is titled "serves JSON build metadata" but names no field.
  • Failure scenario: if the version handler regressed to emitting {"error":"not configured"} or a stub {"ok":true}, losing the commit SHA that deploy verification and the rollback runbooks rely on, the test still passes.
  • Fix direction: assert the specific keys the deploy runbooks read (version, commit).

Cross-repo item (also filed in cloud-commitments-go)

• ci_cd_sanity_tests mock helper panics instead of failing the test (part of the same finding as the internal/mocks panics above, LOW/MEDIUM)
  • Where (go repo): ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go:51-60, accountJSON helper still panic(err)s on a json.Marshal error.
  • What: same class of defect as the internal/mocks panics in this repo: a test-support helper that should fail the test cleanly aborts the goroutine instead.
  • Fix direction: take t *testing.T in accountJSON and use require.NoError(t, err) instead of panic(err). See the sibling issue in cloud-commitments-go for the actual fix location.

Acceptance criteria

• Each checklist item above is fixed with a regression test or an added assertion that would have caught the described gap, matching the repo's test-integrity conventions.
• The internal/mocks panic-to-t.Fatal fix and the ci_cd_sanity_tests sibling fix in cloud-commitments-go land in a consistent shape (both threading *testing.T through, not one using a package-level panic and the other not).