Repository navigation
fix: wire five dark Go tests into CI and fix the two fixtures that blocked them (#708) - #709
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCI now runs catalog, provider, chat, auth, and inference integration suites. Test helpers fail in CI when database configuration is missing. Catalog and provider fixtures now seed required database values. ChangesDatabase integration coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/control-plane/internal/catalog/catalog_integration_test.go (1)
141-161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRegister cleanup before setup can fail.
The tenant cleanup is registered at Line [155], after both tenant inserts and both alias inserts. If a later seed fails,
t.Fatalruns before this cleanup is registered, so earlier tenant rows remain in the database. Register cleanup immediately after assigning the tenant IDs, before any seed call.🤖 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 `@apps/control-plane/internal/catalog/catalog_integration_test.go` around lines 141 - 161, Move the t.Cleanup registration in the catalog integration test to immediately after tenantA and tenantB are assigned, before seedTenant or seedAlias runs. Keep the existing visibility-row and tenant deletion operations unchanged so partial setup failures still clean up all seeded rows.
🤖 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.
Outside diff comments:
In `@apps/control-plane/internal/catalog/catalog_integration_test.go`:
- Around line 141-161: Move the t.Cleanup registration in the catalog
integration test to immediately after tenantA and tenantB are assigned, before
seedTenant or seedAlias runs. Keep the existing visibility-row and tenant
deletion operations unchanged so partial setup failures still clean up all
seeded rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4b83a6d-9d79-4a0a-be8d-6e5522559241
📒 Files selected for processing (6)
.github/workflows/ci.yml.wolf/buglog.jsonlapps/control-plane/internal/catalog/catalog_integration_test.goapps/control-plane/internal/providers/integration_test.goapps/edge-api/internal/auth/tenant_fallback_test.goapps/edge-api/internal/chat/dispatch_test.go
REQUEST CHANGESTwo blockers. Both are in the wiring, not in the fixture fixes. The fixture fixes themselves are correct and no assertion was weakened. The second one matters more than it looks, because this PR's own control-plane leg went green with it present. Blocker 1: the
|
…st step (#709 review) Two defects from independent review of #709. The CI=true loud t.Fatal added for issue #708 fired in the wrong step for the two build-tag-free files (chat/dispatch_test.go, auth/tenant_fallback_test.go). They compile into the plain `go test ./... -short` step, which runs before the DSN export step and never has a DSN by design, so the guard now also checks !testing.Short() to tell that expected case apart from a real missing-DSN bug in the RLS step. litellmconfig's precedent test is build-tagged and never enters the -short step at all, which is why it did not surface this. ci.yml's RLS step ran up to 12 package patterns under Go's default GOMAXPROCS parallelism against one shared live database. routing's catalog_pricing test scans all of model_aliases unfiltered and asserts exactly one enabled route per alias; the new catalog test briefly holds two zero-route aliases. Concurrent runs can collide. Added -p 1 to both matrix legs' RLS step invocations to force sequential execution; the DB suites cost about 8 seconds total, which is negligible next to the flakiness risk. Also: providers/integration_test.go now asserts the route_id insert returned the value the fixture requested instead of computing and discarding it. catalog_integration_test.go's cleanup now tracks which of the two seeded tenant rows this test call actually created (ON CONFLICT DO NOTHING can no-op) and only deletes those, surfacing any cleanup error via t.Errorf instead of discarding it. Refs #708, #709.
|
Both defects fixed in 26b0903, pushed to this branch. Blocker 1: loud guard firing in the wrong step
if os.Getenv("CI") != "" && !testing.Short() {
t.Fatal(...)
}
Blocker 2: parallel packages sharing one DBAdded Nits
VerificationLocal, exact command requested ( Real CI, this head (run 30926685151),
Both jobs report the "go test — RLS suites" step completed (not skipped), and |
…ocked them (#708) Issue #708 found the same failure shape #701/#705 already fixed, repeated five more times: HIVE_TEST_DB_URL-gated tests in edge-api/internal/chat and edge-api/internal/auth were never included in the RLS step's package list; edge-api/internal/inference's integration tests (no DB needed) never got -tags integration; and control-plane/internal/catalog and control-plane/internal/providers were gated behind CATALOG_TEST_DB_URL/PROVIDERS_TEST_DB_URL, neither of which was ever wired into any workflow. The catalog and providers integration tests had never once passed against a real schema. TestTenantVisibilityIntegration inserted tenant_model_visibility rows for two hardcoded tenant UUIDs that were never inserted into public.tenants, so the FK on tenant_id could never be satisfied. TestProviderCRUDIntegration's raw provider_routes INSERT omitted route_id, a text primary key with no default, so the not-null constraint could never be satisfied. Both are fixture bugs, not assertion bugs: fixed by seeding the missing tenant rows and supplying an explicit route_id, with no assertions weakened. CI wiring: added CATALOG_TEST_DB_URL and PROVIDERS_TEST_DB_URL to the existing ephemeral-Postgres bootstrap step (same DSN as HIVE_TEST_DB_URL/ROUTING_TEST_DB_URL/LITELLM_TEST_DB_URL); added catalog and providers to control-plane's -tags integration invocation; added -tags integration plus chat, auth, and inference to edge-api's RLS step. Added a CI=true loud t.Fatal, matching #705's precedent, to every connect/skip helper touched so a future missing env var fails the build instead of silently skipping again. apps/edge-api/internal/audio/live_voice_integration_test.go is deliberately left untouched: its round trip needs a real GROQ_API_KEY and belongs in the paid live-integration lane, not this free job. Verified against an ephemeral pgvector/pgvector:pg17 container with the full 86-migration chain applied: all five newly wired test surfaces pass with explicit PASS lines, and the full curated control-plane and edge-api package lists that ci.yml now runs pass cleanly on a fresh database (control-plane 12/12 ok, edge-api 5/5 ok).
…st step (#709 review) Two defects from independent review of #709. The CI=true loud t.Fatal added for issue #708 fired in the wrong step for the two build-tag-free files (chat/dispatch_test.go, auth/tenant_fallback_test.go). They compile into the plain `go test ./... -short` step, which runs before the DSN export step and never has a DSN by design, so the guard now also checks !testing.Short() to tell that expected case apart from a real missing-DSN bug in the RLS step. litellmconfig's precedent test is build-tagged and never enters the -short step at all, which is why it did not surface this. ci.yml's RLS step ran up to 12 package patterns under Go's default GOMAXPROCS parallelism against one shared live database. routing's catalog_pricing test scans all of model_aliases unfiltered and asserts exactly one enabled route per alias; the new catalog test briefly holds two zero-route aliases. Concurrent runs can collide. Added -p 1 to both matrix legs' RLS step invocations to force sequential execution; the DB suites cost about 8 seconds total, which is negligible next to the flakiness risk. Also: providers/integration_test.go now asserts the route_id insert returned the value the fixture requested instead of computing and discarding it. catalog_integration_test.go's cleanup now tracks which of the two seeded tenant rows this test call actually created (ON CONFLICT DO NOTHING can no-op) and only deletes those, surfacing any cleanup error via t.Errorf instead of discarding it. Refs #708, #709.
26b0903 to
d83d72a
Compare
1 participant
Summary
Issue #708 audited every Go test that can skip or is build-tag-gated and found six items never executing in CI. This PR addresses the Go side of that audit: five tests wired for real, one deliberately deferred.
Never ran, now wired
TestDispatchHappyPathWritesLLMTraceAndAuditsChatRequest(apps/edge-api/internal/chat) —HIVE_TEST_DB_URLwas already exported at the CI job level, but the RLS step's edge-api package list never included./internal/chat/....TestTenantFallback_*tests (apps/edge-api/internal/auth) — same gap,./internal/auth/...was also missing from that package list.TestChatCompletions_ToolCapable_AllowsToolsplus four sibling tests (apps/edge-api/internal/inference) — needs no database at all (mocks LiteLLM and control-plane withhttptest); never ran purely because the edge-api leg never passed-tags integration.TestTenantVisibilityIntegration(apps/control-plane/internal/catalog) — gated behindCATALOG_TEST_DB_URL, which appeared nowhere under.github/.TestProviderCRUDIntegrationandTestProviderCRUDIntegration_MissingToken(apps/control-plane/internal/providers) — gated behindPROVIDERS_TEST_DB_URL, same gap.Items 4 and 5 had never once passed
Both fixtures had a real bug that made them impossible to pass against any real schema, which is exactly why nobody had noticed:
TestTenantVisibilityIntegrationinsertedtenant_model_visibilityrows for two hardcoded tenant UUIDs (a0000000-...-0001,b0000000-...-0001) that were never inserted intopublic.tenants.tenant_model_visibility.tenant_idhas a foreign key totenants(id), so every insert failed withinsert or update on table "tenant_model_visibility" violates foreign key constraint "tenant_model_visibility_tenant_id_fkey".TestProviderCRUDIntegration's rawprovider_routesINSERT omittedroute_id, atext primary keywith no default, so every insert failed withnull value in column "route_id" of relation "provider_routes" violates not-null constraint.Both are fixture bugs, not assertion bugs. The fix is fixture setup only:
seedTenantnow inserts the two tenant rows (with cleanup) before any visibility row references them, and theprovider_routesinsert now supplies an explicitroute_id(route-<slug>, derived from the test's own unique slug). No assertion was weakened, deleted, or loosened.CI wiring
CATALOG_TEST_DB_URLandPROVIDERS_TEST_DB_URLto the existing ephemeral-Postgres bootstrap step, pointed at the same DSN asHIVE_TEST_DB_URL/ROUTING_TEST_DB_URL/LITELLM_TEST_DB_URL(the pattern PR fix: point LiteLLM config sync at provider_routes.provider_model #705 established for issue LiteLLM config sync queries a column that does not exist, so the provider catalog can never reach LiteLLM #701)../internal/catalog/...and./internal/providers/...to control-plane's-tags integrationinvocation.-tags integrationplus./internal/chat/...,./internal/auth/...,./internal/inference/...to edge-api's RLS step.CI=trueloudt.Fatal(matching fix: point LiteLLM config sync at provider_routes.provider_model #705's precedent) to every connect/skip helper touched —catalog,providers,chat,auth— so a future missing env var fails the build instead of shipping another invisible skip.Deliberately out of scope
apps/edge-api/internal/audio/live_voice_integration_test.gois untouched. Its round trip needs a realGROQ_API_KEYand belongs in the paidlive-integrationlane, not this free job. Its gating is otherwise correct.Verification
Ran against an ephemeral
pgvector/pgvector:pg17container with the full 86-migration chain applied (bootstrap SQL + every file undersupabase/migrations/), via the same toolchain image CI uses.All five newly wired test surfaces pass with explicit
--- PASSlines:Also ran the exact package lists
ci.ymlnow invokes for both matrix legs, against a freshly migrated database (single pass, matching real CI's fresh-container-per-job semantics):No shared Supabase pooler was touched at any point.
Test plan
tenant_model_visibility, not-null violation onprovider_routes.route_id--- PASSlines against a real, freshly migrated databaseci.ymlnow runs for both matrix legs pass cleanly on a fresh databaseCI=trueloud-fail path added to every touched connect/skip helper, matching PR fix: point LiteLLM config sync at provider_routes.provider_model #705's precedentapps/edge-api/internal/audio/live_voice_integration_test.goleft untouched and explicitly called out as deferred to the paid live-integration laneRefs #708. Related: #701, #705.
Summary by CodeRabbit
Tests
Chores