[codex] Stabilize DB-backed test lanes#193
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds shared Postgres test helpers, migrates storage and UI integration tests to shared schemas with resets, and updates Makefile, CI, docs, and planning files to use the new DB test lanes and gating. ChangesDB Test Harness Redesign & Test Lane Structure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
dzarlax
force-pushed
the
fix-issue-182-db-backed-tests
branch
from
088ad3b to
4d9ce0e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
69-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the missing
X-API-Keyheader to the smoke request.
/healthis registered throughh.auth(...), so requests withoutX-API-Keyare rejected with401before the POST body is processed. As written,make testnow delegates to a smoke target that will fail against a normally configured server.Suggested fix
+API_KEY ?= + smoke-health-post: + test -n "$(API_KEY)" curl -s -X POST http://localhost$(ADDR)/health \ + -H "X-API-Key: $(API_KEY)" \ -H "Content-Type: application/json" \ -H "automation-name: Test" \ -H "automation-id: test-001" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 69 - 75, The smoke-health-post target is missing the required X-API-Key header, so the authenticated /health route will reject the request before the body is handled. Update the curl invocation in smoke-health-post to include the same API key header expected by h.auth, keeping the rest of the request intact so the smoke test can pass against a normal server configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 45-54: Add an explicit minimal permissions block for the build
workflow so the default inherited GITHUB_TOKEN access is not broader than
needed. Update the workflow near the build job/test section to set permissions
at the workflow or job level, using the build job context and the existing Test
step as the location anchor, and keep it limited to read-only repository access
such as contents: read.
In `@docs/TEST_COVERAGE.md`:
- Line 22: The Energy DB full entry in TEST_COVERAGE.md has a stale runtime
note; update the “currently around 56s after fixture narrowing” text to the
newly measured full-lane runtime after batching seed inserts. Edit the Energy DB
full row only, keeping the command and description intact, and replace the old
estimate with the current ~21.8s figure so the documentation matches the latest
verification result.
In `@internal/storage/energy_bands_test.go`:
- Around line 470-478: Remove the now-dead single-row helpers
`insertEnergySnapshot` and `insertEnergySnapshotAt` from the test file, since
the bulk seeding flow now uses `insertEnergySnapshots` directly and these
wrappers are no longer referenced. Keep the shared helpers like
`insertEnergySnapshots`, `energySnapshot`, and `energySnapshotAt` intact, and
ensure any remaining call sites are updated to use the bulk insert path so the
`unused` linter no longer flags these functions.
In `@internal/storage/recovery_stability_writer_integration_test.go`:
- Around line 151-165: After CreateSchema succeeds in
recovery_stability_writer_integration_test.go, ensure any later setup failure
cleans up the isolated schema before calling t.Fatalf. In the test flow around
CreateSchema, NewPool, and VerifyReadinessRedesignSchema, add schema-drop
cleanup on the error paths so a failed pool open or schema verification does not
leave a stale schema behind. Use the existing testdb and db/pool lifecycle
points to place the cleanup in the same setup sequence.
- Around line 196-220: The shared test DB initialization path can leak schemas
when setup fails after CreateSchema but before the DB is cached, so update the
shared initializers and cleanupSharedTestDBs to track and drop partially created
schemas even on failure. Use the existing shared initializer flow around
CreateSchema, NewPool, NewFromPool, EnsureAllTables, and
VerifyReadinessRedesignSchema to record the schema name for cleanup before any
early t.Fatalf path, and make cleanupSharedTestDBs remove those orphaned schemas
in addition to the cached shared*DB handles.
In `@internal/ui/admin_readiness_redesign_config_test.go`:
- Around line 147-151: The shared-schema reset is not failing fast because
db.EnsureReadinessRedesignTables only logs errors and the test setup ignores any
bootstrap failure. Update EnsureReadinessRedesignTables to return an error
instead of swallowing DDL issues, then check and fail immediately in
admin_readiness_redesign_config_test.go alongside the existing
EnsureSubjectiveCheckinsTable setup so the shared fixture cannot continue with a
partially reset schema.
In `@internal/ui/onboarding_handlers_test.go`:
- Around line 61-80: The fallback assertion in onboarding handler tests is too
narrow because it only fails on one specific 400 message; update the check in
the missing-schema fallback case so any HTTP 400 response is treated as a
failure for endpoints with checkMissingFallback=true. Use the existing
dispatchOnboardingHandler and ep.checkMissingFallback test path to assert that
missing schema must never be rejected as a bad request, regardless of the
response body text.
---
Outside diff comments:
In `@Makefile`:
- Around line 69-75: The smoke-health-post target is missing the required
X-API-Key header, so the authenticated /health route will reject the request
before the body is handled. Update the curl invocation in smoke-health-post to
include the same API key header expected by h.auth, keeping the rest of the
request intact so the smoke test can pass against a normal server configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0de827f-17dd-4ebd-aa49-eed67f38a40b
📒 Files selected for processing (16)
.github/workflows/ci.ymlCONTRIBUTING.mdMakefiledocs/TEST_COVERAGE.mddocs/ai-plans/2026-06-29-db-test-closure-plan.htmldocs/ai-plans/2026-06-29-db-test-harness-redesign.htmldocs/ai-plans/2026-06-29-issue-182-db-backed-integration-tests.htmlinternal/storage/baseline_reason_test.gointernal/storage/chip_calibration_writer_test.gointernal/storage/db.gointernal/storage/energy_bands_test.gointernal/storage/recovery_stability_writer_integration_test.gointernal/testdb/testdb.gointernal/ui/admin_readiness_redesign_config_test.gointernal/ui/admin_readiness_redesign_monitoring_test.gointernal/ui/onboarding_handlers_test.go
dzarlax
force-pushed
the
fix-issue-182-db-backed-tests
branch
from
4d9ce0e to
1ff5c1d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stabilizes the DB-backed integration test workflow for issue #182.
This PR makes DB tests explicit, separates fast routine checks from full domain sweeps, and reduces remote Postgres overhead in the EnergyBank verdict-band tests.
What changed
internal/testdbfor DB-test activation, test-sized pgx pools, schema lifecycle, and schema reset helpers.HEALTH_DB_TESTS=1as the required opt-in flag for DB-backed tests.storage.NewFromPoolso tests can use bounded test pools instead of production tenant pool sizing.TestMaincleanup.make test-unitmake test-dbmake test-db-ui-fastmake test-db-energy-smokemake test-db-readinessmake test-db-uimake test-db-energymake test-db-storagemake testas the existing HTTP smoke POST viasmoke-health-post.make test-unit.make test-dbmake test-db-uimake test-db-energyRuntime impact
make test-dbis now the routine DB contract check and does not run full UI, full Energy, or full Storage sweeps.make test-db-uimake test-db-energymake test-db-storageValidation
GOCACHE=/tmp/health-go-build-cache make test-unitsource ~/.health-db && HEALTH_DB_TESTS=1 GOCACHE=/tmp/health-go-build-cache make test-dbsource ~/.health-db && HEALTH_DB_TESTS=1 GOCACHE=/tmp/health-go-build-cache make test-db-energysource ~/.health-db && HEALTH_DB_TESTS=1 GOCACHE=/tmp/health-go-build-cache make test-db-storage(expected current blocker: timed out at 120s inTestChronicLoad_Integration_NoLeakageInPerDayBaseline)env -u READINESS_TEST_DSN -u PGHOST -u PGDATABASE -u PGUSER -u PGPASSWORD HEALTH_DB_TESTS=1 GOCACHE=/tmp/health-go-build-cache go test ./internal/storage -run TestSaveNaiveBaseline_NilValueWithValidReasonPersists -count=1git diff --checkObserved DB timings on the current remote Postgres path:
make test-db: UI-fast 10.7s, Energy-smoke 5.8s, Readiness 16.1smake test-db-energy: 21.8s after batching seed insertsmake test-db-ui: 41.4smake test-db-storage: currently not automatic; timed out at 120s in chronic-load writer coverageNotes
Readiness monitoring tests are intentionally excluded from the routine readiness lane for now. A trial that included
TestLoadReadinessMonitoringSummary_*ran for 68.3s and failed on a state-sensitiveOverallStatusassertion, so that path remains manual/full-storage until its fixture is narrowed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation