ci: run unit and integration tests in every workspace module

[cristim]

Closes #1751

The gap

go.work declares six modules. Under a workspace, ./... expands to packages in the current module only, and both test steps ran bare from the repository root. So pkg, providers/aws, providers/azure, providers/gcp and tests/e2e were compiled by nothing and asserted by nothing in CI.

Measured: 1,646 test functions across those five modules had never gated a merge.

This was not an unknown hazard. The workflow documents it twice in its own comments and already loops over all six modules for govulncheck (ci.yml:312) and for the gosec SARIF scan (:333). Only the test steps were never updated. Both now mirror that loop.

Sizing first: the gate surfaces zero pre-existing failures

Run locally exactly as the new loop runs them, before writing the fix:

module	result
.	3591 pass, 0 fail (6 skip)
pkg	351 pass, 0 fail
providers/aws	525 pass, 0 fail
providers/azure	577 pass, 0 fail
providers/gcp	193 pass, 0 fail
tests/e2e	no runnable package — see below

So this stays one reviewable PR: nothing is being suppressed, because there is nothing red to suppress. No -skip, no build tags excluding modules, no continue-on-error, no narrowing of the module list.

What the loop does beyond looping

It does not abort on the first failing module. Aborting would hide every later module behind the first failure — the gosec bug from #1717 in a new place. Each run is guarded, the status recorded, and the step fails at the end with all six reported.

Every module must report a non-zero test count. A module sitting in the loop while asserting nothing is the original defect wearing a new costume, and it now fails the step by name:

::error::providers/gcp ran zero tests -- it is in the loop but asserting nothing

The count comes from ^--- (PASS|FAIL|SKIP) — top-level only, since subtest lines are indented.

tests/e2e is handled by name, not by a pattern. The sizing run caught what a comment-only reading would have missed: every file in that module is behind //go:build e2e, so go test ./... there matches no packages and exits 1. A naive loop would have failed permanently on it. It gets go vet -tags=e2e instead, which type-checks a suite nothing else in this job compiles; its tests continue to run in the existing e2e-tests job over docker compose. Both facts are stated in the workflow so a reader can see exactly what that module does and does not get.

Coverage moves to one profile per module merged into the single file the upload steps already expect, following the SARIF step's collect-then-merge shape. The mode: header is read from the first profile rather than hardcoded, because go test chooses the covermode itself and defaults to atomic under -race — a literal would have silently mislabelled the merge. Profiles are written to $RUNNER_TEMP, so no working files land in the checkout.

go mod download / go mod verify was root-only for the same reason and is looped too. Without it, five modules' dependencies were never verified.

Verification

1. The gate is live — proven in real CI, both directions. Two throwaway branches, each with the same deliberate break (TestAWSProvider_Name in providers/aws asserting a wrong value), dispatched via workflow_dispatch:

branch	contents	Unit Tests	Integration Tests
scratch/1751-proof-nofix	break only, main's workflow	✅ success — the bug	✅ success
scratch/1751-proof-withfix	break + this workflow change	❌ failure	❌ failure

A knowingly broken providers/aws test passes CI on main today. With this change it fails, attributed to the right module:

--- FAIL: TestAWSProvider_Name (0.00s)
        expected: "DELIBERATELY-BROKEN-1751"

Runs 31237364977 (control) and 31237371707 (with fix). Both branches are deleted; they exist only as evidence.

Every module reported a non-zero count, and the loop did not abort at the failure. From the with-fix run — note providers/azure and providers/gcp still ran after providers/aws failed:

==> . ran 3597 top-level test(s)
==> pkg ran 351 top-level test(s)
==> providers/aws ran 525 top-level test(s)      <-- contains the failure
==> providers/azure ran 577 top-level test(s)
==> providers/gcp ran 193 top-level test(s)
Merged 5 coverage profile(s), 21194 block(s)

The integration job reports the same five, with the root module at 3701 rather than 3597 — the ~104 //go:build integration tests that only that job adds.

2. Shell logic tested against a stub go, under bash -e (the shell Actions actually uses for run: blocks, confirmed in the act log). Four scenarios:

scenario	expected	result
all modules pass	exit 0	✅
providers/azure fails	exit 1, and modules after it still run	✅
providers/gcp runs zero tests	exit 1, named error	✅
tests/e2e fails type-check	exit 1, named error	✅

The second row is the one that matters: it is the #1717 property.

3. The merged coverage profile is real. Merged two actual per-module profiles (pkg + providers/gcp, 2334 lines) and fed the result to go tool cover: -func reports 84.0% total, -html produces a 587 KB report. Cross-module merge is valid, not just concatenation that happens to parse.

4. act — act -n -j unit-tests passes. A full local act run confirmed the step shell is bash -e (which is what the stub-go harness above reproduces) and that the per-module go mod download/verify loop runs, reporting all modules verified for all six.

That run did not finish: go mod download alone took 17m05s inside Docker on this machine, and my own 50-minute timeout cancelled it partway through the root module's tests — context canceled, exit 124, everything green up to that point. It is an incomplete run, not a failing one, and I am not counting it as a pass. The GitHub CI evidence above exercised the identical steps end to end on real runners and is the actual proof.

Cost

The integration job gains a second execution of the four non-root modules' unit suites. -tags=integration adds a tag rather than selecting only tagged files, and every //go:build integration file currently lives in the root module, so for pkg and providers/* that run is a compile-under-tag check plus a re-run. That is deliberate and commented: it is what makes an integration test added to those modules later actually gate.

Out of scope, filed separately

#1754 — the lint job is root-only for the same reason, and the five unlinted modules carry 1291 golangci-lint findings (pkg 205, providers/aws 272, providers/azure 592, providers/gcp 222; tests/e2e needs --build-tags=e2e to lint at all). Not folded in here: mixing a lint-debt excavation into a CI-gating fix buries both. That issue flags the non-cosmetic subset — 193 bodyclose in providers/azure, 18 errcheck across modules — as needing triage ahead of any mass cosmetic fix.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 50e15be0-1457-4bd9-aaae-9f90dd4b9536

📥 Commits

Reviewing files that changed from the base of the PR and between d04cf31 and bce0c23.

📒 Files selected for processing (1)

• .github/workflows/ci.yml

---

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

[cristim]

Independent adversarial review — head bce0c236

Fresh worktree at the PR head. Everything below was produced by execution, not by reading the YAML. No blocking findings. One low-severity nit and two notes, at the end.

---

The evidence branches: verified at source level, not just "the runs exist"

The two runs exist, are on the SHAs claimed, and — checked rather than assumed — the two commits differ in exactly one variable:

	ci.yml at that SHA	the break	Unit Tests
143a2b43 (nofix)	byte-identical to base e0fc45e25	provider_test.go:152 "DELIBERATELY-BROKEN-1751"	31237364977 ✅ success
61d5b80b (withfix)	byte-identical to this PR's ci.yml	same line, same string	31237371707 ❌ failure

Confirmed by diff of both fetched ci.yml blobs (gh api .../contents/...?ref=<sha>) against the base and against the head worktree. Single-variable controlled experiment. A knowingly broken providers/aws test does pass CI on main today.

---

1. Can the loop pass while doing nothing? — No. Verified by execution.

I extracted the verbatim run: bodies from ci.yml with a YAML parser (not by retyping), and ran them under bash -e — the shell Actions actually uses for run: — against a stub go, for both jobs:

scenario	unit job	integration job
all modules pass	exit 0	exit 0
providers/aws fails (first non-root)	exit 1, azure + gcp + e2e still ran	exit 1, same
providers/azure fails (mid-loop)	exit 1, gcp + e2e still ran	exit 1, same
providers/gcp runs zero tests	exit 1, ::error::providers/gcp ran zero tests	exit 1, same
tests/e2e fails type-check	exit 1, named error	exit 1, same

Mechanics that make this work, each checked:

• set -o pipefail is load-bearing. (cd … && go test …) 2>&1 | tee "$log" would otherwise report tee's status (0) and swallow every failure. It is set.
• -e is inherited from bash -e {0} and set -uo pipefail does not clear it — but every go invocation sits in an if ! condition, which is errexit-exempt. Nothing aborts early; confirmed because all six scenarios reached the merge block and the final exit "$status".
• The ^ anchor genuinely excludes subtests. The stub emitted 4 top-level --- PASS: plus 4 indented --- PASS: …/subcase lines per module; the count came back 4, not 8.
• No tag collisions: root, pkg, providers-aws, providers-azure, providers-gcp, tests-e2e — all distinct, so no log or profile overwrites.

And in real CI, both halves of the non-abort property: withfix run, Unit Tests job 93052276668 —

==> . ran 3597 top-level test(s)
==> pkg ran 351 top-level test(s)
--- FAIL: TestAWSProvider_Name (0.00s)
==> providers/aws ran 525 top-level test(s)
==> providers/azure ran 577 top-level test(s)   <-- ran AFTER the failure
==> providers/gcp ran 193 top-level test(s)     <-- ran AFTER the failure
Merged 5 coverage profile(s), 21194 block(s)

and the check-run annotations API returns failure: unit tests failed in providers/aws — attribution is real, not just a log line.

---

2. tests/e2e — a real constraint, not a disguised exemption

The thing that would make this a carve-out is go vet -tags=e2e ./... silently succeeding on zero packages, since that arm has no explicit count guard. It cannot. Probed against real go in a throwaway module whose only file is behind a tag:

go vet ./...        -> "matched no packages" ... exit 1
go test ./...       -> "matched no packages" ... exit 1
go vet -tags=e2e ./... -> exit 0

So the author's stated reason for the special case is factual, and if the e2e tag were ever renamed or those files removed, the step fails loudly rather than quietly covering nothing. The arm has no zero-work escape hatch despite lacking a count check.

go vet does type-check _test.go files (harness scenario returns exit 1 with the named error); Dockerfile.test:27 already leans on the same go vet -tags=e2e ./... for exactly this.

The tests do still run in e2e-tests — verified, not accepted: docker-compose.test.yml gives test-runner the command ["go","test","-v","-tags=e2e","./..."], and the job uses --exit-code-from test-runner. Job 93052857289 on this PR's head shows 3 --- PASS and ok github.com/LeanerCloud/CUDly/tests/e2e 0.310s. The module is tests/e2e/e2e_test.go alone, so 3/3 is the whole suite.

---

3. Coverage merge — validated on the artifact CI actually produced

Rather than reproduce it, I downloaded coverage-report (artifact 9016058931) from run 31237583907 and parsed it:

• 21195 lines; exactly one mode: line, at index 0, value atomic
• 21194 block lines, 0 malformed
• blocks per module: root 14298, providers/aws 2423, providers/azure 2140, pkg 1399, providers/gcp 934
• 0 duplicate file:range keys. The five go.mod module paths are disjoint prefixes, so nested modules are excluded from the root's ./... and nothing is double-counted.
• go tool cover -func exit 0 → 76.3%; -html exit 0 → 5.59 MB

Mixed-mode hazard: not reachable. Probed the actual default — go test -race -coverprofile (no explicit -covermode) yields mode: atomic; without -race it yields mode: set. The unit job passes -covermode=atomic to every module; the integration job passes -race to every module from the same single command. All five profiles from my local run: mode: atomic. It cannot diverge without editing that one shared line.

Downstream consumers — all four checked on this PR's own run:

• Codecov: located the file at the new absolute path (> /home/runner/work/_temp/coverage.out), then failed with Token required - not valid tokenless upload. That is pre-existing and unrelated to the path change; there is no codecov.yml in the repo and fail_ci_if_error: false, so nothing is gated on it either way.
• go tool cover -html → succeeded.
• upload-artifact → coverage-report 1,359,457 B and integration-coverage 183,824 B both uploaded.
• Coverage threshold step → Total coverage: 76.3%. I specifically checked its fragile | grep total |: on the merged profile, exactly 1 of 3771 -func output lines matches total unanchored, and replaying the pipeline verbatim gives 76.3. Widening the profile did not break it.

---

4. The disclosed duplication — reasoning holds, and it can't mask a divergence

• git grep -P '^//go:build.*integration': 34 files, all 34 in the root module. (1 //go:build e2e, 1 //go:build ignore in scripts/generate-federation-iac.go.) The claim is exact.
• The only flag difference for the four non-root modules is -short (unit) vs not (integration). testing.Short() appears 6 times, all in the root module, 0 in pkg/providers/*/tests/*. So those modules behave identically under both jobs — no test can pass under one tag set and fail under the other. The identical counts in both CI jobs (351 / 525 / 577 / 193) are consistent with that.
• Measured cost from the integration job's own timestamps: root done 03:47:19, then pkg 03:47:25, aws 03:47:35, azure 03:47:47, gcp 03:47:53 → ~34 s. Cheap for the property bought.

---

5. Workflow sweep — one root-only invocation left, already tracked

Swept every .github/workflows/*.yml. (Note: my first pass used \b in git grep -E, which POSIX ERE does not support — it returned 0 files silently. Redone with -P and a control line that must match.)

site	verdict
ci.yml:54 go vet ./... (lint job)	still root-only — but LeanerCloud/cloud-commitments-platform#174 names it explicitly ("runs golangci-lint and go vet from the repository root"). Correctly out of scope here.
ci.yml:65 gocyclo -over 10 -ignore "_test\.go" .	not root-only. Verified empirically: the filesystem walk descends into nested modules — root 2470 / providers/azure 420 / providers/aws 414 / pkg 300 / providers/gcp 197 functions. (tests/e2e contributes 0 only because its one file is e2e_test.go, excluded by -ignore.)
aws_sanity.yml:57, azure_sanity.yml:59 go test ./ci_cd_sanity_tests/...	deliberately directory-scoped inside the root module (ci_cd_sanity_tests has no go.mod) — not a blind spot.
database-migration.yml	migrate only, not module-scoped.
pre-commit.yml	installs gosec/gocyclo; no module-scoped Go run of its own.

Outside the workflows: .pre-commit-config.yaml:19 (go vet ./...) and :207 (go test -short -race ./...) are also root-only. The go-test hook is stages: [pre-push], so CI's pre-commit run --all-files skips it; the go vet hook does run in CI. Same class as ci.yml:54 — worth a line on #1754 rather than a change here.

---

Re-ran all six modules myself

Verbatim per-module loop, on this worktree, FINAL_STATUS=0:

module	this review	PR body	CI (unit)
.	3597, 0 fail	3591	3606
pkg	351, 0 fail	351	351
providers/aws	525, 0 fail	525	525
providers/azure	577, 0 fail	577	577
providers/gcp	193, 0 fail	193	193
tests/e2e	type-check clean	—	—

Counts from grep -cE '^--- (PASS|FAIL|SKIP)' on captured go test -v output (the same method the workflow uses); module attribution and all file counts from Python collections.Counter over git grep -l/-c output. 351+525+577+193 = 1646, matching the issue's headline number exactly.

---

Findings

Nit (low, non-blocking) — the covermode header is adopted, not asserted.
head -n 1 "${profiles[0]}" doesn't detect a mixed-mode merge, it silently adopts whichever mode sorted first (which is pkg, not root — the glob is alphabetical). The comment above it reads as though the mode question is handled; strictly, only the first profile's mode is. A one-line assert that all headers are equal would be the fail-loud form this repo generally prefers. I'd leave it: mixed mode is unreachable as written (per §3), and even if it happened the damage is cosmetic, since coverage percentages derive from non-zero-ness under either mode. Flagging only so the next reader doesn't over-trust the comment.

Note — the root module's test count is environment-dependent. Three different values for the same claim: 3591 (PR body), 3597 (my macOS run and the withfix CI run), 3606 (this PR's own linux CI run). Not a defect — the guard is -eq 0, not an equality — but the PR body's table states a precise number that doesn't reproduce across platforms.

Note — go mod download/verify uses set -e and does abort at the first failing module, unlike the two test loops which deliberately do not. Fine for dependency resolution (fail-fast is right there, and the preceding echo "==> … in $mod" names the module), just an asymmetry worth knowing.

Nothing else. Items 1-5 all pass; the gate does what it claims, in both directions, and the merged profile is valid rather than merely parseable.

[cristim]

Merging. Closes #1751 (p0): five of six workspace modules had tests that CI never executed.

The gate was proven live in real CI, both directions, rather than argued from the diff. Two throwaway branches with the same deliberate break to TestAWSProvider_Name in providers/aws, dispatched via workflow_dispatch:

branch	Unit Tests
break only, on main's workflow (31237364977)	success — the bug
break + this fix (31237371707)	failure

A knowingly broken money-path-adjacent test passed CI on main. Both scratch branches deleted.

Sizing came back clean: 1,646 previously-ungated test functions across five modules, zero pre-existing failures. So no split was needed and nothing was suppressed — no -skip, no build tags, no continue-on-error, no narrowing of the module list.

The independent review found no blocking findings, and verified the things most likely to make this fix hollow:

• The loop cannot pass while doing nothing. All four harness scenarios exit 1 with the right diagnostic, and later modules still run after an earlier failure — so one break cannot hide others. A module running zero tests is a named hard failure, not a silent pass.
• tests/e2e is a real constraint, not a disguised exemption. That module is entirely //go:build e2e, so go test ./... matches no packages and exits 1. It gets go vet -tags=e2e instead — and the arm has no zero-work escape hatch: probed against real go, go vet -tags=e2e ./... on zero packages fails loudly. Its tests genuinely still run in the e2e-tests docker-compose job, confirmed on this head (3/3 pass), not merely asserted.
• The duplication disclosure is exact. //go:build integration appears in 34 files, all 34 in the root module — so the integration job re-running the four non-root unit suites is the deliberate cost of making a future integration test in those modules actually gate.
• Coverage merge validated on the artifact CI actually produced, not a synthetic one.
• Workflow sweep for other root-only invocations: gocyclo was checked empirically and is not root-only — its filesystem walk descends into nested modules (root 2470 / azure 420 / aws 414 / pkg 300 / gcp 197 functions). The one remaining root-only job is lint, tracked separately as ci: lint job is root-only — 1291 golangci-lint findings across five unlinted workspace modules cloud-commitments-platform#174.

Two things the author corrected in its own work, both worth recording:

The first draft would have failed permanently on tests/e2e, and the author named that as exactly where the prohibited suppression becomes tempting. Handling it by name with the reason in the workflow — rather than by a pattern — also allowed dropping the zero-count exemption entirely, making the check stronger than specified.

The coverage mode: header was initially hardcoded to set. go test defaults to atomic under -race, so that literal would have silently mislabelled every merged profile indefinitely. It now derives from the profile.

One non-blocking nit, left deliberately: the covermode header is adopted from the first profile rather than asserted equal across all of them. Mixed mode is unreachable as written, and the consequence would be cosmetic since coverage percentages derive from non-zero-ness under either mode. Recorded so the next reader does not over-trust the comment above it.

An act run was attempted and did not complete — go mod download alone took 17m05s in Docker and a 50-minute timeout cancelled it partway. The PR body says so plainly rather than implying a pass; the GitHub CI evidence above is the proof. The partial run did confirm two things: the step shell really is bash -e, and the per-module go mod download/verify loop reports all six verified.

Note for the PRs still open: after this lands, CI gates all six modules. Rebase before merging.