Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new end-to-end test file that provisions and validates RAG service behavior with five tests: single-host provisioning and pipeline query, multi-host provisioning, adding a service to an existing DB, unsupported-version failure, plus helpers for schema, ingestion, HTTP queries, and env-based API keys. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 review |
|
✅ Actions performedReview triggered.
|
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/rag_service_test.go`:
- Line 33: The test directly indexes fixture.HostIDs() into variables like host1
(and similar uses for host2/host3) which can panic if the slice is shorter; add
a guard that checks len(fixture.HostIDs()) >= N before indexing and fail the
test with a clear error (or skip) when not enough hosts are available, or use a
helper that safely returns hosts with an explicit error. Update all occurrences
that assign from fixture.HostIDs() (e.g., where host1/host2/host3 are set) to
perform this length check and handle the undersized case instead of indexing
blindly.
- Around line 120-126: Replace the fixed time.Sleep by polling the RAG endpoint
until it returns a non-empty answer: implement a helper like
waitForNonEmptyRAGAnswer(ctx, t, ragURL, query, maxWait) that repeatedly calls
queryRAGPipeline(ctx, t, ragURL, "What is pgEdge?") (sleeping a short interval,
e.g. 2–3s) until a non-empty string is returned or the deadline expires, then
use its returned answer in place of the current fixed-sleep + single query;
ensure the helper calls t.Fatalf on timeout so tests fail deterministically.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f95e89a1-ea58-4b28-99d9-b56bfc28b430
📒 Files selected for processing (1)
e2e/rag_service_test.go
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/rag_service_test.go (1)
61-93: Extract shared RAGServiceSpecbuilder to reduce duplication.The repeated
Servicesconfig blocks are nearly identical across tests. A helper (with inputs for API keys/version/hosts) will cut drift risk and simplify future model/config updates.Also applies to: 149-181, 367-403, 481-513, 560-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/rag_service_test.go` around lines 61 - 93, Extract the repeated Services block into a helper function (e.g., newRagServiceSpec or buildRagServiceSpec) that returns a *controlplane.ServiceSpec and accepts parameters for host identifiers, service Version, embedding API key, rag API key, and any other per-test overrides; replace inline literals in the tests with calls to that helper (use the helper when constructing the Services slice) so all occurrences (the blocks around lines 61–93, 149–181, 367–403, 481–513, 560–592) reuse the single builder and reduce duplication and drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/rag_service_test.go`:
- Around line 316-323: The polling helper waitForNonEmptyRAGAnswer is aborting
on transient non-200 responses because queryRAGPipeline uses require.Equal on
resp.StatusCode; change queryRAGPipeline to not call require.Equal for HTTP
status, instead handle resp.StatusCode != http.StatusOK by logging/t.Logf the
status and response body, close the response and return a sentinel (e.g. empty
body or specific error) so the caller can continue polling; apply the same fix
to the duplicate block in queryRAGPipeline (the other occurrence around lines
647-652) so transient warm-up HTTP errors don't fail the test immediately.
- Around line 249-253: In waitForServiceRunning, add an early check for the
instance entering the "failed" state so we fail fast instead of waiting to
timeout: inside the loop that iterates db.ServiceInstances (the block that
currently looks for si.ServiceInstanceID == serviceInstanceID && si.State ==
"running"), detect if si.ServiceInstanceID == serviceInstanceID && si.State ==
"failed" and immediately return an error (or fail the test) with a clear message
including serviceInstanceID and any si.FailureReason/Logs fields; keep the
existing success path for "running" unchanged.
---
Nitpick comments:
In `@e2e/rag_service_test.go`:
- Around line 61-93: Extract the repeated Services block into a helper function
(e.g., newRagServiceSpec or buildRagServiceSpec) that returns a
*controlplane.ServiceSpec and accepts parameters for host identifiers, service
Version, embedding API key, rag API key, and any other per-test overrides;
replace inline literals in the tests with calls to that helper (use the helper
when constructing the Services slice) so all occurrences (the blocks around
lines 61–93, 149–181, 367–403, 481–513, 560–592) reuse the single builder and
reduce duplication and drift.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ac0b6d6-d03d-464e-8925-d290e9d16626
📒 Files selected for processing (1)
e2e/rag_service_test.go
2 participants
Summary
This PR adds E2E tests covering the full RAG service lifecycle
Changes
TestProvisionRAGService— provisions a single-host RAG service, inserts a document with a pre-computed pgvector embedding, queries the pipeline, and asserts a non-empty answerTestProvisionMultiHostRAGService— provisions a 3-node database with a RAG instance on each host and verifies each host gets its own independent service instanceTestAddRAGServiceToExistingDatabase— verifies a RAG service can be added to an already-running database via an updateTestProvisionRAGServiceUnsupportedVersion— verifies that an unregistered image version causes the workflow to fail and the database to enter "failed" stateragServiceURL,waitForServiceRunning,setupRAGSchema,insertRAGDocument,queryRAGPipeline,ragOpenAIKey,ragAnthropicKey,getEnvOrSkipTesting
Done
Checklist
PLAT-494