OpenAI Threads: Fixing testcases #324
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors tests in backend/app/tests/api/routes/test_threads.py to use a shared client fixture with header-based auth, replace OpenAI patches with configure_openai/get_provider_credential mocks, add mocks for send_callback/process_run, switch endpoints to /api/v1, expand/rename tests, add an active-run /threads/sync case, and validate diagnostics and 422 errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (pytest)
participant Client as Test Client
participant API as Threads API (/api/v1)
participant Svc as Threads Service
participant Prov as Provider Config
participant CB as Callback
Test->>Client: POST /threads/start (headers auth)
Client->>API: Request
API->>Prov: configure_openai/get_provider_credential
API->>Svc: process_run / poll_run
Svc-->>API: Run ID / status
API-->>Client: 200 with thread/run info
Test->>Client: POST /threads/sync
Client->>API: Request
API->>Svc: Check active run
alt Active run exists
API-->>Client: 400/409 error (active run)
else No active run
API->>Svc: process + diagnostics
Svc-->>API: message + token usage + model
API-->>Client: 200 with diagnostics
end
Test->>Client: GET /threads/result
Client->>API: Request
API->>Svc: Fetch run status/result
API-->>Client: 200/202/404 accordingly
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_threads.py(13 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_threads.py
17-17: app.models.APIKey imported but unused
Remove unused import
(F401)
17-17: app.models.APIKeyPublic imported but unused
Remove unused import
(F401)
30-30: Unused function argument: db
(ARG001)
129-129: Unused function argument: db
(ARG001)
173-173: Unused function argument: db
(ARG001)
460-460: Unused function argument: db
(ARG001)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (12)
backend/app/tests/api/routes/test_threads.py (12)
59-59: Good move to standardized auth header fixture.Using user_api_key_header simplifies setup and removes DB coupling from the test.
161-163: LGTM — consistent use of user_api_key_header.This standardizes auth setup across tests.
190-192: LGTM — header fixture applied.Keeps the test focused on behavior, not setup.
471-471: LGTM — using user_api_key_header.Auth setup is consistent and concise.
480-480: LGTM — inject header fixture while retaining db.This test persists/reads from DB and now authenticates via the fixture; correct balance.
489-489: LGTM — client.get includes user_api_key_header.Matches the new pattern.
499-499: LGTM — db retained, header fixture added.Appropriate for a DB-backed test with authenticated endpoint.
507-507: LGTM — standardized header usage.Consistent with the rest of the suite.
517-517: LGTM — remove unnecessary db fixture.This test doesn’t touch DB; using only user_api_key_header is correct.
519-521: LGTM — authenticated request via fixture.Consistent and clear.
529-529: LGTM — remove db from signature.Test does not require DB; just mocks OpenAI and checks validation.
535-535: LGTM — posting with user_api_key_header.Keeps auth handling uniform.
|
|
||
| @patch("app.api.routes.threads.OpenAI") | ||
| def test_threads_endpoint(mock_openai, db): | ||
| def test_threads_endpoint(mock_openai, db, user_api_key_header): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop unused db fixture (ARG001).
This test doesn’t use the db fixture. Removing it reduces noise and satisfies lint.
-@patch("app.api.routes.threads.OpenAI")
-def test_threads_endpoint(mock_openai, db, user_api_key_header):
+@patch("app.api.routes.threads.OpenAI")
+def test_threads_endpoint(mock_openai, user_api_key_header):📝 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_threads_endpoint(mock_openai, db, user_api_key_header): | |
| @patch("app.api.routes.threads.OpenAI") | |
| def test_threads_endpoint(mock_openai, user_api_key_header): | |
| ... |
🧰 Tools
🪛 Ruff (0.12.2)
30-30: Unused function argument: db
(ARG001)
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_threads.py at line 30, the
test_threads_endpoint function includes an unused db fixture argument. Remove
the db parameter from the function signature to eliminate unused code and
satisfy linting rules.
|
|
||
| @patch("app.api.routes.threads.OpenAI") | ||
| def test_threads_sync_endpoint_active_run(mock_openai, db): | ||
| def test_threads_sync_endpoint_active_run(mock_openai, db, user_api_key_header): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused db fixture (ARG001).
No db usage here; safe to drop.
-@patch("app.api.routes.threads.OpenAI")
-def test_threads_sync_endpoint_active_run(mock_openai, db, user_api_key_header):
+@patch("app.api.routes.threads.OpenAI")
+def test_threads_sync_endpoint_active_run(mock_openai, user_api_key_header):📝 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_threads_sync_endpoint_active_run(mock_openai, db, user_api_key_header): | |
| @patch("app.api.routes.threads.OpenAI") | |
| def test_threads_sync_endpoint_active_run(mock_openai, user_api_key_header): | |
| ... |
🧰 Tools
🪛 Ruff (0.12.2)
173-173: Unused function argument: db
(ARG001)
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_threads.py at line 173, the test function
test_threads_sync_endpoint_active_run includes an unused db fixture argument.
Remove the db parameter from the function signature since it is not used in the
test, to clean up the code and avoid the ARG001 warning.
|
|
||
| @patch("app.api.routes.threads.OpenAI") | ||
| def test_threads_start_endpoint_creates_thread(mock_openai, db): | ||
| def test_threads_start_endpoint_creates_thread(mock_openai, db, user_api_key_header): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused db fixture (ARG001).
The test doesn’t touch the database.
-@patch("app.api.routes.threads.OpenAI")
-def test_threads_start_endpoint_creates_thread(mock_openai, db, user_api_key_header):
+@patch("app.api.routes.threads.OpenAI")
+def test_threads_start_endpoint_creates_thread(mock_openai, user_api_key_header):📝 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_threads_start_endpoint_creates_thread(mock_openai, db, user_api_key_header): | |
| @patch("app.api.routes.threads.OpenAI") | |
| def test_threads_start_endpoint_creates_thread(mock_openai, user_api_key_header): | |
| # existing test body... |
🧰 Tools
🪛 Ruff (0.12.2)
460-460: Unused function argument: db
(ARG001)
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_threads.py at line 460, the test function
test_threads_start_endpoint_creates_thread includes an unused db fixture
argument. Remove the db parameter from the function signature since the test
does not interact with the database, eliminating the unused argument warning.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_threads.py (1)
17-17: Remove unused imports & fixture arguments (lint keeps failing)
APIKey,APIKeyPublicand thedbfixture are still unused across these tests.
Please drop them to silence Ruff F401 / ARG001 warnings.Also applies to: 29-30, 143-144, 356-357, 385-386, 416-417, 446-447
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/threads.py(1 hunks)backend/app/tests/api/routes/test_threads.py(13 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_threads.py
17-17: app.models.APIKey imported but unused
Remove unused import
(F401)
17-17: app.models.APIKeyPublic imported but unused
Remove unused import
(F401)
29-29: Unused function argument: db
(ARG001)
143-143: Unused function argument: db
(ARG001)
446-446: Unused function argument: db
(ARG001)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
| is_valid, error_message = validate_thread(client, request.get("thread_id")) | ||
| if not is_valid: | ||
| raise Exception(error_message) | ||
| return APIResponse.failure_response(error=error_message) | ||
|
|
||
| # Setup thread | ||
| is_success, error_message = setup_thread(client, request) | ||
| if not is_success: | ||
| raise Exception(error_message) | ||
| return APIResponse.failure_response(error=error_message) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return an explicit HTTP error status, not a 200 “success”: false payload
APIResponse.failure_response() is returned directly, so FastAPI will respond with HTTP 200.
Clients and automated tooling generally rely on status codes to distinguish success from validation/setup errors; a silent 200 breaks that contract and is also inconsistent with the asynchronous /threads endpoint which still raises an exception.
Suggestion:
-from app.utils import APIResponse
+from fastapi import HTTPException
...
if not is_valid:
- return APIResponse.failure_response(error=error_message)
+ raise HTTPException(status_code=409, detail=error_message) # 409: conflict / active run
...
if not is_success:
- return APIResponse.failure_response(error=error_message)
+ raise HTTPException(status_code=400, detail=error_message)This preserves structured error info while conveying failure via the HTTP layer and keeps behaviour consistent across endpoints.
🤖 Prompt for AI Agents
In backend/app/api/routes/threads.py around lines 393 to 401, the failure
responses use APIResponse.failure_response() which returns HTTP 200 with a
failure payload, causing clients to misinterpret errors. Modify these failure
responses to raise HTTP exceptions with appropriate error status codes (e.g.,
400 or 422) using FastAPI's HTTPException, including the error message in the
detail field. This ensures the HTTP status code correctly indicates failure and
aligns behavior with the asynchronous /threads endpoint.
There was a problem hiding this comment.
@AkhileshNegi can you take care of this and raise proper error with appropriate status code. refrence
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/app/tests/api/routes/test_threads.py (4)
28-30:dbfixture still unused – please drop it
This was already raised in an earlier review; the parameter is still present but never referenced.
142-145: Redundantdbparameter
dbisn’t used inside this test body – remove it to silence ARG001.
174-176: Unuseddbfixture
Same lint warning as before; delete thedbargument.
500-503:dbfixture not required here
The test never touches the database; drop the parameter.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_threads.py (1)
19-20: Consistent exception reference (nitpick)
You importOpenAIErrordirectly but still callopenai.OpenAIErrorelsewhere. Pick one form (prefer the direct import) for clarity.- error = openai.OpenAIError() + error = OpenAIError()Also applies to: 237-238, 443-444
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_threads.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_threads.py
29-29: Unused function argument: db
(ARG001)
143-143: Unused function argument: db
(ARG001)
174-174: Unused function argument: db
(ARG001)
501-501: Unused function argument: db
(ARG001)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
| is_valid, error_message = validate_thread(client, request.get("thread_id")) | ||
| if not is_valid: | ||
| raise Exception(error_message) | ||
| return APIResponse.failure_response(error=error_message) | ||
|
|
||
| # Setup thread | ||
| is_success, error_message = setup_thread(client, request) | ||
| if not is_success: | ||
| raise Exception(error_message) | ||
| return APIResponse.failure_response(error=error_message) | ||
|
|
There was a problem hiding this comment.
@AkhileshNegi can you take care of this and raise proper error with appropriate status code. refrence
|
@AkhileshNegi Changes looks fine there is one test failing, please fix that. |
* using API key fixture * running the skipped testcases * minor changes * following PEP8 * added testcase * minor cleanups * reverting some changes * assert exception
3 participants
Summary
Target issue is #229
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Summary by CodeRabbit