Repository navigation
test(unit): run internal/app's tests in parallel to fit the unit budget - #811
EricAndrechek wants to merge 1 commit into
Conversation
internal/app's 77 tests ran one after another and took 6 to 8s of test time alone under -race -cover, against the 15s make test-unit gives each package. Most of that was waiting, not work: the binary used about 2.5s of CPU over 8s of wall time. - TestMain silences the default logger once, and newApp no longer swaps it or the OTel providers per test. guardGlobals stays only in the serial tests that turn Prometheus on or replace the logger themselves. - Every test that leaves process-wide state alone calls t.Parallel, and so do its subtests where they are independent. The tests that capture the logger, turn on Prometheus, set the environment or signal the process stay serial, as does TestNew_NestedDirectory, whose subtests run before the reload it then checks. - Two serial tests waited out a production retry. A moved tenant's schema discovery retried after a random wait of up to 2s (more when an attempt came before the fix), and the DynamoDB table check after 1s. discoveries.backoff and App.dynamoRetry now hold those first waits, at their production values, and each test shortens its own. The package now takes 2.2 to 3.1s of test time alone, from 5.9 to 7.7s interleaved on the same machine, with coverage unchanged at 93.0%. internal/mq already fit after #745 (2.6s alone). Closes #739 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Summary
Merge Risk: ⚪ Minimal · up to The retry behavior retains its production defaults, and the inspected parallel tests isolate their state. A reported shuffled-run failure was not reproduced; no actionable merge-blocking risk is established, so the PR appears ready for normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 74.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 8 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check The PR addresses the active coding requirement in #739to keep the affected unit packages within the 15-second budget. Ininternal/app, eligible tests run in parallel, tests that use process-wide st…Out of Scope Changes check The changes stay within #739. Theinternal/appparallel test changes, process-wide state isolation, configurable test retry waits, and the changelog entry all support the unit-test budget objective.…Title check The title clearly and concisely describes the main change: running eligible internal/app unit tests in parallel to reduce test time. Description check The description directly explains the parallel test changes, preserved serial tests, configurable retry waits, measured performance improvement, and verification results.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 74.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 8 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
📝 Generate docstrings 💡
-
-
-
- Commit to this branch
- Create a new PR
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
✨ Simplify code
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
github-actions
Bot
added
go
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 6f7aca4 in the Show a line coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
internal/app's unit tests ran one after another and took 6–8 s of test time alone (8.3–10.6 s in CI's unit job), against the 15 s thatmake test-unitgives each package. Most of that was waiting: about 2.6 s of CPU over 7–8 s of wall time.TestMainsilences the default logger once, andnewAppno longer swaps it or the OTel providers per test. Tests that leave process-wide state alone run in parallel. Tests that capture the logger, turn on Prometheus, set the environment or send signals stay serial.discoveries.backoffandApp.dynamoRetryhold those first waits at their production values, and each test shortens its own.internal/mqalready fits: test(mq): tolerate the nats version advisory and speed up topology tests #745 madeTestVerifyNATSTopology_Findingsrun its cases in parallel after test(unit): internal/mq and internal/app use 10-11 s of the 15 s unit budget #739 was filed.Verified
internal/appalone: 2.2–3.1 s of test time, down from 5.9–7.7 s, measured with old and new binaries interleaved on the same machine. Coverage is unchanged at 93.0%.go test -race -count=5passes, and so do-count=20 -cpu 4and-cpu 2. A run under load besideinternal/dedupe -count=3passes too.-shuffle=on, alternating-cpu 1and-cpu 4, pass. One failure was seen in an earlier batch of 19 shuffled runs while the machine was also running a full suite; the failing test wasn't captured, and the 40 later runs did not reproduce it.make verify,make lint-go(including tparallel),make test-unitandmake ciare green.Closes #739
🤖 Generated with Claude Code