Add lightweight API test workflow with mock VLLM - #194
Conversation
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow and CI test stack that brings up a Docker Compose environment (mock VLLM, Postgres, Etcd, MinIO, Milvus, OpenRAG), waits for readiness, runs pytest-based API tests in Changes
Sequence Diagram(s)sequenceDiagram
actor GitHubActions as "GitHub Actions"
participant DockerCompose as "Docker Compose"
participant MockVLLM as "mock-vllm"
participant Milvus as "Milvus"
participant OpenRAG as "OpenRAG API"
participant TestRunner as "Pytest (httpx)"
GitHubActions->>DockerCompose: checkout + docker compose up -d
DockerCompose->>MockVLLM: start service
DockerCompose->>Milvus: start service
DockerCompose->>OpenRAG: build & start (depends on others)
GitHubActions->>MockVLLM: poll /health until ready
GitHubActions->>OpenRAG: poll /health_check until ready
GitHubActions->>TestRunner: run pytest tests/api_tests/
TestRunner->>OpenRAG: send API requests (health, index, search, etc.)
OpenRAG->>MockVLLM: request embeddings
OpenRAG->>Milvus: store/query vectors
TestRunner->>GitHubActions: reports results
GitHubActions->>DockerCompose: docker compose down -v (always)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Adds a new GitHub Actions workflow that tests the OpenRAG API without requiring VLLM or heavy ML dependencies. This runs much faster than the existing smoke tests (2-3 min vs 10-30+ min). Key components: - Mock VLLM server that returns fake embeddings for testing - Lightweight docker-compose with PostgreSQL, Milvus, and mock VLLM - Comprehensive pytest test suite (38 tests) covering: - Health checks and OpenAPI docs - Partition CRUD operations - File upload and indexing - Semantic search - User management - Queue and task management - Tools API - OpenAI-compatible endpoints The workflow runs on every PR and push to main/dev branches. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4c00952 to
dceb272
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: 8
🤖 Fix all issues with AI agents
In @.github/workflows/api_tests.yml:
- Around line 41-43: The pipeline step that waits for mock-vllm uses the command
"timeout 60 bash -c 'until curl -sf http://localhost:8000/health 2>/dev/null; do
sleep 2; done' || true" which masks failures; remove the "|| true" so the job
fails when the timeout is reached (or replace it with an explicit error/log and
exit 1) to ensure the workflow does not proceed if mock-vllm failed to start.
In @tests/api_tests/test_extract.py:
- Around line 8-12: The test method test_get_nonexistent_extract currently
allows a 500 status which masks server errors; update the assertion in
test_get_nonexistent_extract (where
api_client.get("/extract/nonexistent-id-12345") is called) to assert
response.status_code == 404 only so missing resources must return Not Found; if
the API intentionally returns 500 for missing extracts, change the API instead,
but do not keep accepting 500 in this test.
In @tests/api_tests/test_indexer.py:
- Around line 127-130: The test test_get_nonexistent_task currently accepts 500
which masks server errors; change the assertion to require a 404 for the
api_client.get("/indexer/task/nonexistent-task-12345") call by replacing the
permissive assertion (response.status_code in [404, 500]) with a strict check
for 404 (e.g., assert response.status_code == 404) so the test fails on internal
server errors instead of treating them as expected.
In @tests/api_tests/test_openai_compat.py:
- Around line 31-41: The test function test_chat_completions_invalid_model
currently allows a 200 success for a nonexistent model; change the assertion so
the API must return an error status (not 200). Update the assertion on the
response from api_client.post("/v1/chat/completions", ...) to assert
response.status_code is in an error set (e.g., [400, 404, 500]) and optionally
assert the response body contains an error field/message to ensure the invalid
model is reported.
- Around line 22-29: The test_chat_completions_endpoint currently allows 500
which can hide validation bugs; update the assertion in
test_chat_completions_endpoint so the accepted response.status_code values are
only 400, 404, or 422 (remove 500) and/or add an explicit failure if status_code
== 500 to ensure server errors are surfaced; modify the assertion in that
function to assert response.status_code in [400, 404, 422] (or add a separate
assert response.status_code != 500) to tighten the expected outcomes.
In @tests/api_tests/test_partition.py:
- Around line 28-35: Update test_create_duplicate_partition_fails to explicitly
assert the first api_client.post(f"/partition/{test_partition_name}") returned a
success status (e.g., 200 or 201) before making the second POST so a failing
first call can't mask failures; remove the manual cleanup delete and rely on the
existing test fixture for teardown (or convert the test to use the fixture that
guarantees partition cleanup), keeping the duplicate-create assertion that
response.status_code is in [400, 409].
In @tests/api_tests/test_search.py:
- Around line 57-65: The test_search_multiple_partitions test currently ignores
the indexed_partition fixture; update the API call in
test_search_multiple_partitions to pass the partition filter using the
indexed_partition value (e.g., add the appropriate partition parameter to the
params dict sent to api_client.get for "/search") so the search is constrained
to the partition known to be indexed; ensure you use the correct query key
expected by the endpoint (e.g., "partition" or "partitions") when adding
indexed_partition to the params.
In @tests/api_tests/test_users.py:
- Around line 32-35: The test function test_get_nonexistent_user currently
accepts a 500 as valid, which masks server errors; update the assertion in
test_get_nonexistent_user (in tests/api_tests/test_users.py) to require a 404
Not Found (e.g., assert response.status_code == 404) so the test fails on 500
responses and surfaces server errors for fixing.
🧹 Nitpick comments (15)
tests/api_tests/test_actors.py (2)
2-2: Unused import.
pytestis imported but not used in this module.Suggested fix
"""Ray actors API tests.""" -import pytest
8-18: Test is very permissive - consider tightening assertions.Accepting 307 (redirect) and 403 (forbidden) alongside 200 means this test will pass even when the endpoint is misconfigured or authorization is broken. If 307 or 403 are expected in CI, consider using
follow_redirects=Trueon the client or documenting why these are acceptable. Otherwise, this test provides limited validation.tests/api_tests/test_tools.py (2)
2-2: Unused import.
pytestis imported but not used in this module.Suggested fix
"""Tools API tests.""" -import pytest
18-25: Missing status code assertion before parsing response.If the endpoint returns a non-200 response,
response.json()may fail or return unexpected data, leading to confusing test failures.Suggested fix
def test_tool_has_required_fields(self, api_client): """Test that tools have required fields.""" response = api_client.get("/v1/tools") + assert response.status_code == 200 data = response.json() for tool in data: assert "name" in tool assert "description" in tooltests/api_tests/test_extract.py (1)
5-18: Consider adding positive test cases.Currently only negative scenarios are tested. Consider adding a test that verifies successful extract retrieval with a valid ID (possibly after creating/indexing a document first).
tests/api_tests/test_users.py (1)
23-30: Consider asserting successful info_response first.Line 27 silently defaults to
user_id=1if the ID is missing. While this provides a fallback, it may mask issues with the/users/infoendpoint. Consider assertinginfo_response.status_code == 200before extracting the ID.💡 Suggested improvement
def test_get_user_by_id(self, api_client): """Test getting user by ID.""" # First get current user to know a valid ID info_response = api_client.get("/users/info") + assert info_response.status_code == 200 user_id = info_response.json().get("id", 1) response = api_client.get(f"/users/{user_id}") assert response.status_code == 200tests/api_tests/test_indexer.py (2)
68-92: Test accepts too many outcomes, reducing its value.The fixed
time.sleep(2)is fragile and may be insufficient on slower systems. More importantly, accepting both success (200/201/202) and error (400/409) status codes means the test doesn't enforce any specific behavior for duplicate uploads. Consider either:
- Deciding on the expected duplicate-upload behavior (replace or reject) and testing only that path, or
- Splitting into two separate tests: one for replacement and one for rejection.
Also consider polling the task status instead of using a fixed sleep.
112-119: URL parsing is fragile.Line 115 uses brittle string manipulation (
task_url.split("/")[3:]) that assumes a specific URL structure. Consider usingurllib.parsefor more robust URL parsing.💡 Suggested improvement
+from urllib.parse import urlparse + # ... # Extract task ID from response if "task_status_url" in data: task_url = data["task_status_url"] - # Get relative path - task_path = "/" + "/".join(task_url.split("/")[3:]) + # Get path from URL + parsed = urlparse(task_url) + task_path = parsed.path elif "task_id" in data: task_path = f"/indexer/task/{data['task_id']}" else: pytest.skip("No task ID in response")tests/api_tests/test_openai_compat.py (1)
43-50: Test is too permissive with acceptable status codes.Accepting both success (200) and multiple error codes (400, 404, 422, 500) makes this test non-deterministic and reduces its value. Consider testing specific scenarios separately (e.g., endpoint disabled vs. valid request) or removing 500 from accepted codes.
tests/api_tests/test_queue.py (1)
15-23: Consider validating response structure and filter correctness.The filtered task tests only verify status codes. Consider adding assertions to validate the response structure (e.g., presence of "tasks" key) and, if feasible, that the returned tasks match the requested filter status.
♻️ Proposed enhancement
def test_list_active_tasks(self, api_client): """Test listing active tasks.""" response = api_client.get("/queue/tasks", params={"task_status": "active"}) assert response.status_code == 200 + data = response.json() + assert "tasks" in data def test_list_completed_tasks(self, api_client): """Test listing completed tasks.""" response = api_client.get("/queue/tasks", params={"task_status": "completed"}) assert response.status_code == 200 + data = response.json() + assert "tasks" in datatests/api_tests/test_partition.py (2)
15-26: Prefer thecreated_partitionfixture for automatic cleanup.Manual cleanup on Line 26 could be skipped if the test fails before reaching that line, potentially polluting subsequent tests. Consider using the
created_partitionfixture which ensures cleanup even if assertions fail.♻️ Proposed refactor using fixture
-def test_create_partition(self, api_client, test_partition_name): +def test_create_partition(self, api_client, created_partition): """Test creating a new partition.""" - response = api_client.post(f"/partition/{test_partition_name}") - assert response.status_code in [200, 201] - # Verify it exists response = api_client.get("/partition/") partitions = [p["partition"] for p in response.json()["partitions"]] - assert test_partition_name in partitions - - # Cleanup - api_client.delete(f"/partition/{test_partition_name}") + assert created_partition in partitions
51-54: Status code 500 seems overly permissive for a not-found scenario.Accepting 500 (internal server error) for a non-existent partition lookup might mask actual server bugs. Consider whether 404 is the expected response and if 500 should be investigated separately.
💡 Suggested adjustment
def test_delete_nonexistent_partition(self, api_client): """Test deleting non-existent partition returns error.""" response = api_client.delete("/partition/nonexistent-partition-xyz123") - assert response.status_code in [404, 500] + assert response.status_code == 404If the API legitimately returns 500 for this case, consider documenting why or filing an issue to improve the API's error handling.
tests/api_tests/test_search.py (3)
25-33: URL parsing logic is brittle.The task URL parsing on Lines 27-28 assumes a specific URL structure (
task_path = "/" + "/".join(task_url.split("/")[3:])). This could break if the API changes URL formats or if the URL contains query parameters.♻️ More robust URL parsing
# Wait for indexing to complete if "task_status_url" in data: task_url = data["task_status_url"] - task_path = "/" + "/".join(task_url.split("/")[3:]) + # Parse URL to extract path, handling both absolute and relative URLs + from urllib.parse import urlparse + parsed = urlparse(task_url) + task_path = parsed.path elif "task_id" in data: task_path = f"/indexer/task/{data['task_id']}" else:
35-43: Consider the implications of usingpytest.skipin a fixture.When indexing fails (Line 42),
pytest.skipis called from within the fixture. This will skip all tests that depend onindexed_partition, which may be the intended behavior. However, this could mask underlying indexing issues if they occur frequently.Consider whether failing loudly (raising an exception) would be more appropriate for CI environments to catch indexing problems early, or if skipping is acceptable for flaky test environments.
78-94: Overly permissive status codes may hide bugs.Both
test_search_empty_query(Line 85) andtest_search_nonexistent_partition(Line 94) accept status code 500. Internal server errors might indicate actual bugs rather than expected edge-case behavior.💡 More specific status code expectations
def test_search_empty_query(self, api_client, indexed_partition): """Test search with empty query.""" response = api_client.get( f"/search/partition/{indexed_partition}", params={"text": "", "top_k": 5} ) - # Should return error or empty results - assert response.status_code in [200, 400, 422] + # Empty query should return validation error + assert response.status_code in [400, 422] def test_search_nonexistent_partition(self, api_client): """Test searching non-existent partition.""" response = api_client.get( "/search/partition/nonexistent-partition-xyz", params={"text": "test", "top_k": 5} ) - # May return empty results or error - assert response.status_code in [200, 404, 500] + # Non-existent partition should return not found + assert response.status_code == 404If the API legitimately returns 500 for these cases, document why or consider filing API improvement issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/api_tests.yml.github/workflows/api_tests/Dockerfile.mock-vllm.github/workflows/api_tests/docker-compose.yaml.github/workflows/api_tests/fixtures/sample.txt.github/workflows/api_tests/mock_vllm.pytests/api_tests/__init__.pytests/api_tests/conftest.pytests/api_tests/test_actors.pytests/api_tests/test_extract.pytests/api_tests/test_health.pytests/api_tests/test_indexer.pytests/api_tests/test_openai_compat.pytests/api_tests/test_partition.pytests/api_tests/test_queue.pytests/api_tests/test_search.pytests/api_tests/test_tools.pytests/api_tests/test_users.py
🧰 Additional context used
🧬 Code graph analysis (10)
tests/api_tests/test_health.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_partition.py (1)
tests/api_tests/conftest.py (3)
api_client(16-19)test_partition_name(81-83)created_partition(87-96)
tests/api_tests/test_actors.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_extract.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_tools.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_queue.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_indexer.py (1)
tests/api_tests/conftest.py (4)
api_client(16-19)created_partition(87-96)sample_text_file(39-53)sample_markdown_file(57-77)
tests/api_tests/test_users.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_openai_compat.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_search.py (1)
tests/api_tests/conftest.py (3)
api_client(16-19)created_partition(87-96)sample_text_file(39-53)
🪛 actionlint (1.7.10)
.github/workflows/api_tests/docker-compose.yaml
4-4: "jobs" section is missing in workflow
(syntax-check)
4-4: "on" section is missing in workflow
(syntax-check)
4-4: unexpected key "services" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
126-126: unexpected key "volumes" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🪛 Ruff (0.14.10)
tests/api_tests/conftest.py
95-96: try-except-pass detected, consider logging the exception
(S110)
95-95: Do not catch blind exception: Exception
(BLE001)
tests/api_tests/test_search.py
57-57: Unused method argument: indexed_partition
(ARG002)
.github/workflows/api_tests/mock_vllm.py
38-38: Probable use of insecure hash functions in hashlib: md5
(S324)
80-80: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-tests
- GitHub Check: index-backup-restore
🔇 Additional comments (22)
tests/api_tests/__init__.py (1)
1-1: Inconsistency: PR summary mentions extensive test infrastructure but only__init__.pyprovided for review.The PR objectives describe adding 38 test modules, mock_vllm.py, docker-compose configuration, Dockerfile.mock-vllm, GitHub Actions workflow, and test fixtures, but only this package initialization file has been provided for review.
Please clarify:
- Are the other files intentionally excluded from this review?
- If included, please provide them so a comprehensive review can be performed on the full test infrastructure.
The
__init__.pyfile itself is correct as a package marker..github/workflows/api_tests/Dockerfile.mock-vllm (1)
1-6: LGTM - Clean minimal Dockerfile for CI testing.The Dockerfile is appropriately lightweight for a mock service. Consider pinning package versions for reproducibility (e.g.,
fastapi==0.115.0 uvicorn==0.32.0 pydantic==2.10.0), though this is optional for a CI-only mock service where reproducibility is less critical..github/workflows/api_tests/docker-compose.yaml (1)
1-127: Well-structured Docker Compose configuration for API testing.The service dependencies, health checks, and configuration are well thought out. The static analysis hints from actionlint are false positives - actionlint incorrectly parsed this docker-compose.yaml as a GitHub Actions workflow file due to its location under
.github/workflows/.Note: The hardcoded credentials (postgres password, minio keys) are acceptable for an isolated CI test environment.
.github/workflows/api_tests/mock_vllm.py (2)
36-43: MD5 usage is acceptable here for deterministic test data.The static analysis flag (S324) about MD5 is a false positive in this context. MD5 is not being used for cryptographic security but rather to generate deterministic fake embeddings from text, which is appropriate for test mocking.
78-80: Binding to 0.0.0.0 is required for Docker container usage.The static analysis flag (S104) about binding to all interfaces is expected here since this server runs inside a Docker container and needs to accept connections from other containers in the compose network.
tests/api_tests/test_health.py (1)
1-23: LGTM - Clean health check and OpenAPI validation tests.The tests are focused and appropriately verify API availability and documentation accessibility. Good foundation for the test suite.
.github/workflows/api_tests/fixtures/sample.txt (1)
1-42: LGTM - Good test fixture content.The fixture provides structured, meaningful content for testing file upload and indexing functionality. The size and structure are appropriate for fast CI tests.
tests/api_tests/test_users.py (2)
8-13: LGTM!Test correctly validates the current user info endpoint returns 200 with an ID field.
15-21: LGTM!Test appropriately validates the list users endpoint and acknowledges the unauthenticated context in the comment.
.github/workflows/api_tests.yml (4)
1-11: LGTM!Workflow triggers and job configuration are appropriate for API testing. The 15-minute timeout provides adequate buffer while preventing hung workflows.
13-31: LGTM!Setup steps are well-configured with appropriate version pinning and minimal test dependencies.
65-69: LGTM!Test execution step is properly configured with appropriate timeout and output settings.
71-86: LGTM!Failure logging and cleanup steps are well-designed. The
if: always()ensures cleanup runs regardless of test outcome, and the-vflag properly removes volumes.tests/api_tests/test_indexer.py (4)
7-20: LGTM!Test correctly validates the supported types endpoint and checks for common file extensions.
26-39: LGTM!Test properly handles multiple success status codes appropriate for asynchronous upload operations and validates task tracking information in the response.
41-52: LGTM!Test appropriately validates markdown file upload with proper status code expectations.
54-66: LGTM!Test correctly validates file upload with custom metadata.
tests/api_tests/test_openai_compat.py (1)
12-20: LGTM!Test appropriately handles both enabled (200) and disabled (404) states, with proper validation when the endpoint is available.
tests/api_tests/conftest.py (3)
22-35: LGTM! Health check logic is sound.The wait-for-API fixture correctly uses a direct httpx client rather than the session-scoped
api_clientfixture, since it needs to run before the session starts. The retry logic with 60 attempts × 2s intervals provides adequate time (120s) for services to start.
86-96: Cleanup exception handling is acceptable for test fixtures.The broad exception catch in cleanup (Lines 93-96) was flagged by static analysis. While generally not ideal, this pattern is acceptable in test cleanup code to ensure teardown doesn't fail the test. The fixture prioritizes test isolation over reporting cleanup failures.
38-77: LGTM! Sample file fixtures are well-structured.The text and markdown fixtures provide appropriate test content for validating file upload, indexing, and search functionality.
tests/api_tests/test_search.py (1)
10-45: Fixture correctly handles multiple indexing response formats.The
indexed_partitionfixture properly handles bothtask_status_urlandtask_idresponse formats, with a reasonable fallback sleep. The polling logic with state checks for success/failure provides good coverage.
| def test_create_duplicate_partition_fails(self, api_client, test_partition_name): | ||
| """Test creating duplicate partition returns error.""" | ||
| api_client.post(f"/partition/{test_partition_name}") | ||
| response = api_client.post(f"/partition/{test_partition_name}") | ||
| assert response.status_code in [400, 409] | ||
|
|
||
| # Cleanup | ||
| api_client.delete(f"/partition/{test_partition_name}") |
There was a problem hiding this comment.
Assert first creation succeeds and use fixture for cleanup.
The test doesn't verify that the first partition creation succeeded (Line 30), which could lead to false positives if both calls fail. Also, manual cleanup could be skipped if assertions fail.
🔧 Proposed fix
def test_create_duplicate_partition_fails(self, api_client, test_partition_name):
"""Test creating duplicate partition returns error."""
- api_client.post(f"/partition/{test_partition_name}")
+ response = api_client.post(f"/partition/{test_partition_name}")
+ assert response.status_code in [200, 201], "First partition creation should succeed"
+
response = api_client.post(f"/partition/{test_partition_name}")
assert response.status_code in [400, 409]
-
- # Cleanup
- api_client.delete(f"/partition/{test_partition_name}")
+
+ # Cleanup (or use created_partition fixture)
+ try:
+ api_client.delete(f"/partition/{test_partition_name}")
+ except Exception:
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_create_duplicate_partition_fails(self, api_client, test_partition_name): | |
| """Test creating duplicate partition returns error.""" | |
| api_client.post(f"/partition/{test_partition_name}") | |
| response = api_client.post(f"/partition/{test_partition_name}") | |
| assert response.status_code in [400, 409] | |
| # Cleanup | |
| api_client.delete(f"/partition/{test_partition_name}") | |
| def test_create_duplicate_partition_fails(self, api_client, test_partition_name): | |
| """Test creating duplicate partition returns error.""" | |
| response = api_client.post(f"/partition/{test_partition_name}") | |
| assert response.status_code in [200, 201], "First partition creation should succeed" | |
| response = api_client.post(f"/partition/{test_partition_name}") | |
| assert response.status_code in [400, 409] | |
| # Cleanup (or use created_partition fixture) | |
| try: | |
| api_client.delete(f"/partition/{test_partition_name}") | |
| except Exception: | |
| pass |
🤖 Prompt for AI Agents
In @tests/api_tests/test_partition.py around lines 28 - 35, Update
test_create_duplicate_partition_fails to explicitly assert the first
api_client.post(f"/partition/{test_partition_name}") returned a success status
(e.g., 200 or 201) before making the second POST so a failing first call can't
mask failures; remove the manual cleanup delete and rely on the existing test
fixture for teardown (or convert the test to use the fixture that guarantees
partition cleanup), keeping the duplicate-create assertion that
response.status_code is in [400, 409].
| def test_search_multiple_partitions(self, api_client, indexed_partition): | ||
| """Test searching across partitions.""" | ||
| response = api_client.get( | ||
| "/search", | ||
| params={"text": "machine learning", "top_k": 5} | ||
| ) | ||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert "documents" in data |
There was a problem hiding this comment.
Bug: indexed_partition parameter is unused.
The test_search_multiple_partitions test receives indexed_partition but never uses it. This means the test searches across partitions without ensuring any partition is actually indexed, potentially causing false positives if the test passes despite indexing failures.
🐛 Proposed fix
def test_search_multiple_partitions(self, api_client, indexed_partition):
"""Test searching across partitions."""
+ # Ensure partition is indexed before searching
+ assert indexed_partition is not None
+
response = api_client.get(
"/search",
- params={"text": "machine learning", "top_k": 5}
+ params={
+ "text": "machine learning",
+ "top_k": 5,
+ "partitions": [indexed_partition]
+ }
)
assert response.status_code == 200
data = response.json()
assert "documents" in dataNote: Verify the correct parameter name for partition filtering in the /search endpoint.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
57-57: Unused method argument: indexed_partition
(ARG002)
🤖 Prompt for AI Agents
In @tests/api_tests/test_search.py around lines 57 - 65, The
test_search_multiple_partitions test currently ignores the indexed_partition
fixture; update the API call in test_search_multiple_partitions to pass the
partition filter using the indexed_partition value (e.g., add the appropriate
partition parameter to the params dict sent to api_client.get for "/search") so
the search is constrained to the partition known to be indexed; ensure you use
the correct query key expected by the endpoint (e.g., "partition" or
"partitions") when adding indexed_partition to the params.
- Remove `|| true` from mock-vllm wait step to fail explicitly on timeout - Change status code assertions from [404, 500] to 404 only for non-existent resources (extract, task, user, partition) - Remove 500 from acceptable codes in OpenAI compat tests - Remove 200 from invalid model test (should return error) - Add assertion that first partition creation succeeds before testing duplicate rejection Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/api_tests/test_openai_compat.py:
- Around line 22-29: Update the assertion in test_chat_completions_endpoint to
accept 500 (and 200) like the completions test: modify the accepted status codes
list in the test_chat_completions_endpoint function from [400, 404, 422] to
[200, 400, 404, 422, 500] so it matches the error handling behavior
(HTTPException(status_code=500)) implemented in the chat/completions handlers
and mirrors test_completions_endpoint.
In @tests/api_tests/test_partition.py:
- Around line 39-43: The test_delete_partition function currently posts to
create a partition but doesn't assert the creation succeeded; update the test to
verify the POST to api_client.post(f"/partition/{test_partition_name}") returned
a successful status (e.g., 200 or 201) and optionally validate any returned body
before calling api_client.delete, then assert the delete response status_code in
[200, 204]; reference the test function name test_delete_partition and the
api_client.post / api_client.delete calls to locate and modify the code.
In @tests/api_tests/test_users.py:
- Around line 23-30: In test_get_user_by_id, assert the prerequisite /users/info
call succeeded and returned an id before using it: check
info_response.status_code == 200 and that "id" exists in info_response.json()
(do not use the fallback default 1), then set user_id from the JSON and proceed
to call api_client.get(f"/users/{user_id}") so failures in the prerequisite
request fail the test clearly; update the assertions in the test_get_user_by_id
function accordingly.
🧹 Nitpick comments (1)
tests/api_tests/test_extract.py (1)
13-17:test_extract_invalid_id_formatdoesn’t send an ID (it’s “missing param”, not “invalid format”).
GET /extract/is testing the collection path (or a missing path param), not an invalid ID. If you want “invalid format”, pass a clearly invalid ID (e.g.,not-a-uuid) and assert the contract-accurate code (commonly 422 for FastAPI UUID path params). Otherwise, rename the test to reflect what it’s actually verifying.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/api_tests.ymltests/api_tests/test_extract.pytests/api_tests/test_indexer.pytests/api_tests/test_openai_compat.pytests/api_tests/test_partition.pytests/api_tests/test_users.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/api_tests/test_indexer.py
- .github/workflows/api_tests.yml
🧰 Additional context used
🧬 Code graph analysis (4)
tests/api_tests/test_partition.py (1)
tests/api_tests/conftest.py (3)
api_client(16-19)test_partition_name(81-83)created_partition(87-96)
tests/api_tests/test_extract.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_openai_compat.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
tests/api_tests/test_users.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-tests
- GitHub Check: index-backup-restore
🔇 Additional comments (12)
tests/api_tests/test_extract.py (1)
8-11: The test is correctly written and does not require modification.The
/extract/{extract_id}route accepts a plain string parameter (extract_id: str) with no validation constraints. There is no UUID type annotation or format validation that would cause"nonexistent-id-12345"to return a 422 error. The vectordb simply performs string matching on the ID and returnsNonefor nonexistent chunks, which the handler correctly converts to a 404 response. The test will work as intended.Likely an incorrect or invalid review comment.
tests/api_tests/test_users.py (3)
8-13: LGTM!The test correctly validates the current user endpoint and checks for the required "id" field in the response.
15-21: LGTM!The test properly validates the users list endpoint and includes a helpful comment about authentication behavior.
32-35: LGTM!The test properly validates the error handling for non-existent users.
tests/api_tests/test_openai_compat.py (3)
12-20: LGTM!The test correctly handles both the enabled and disabled states of the models endpoint, with proper validation of the response structure when available.
31-41: Same status code inconsistency astest_chat_completions_endpoint.This test also excludes
500whiletest_completions_endpointaccepts it. See the comment on Lines 22-29.
43-50: Status code handling looks reasonable.This test appropriately accepts a range of status codes including
500, which is reasonable for LLM endpoints that may encounter runtime errors. The chat completions endpoints should likely be aligned with this approach.tests/api_tests/test_partition.py (5)
8-13: LGTM!The test correctly validates the partition listing endpoint.
15-26: LGTM!The test properly validates partition creation and cleanup. The manual cleanup is appropriate here since the test explicitly validates the creation flow.
28-37: LGTM!The test properly validates duplicate partition rejection with a clear assertion (Line 31) that the initial creation succeeded, as mentioned in the PR objectives.
45-51: LGTM!The test correctly validates listing files in an empty partition and appropriately uses the
created_partitionfixture for setup and cleanup.
53-56: LGTM!The test properly validates error handling for deleting a non-existent partition.
| def test_chat_completions_endpoint(self, api_client): | ||
| """Test chat completions endpoint exists or is disabled.""" | ||
| response = api_client.post( | ||
| "/v1/chat/completions", | ||
| json={"model": "openrag-all", "messages": []} | ||
| ) | ||
| # 404 if endpoint disabled, 400/422 if enabled but invalid input | ||
| assert response.status_code in [400, 404, 422] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the test file
find . -name "test_openai_compat.py" -type fRepository: linagora/openrag
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Get line count to understand file size
wc -l tests/api_tests/test_openai_compat.py 2>/dev/null || find . -name "test_openai_compat.py" -type f -exec wc -l {} \;Repository: linagora/openrag
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Read the test file around the mentioned lines
cat -n tests/api_tests/test_openai_compat.py | head -60Repository: linagora/openrag
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Search for chat completions implementation
rg -n "chat.*completions|/v1/chat" --type=py -A 5 -B 2Repository: linagora/openrag
Length of output: 5824
🏁 Script executed:
#!/bin/bash
# Search for completions endpoint implementation to understand error handling
rg -n "def.*completions|@.*route.*completions" --type=py -A 10Repository: linagora/openrag
Length of output: 4848
🏁 Script executed:
#!/bin/bash
# Read the chat completions endpoint implementation
cat -n openrag/routers/openai.py | head -200 | tail -100Repository: linagora/openrag
Length of output: 4102
🏁 Script executed:
#!/bin/bash
# Find and read the completions endpoint implementation
rg -n "^def.*completions|^@router.post.*completions" openrag/routers/openai.py -A 30Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for any error handling or 500 status code raising in the endpoints
rg -n "500|Internal|raise.*Exception|except" openrag/routers/openai.py -B 2 -A 2Repository: linagora/openrag
Length of output: 3007
🏁 Script executed:
#!/bin/bash
# Look for the full structure of both endpoint handlers
ast-grep --pattern 'async def $_($$$) {
$$$
log = logger.bind($_)
$$$
}'Repository: linagora/openrag
Length of output: 42
Include 500 status code in accepted responses to match the completions endpoint.
Both test_chat_completions_endpoint and test_completions_endpoint use identical error handling patterns that can raise 500 on any exception during request processing. The chat completions endpoint implementation (lines 187–192 and 225–230) explicitly raises HTTPException(status_code=500) for runtime failures, matching the completions endpoint behavior (lines 302–307 and 317–322). Update line 29 to accept [200, 400, 404, 422, 500] for consistency and to prevent test failures from legitimate server errors.
🤖 Prompt for AI Agents
In @tests/api_tests/test_openai_compat.py around lines 22 - 29, Update the
assertion in test_chat_completions_endpoint to accept 500 (and 200) like the
completions test: modify the accepted status codes list in the
test_chat_completions_endpoint function from [400, 404, 422] to [200, 400, 404,
422, 500] so it matches the error handling behavior
(HTTPException(status_code=500)) implemented in the chat/completions handlers
and mirrors test_completions_endpoint.
| def test_delete_partition(self, api_client, test_partition_name): | ||
| """Test deleting a partition.""" | ||
| api_client.post(f"/partition/{test_partition_name}") | ||
| response = api_client.delete(f"/partition/{test_partition_name}") | ||
| assert response.status_code in [200, 204] |
There was a problem hiding this comment.
Add assertion for partition creation.
The test deletes a partition without verifying that creation succeeded (Line 41). If creation fails, the test becomes ambiguous—it would actually be testing deletion of a non-existent partition rather than successful deletion.
✨ Proposed fix
def test_delete_partition(self, api_client, test_partition_name):
"""Test deleting a partition."""
- api_client.post(f"/partition/{test_partition_name}")
+ create_response = api_client.post(f"/partition/{test_partition_name}")
+ assert create_response.status_code in [200, 201], "Failed to create partition for deletion test"
response = api_client.delete(f"/partition/{test_partition_name}")
assert response.status_code in [200, 204]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_delete_partition(self, api_client, test_partition_name): | |
| """Test deleting a partition.""" | |
| api_client.post(f"/partition/{test_partition_name}") | |
| response = api_client.delete(f"/partition/{test_partition_name}") | |
| assert response.status_code in [200, 204] | |
| def test_delete_partition(self, api_client, test_partition_name): | |
| """Test deleting a partition.""" | |
| create_response = api_client.post(f"/partition/{test_partition_name}") | |
| assert create_response.status_code in [200, 201], "Failed to create partition for deletion test" | |
| response = api_client.delete(f"/partition/{test_partition_name}") | |
| assert response.status_code in [200, 204] |
🤖 Prompt for AI Agents
In @tests/api_tests/test_partition.py around lines 39 - 43, The
test_delete_partition function currently posts to create a partition but doesn't
assert the creation succeeded; update the test to verify the POST to
api_client.post(f"/partition/{test_partition_name}") returned a successful
status (e.g., 200 or 201) and optionally validate any returned body before
calling api_client.delete, then assert the delete response status_code in [200,
204]; reference the test function name test_delete_partition and the
api_client.post / api_client.delete calls to locate and modify the code.
| def test_get_user_by_id(self, api_client): | ||
| """Test getting user by ID.""" | ||
| # First get current user to know a valid ID | ||
| info_response = api_client.get("/users/info") | ||
| user_id = info_response.json().get("id", 1) | ||
|
|
||
| response = api_client.get(f"/users/{user_id}") | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
Add assertion for the prerequisite API call.
The test fetches the current user ID without verifying the prerequisite request succeeded. If /users/info fails or returns data without an "id" field, the fallback value 1 masks the problem, potentially leading to false positives or confusing test failures.
✨ Proposed fix
def test_get_user_by_id(self, api_client):
"""Test getting user by ID."""
# First get current user to know a valid ID
info_response = api_client.get("/users/info")
+ assert info_response.status_code == 200
- user_id = info_response.json().get("id", 1)
+ user_id = info_response.json()["id"]
response = api_client.get(f"/users/{user_id}")
assert response.status_code == 200🤖 Prompt for AI Agents
In @tests/api_tests/test_users.py around lines 23 - 30, In test_get_user_by_id,
assert the prerequisite /users/info call succeeded and returned an id before
using it: check info_response.status_code == 200 and that "id" exists in
info_response.json() (do not use the fallback default 1), then set user_id from
the JSON and proceed to call api_client.get(f"/users/{user_id}") so failures in
the prerequisite request fail the test clearly; update the assertions in the
test_get_user_by_id function accordingly.
- Fix extract router to re-raise HTTPException before generic handler - Fix get_chunk_by_id to handle Milvus Int64 _id field properly: - Convert string chunk_id to integer before querying - Return None for invalid ID formats instead of raising exception Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openrag/components/indexer/vectordb/vectordb.py (1)
727-727: Consider using parameterized queries for consistency.While the f-string filter is safe due to integer validation, using parameterized queries would align with the pattern used in
get_file_chunks(lines 660-661):♻️ Suggested refactor for consistency
response = await self._async_client.query( collection_name=self.collection_name, - filter=f"_id == {chunk_id_int}", + filter="_id == {chunk_id}", + filter_params={"chunk_id": chunk_id_int}, limit=1, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openrag/components/indexer/vectordb/vectordb.pyopenrag/routers/extract.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-tests
- GitHub Check: index-backup-restore
🔇 Additional comments (3)
openrag/routers/extract.py (1)
69-70: LGTM! Critical fix for proper HTTP error codes.This correctly preserves the intended 404 and 403 responses raised earlier in the function instead of converting them to 500 errors via the generic exception handler.
openrag/components/indexer/vectordb/vectordb.py (2)
712-714: LGTM: Clear documentation of Int64 handling and None return cases.The docstring updates accurately reflect the Milvus Int64 _id requirements and all possible return scenarios.
717-722: LGTM: Robust input validation prevents invalid Milvus queries.The string-to-integer conversion with graceful error handling correctly prevents invalid _id queries and maintains consistency with the "not found" return behavior.
The CI workflow checks mock-vllm health from the host via curl, but the port wasn't exposed. Add port mapping 8000:8000. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/api_tests/docker-compose.yaml (1)
50-50: Update MinIO to a more recent release.The MinIO version (RELEASE.2023-03-20T20-16-18Z) is from March 2023 and is over 2.5 years old. While this release contains important security patches, consider updating to a more recent version such as RELEASE.2025-10-15T17-29-55Z (current as of early 2026) to benefit from additional security patches and improvements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/api_tests/docker-compose.yaml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/api_tests/docker-compose.yaml
4-4: "jobs" section is missing in workflow
(syntax-check)
4-4: "on" section is missing in workflow
(syntax-check)
4-4: unexpected key "services" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
128-128: unexpected key "volumes" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-tests
- GitHub Check: index-backup-restore
🔇 Additional comments (2)
.github/workflows/api_tests/docker-compose.yaml (2)
109-109: No action needed—thePROMPTS_DIRpath is correct.The relative path
../prompts/example1resolves correctly within the container. The Dockerfile copiesprompts/to/app/prompts/, and since the container's working directory is/app/openrag, the relative path../prompts/example1correctly resolves to/app/prompts/example1. The directory structure is properly set up during the Docker build process.Likely an incorrect or invalid review comment.
94-94: No action needed — these environment variables are not used by the OpenRAG service.The
BASE_URLandVLM_BASE_URLvariables set in the docker-compose.yaml are only consumed by the evaluation-pipeline scripts (automatic-evaluation-pipeline/generate_questions.pyandbenchmark.py), not by the main OpenRAG API service. The OpenRAG service itself does not reference these environment variables, so setting them tolocalhostURLs does not cause connection failures. The API service correctly listens on port 8080 as defined in the docker-compose configuration.Likely an incorrect or invalid review comment.
|
Next actions:
|
Ahmath-Gadji
added
chore
2 participants
Summary
Adds a new GitHub Actions workflow that tests the OpenRAG API without requiring VLLM or heavy ML dependencies. This provides much faster CI feedback compared to the existing smoke tests.
Performance comparison:
Key components
mock_vllm.py): Lightweight FastAPI server that returns fake embeddings, allowing full API testing without actual ML model loadingFiles added
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.