Response API: Fixing testcases #325
Conversation
WalkthroughTests for POST /api/v1/responses were refactored to centralized mock helpers and now assert that a background task Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.6)
participant Test as Test Suite
participant API as POST /api/v1/responses
participant Scheduler as Background Scheduler
participant Worker as process_response
end
Test->>API: POST /api/v1/responses
API->>Scheduler: schedule Worker(ResponsesAPIRequest)
Scheduler->>Worker: invoke Worker (mocked in tests)
Worker-->>Scheduler: complete
API-->>Test: HTTP 200 / accepted
sequenceDiagram
rect rgba(240,255,220,0.6)
participant Worker as process_response
participant DB as Conversation Store
participant OAI as OpenAI Client
participant CB as Callback Sender
end
Worker->>DB: lookup ancestor conversation (if provided)
alt Ancestor found
Worker->>OAI: generate response using ancestor.response_id as previous_response_id
else Ancestor not found
Worker->>OAI: generate response using request.response_id or none
end
Worker->>DB: create/update conversation & response records
Worker->>CB: POST callback to request.callback_url with payload
Worker-->>Caller: return / side-effects complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. ✨ 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 (
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
♻️ Duplicate comments (5)
backend/app/tests/api/routes/test_responses.py (5)
396-402: See earlier note on positional vs keyword assertions – same recommendation applies.
493-499: Same positional-arg caveat; please switch to kw-arg based check or helper.
587-593: Same positional-arg caveat; consider helper.
678-684: Same positional-arg caveat; consider helper.
787-874: Logic mirrors the previous test; same DRY & DB-skip concerns apply.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
204-210: Duplicate assertion logic – extract helperThe same positional-based assertions appear in multiple tests (Lines 115-122, 204-210, 396-402, etc.).
Factor them into a small helper, e.g.assert_request_fields(call_args, expected), to cut duplication and keep tests concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(16 hunks)
⏰ 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 (8)
backend/app/tests/api/routes/test_responses.py (8)
7-8: No concerns for the added import – looks correct.
16-24: Decorator stack order & injected mock parameters line-up correctly – nothing to fix here.
38-40: Mocking the background task to prevent execution is sound.
145-147: Background task correctly stubbed.
323-324: Nothing to flag – simple stubbing.
427-428: OK.
524-525: OK.
618-619: OK.
| # Verify that the background task was scheduled with correct parameters | ||
| mock_process_response.assert_called_once() | ||
| call_args = mock_process_response.call_args | ||
| assert call_args[0][0].assistant_id == "assistant_dalgo" | ||
| assert call_args[0][0].question == "What is Dalgo?" | ||
| assert call_args[0][0].callback_url == "http://example.com/callback" | ||
| assert call_args[0][0].response_id is None | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer kw-arg based assertion for long-term safety
process_response is asserted via call_args[0][0], assuming the request object is the first positional arg.
If the production code ever switches to keyword invocation (background_tasks.add_task(process_response, request=request, …)), this test will start failing even though behaviour is unchanged. Use call_args.kwargs["request"] (or call_args.kwargs.get("request")) instead.
-assert call_args[0][0].assistant_id == "assistant_dalgo"
+request_arg = call_args.kwargs.get("request") or call_args.args[0]
+assert request_arg.assistant_id == "assistant_dalgo"Run the script below to confirm the current calling style:
🏁 Script executed:
#!/bin/bash
# Search for process_response scheduling
rg -n "add_task\(.*process_response" app/api/routes/responses.pyLength of output: 134
To locate where process_response is scheduled, let’s search the entire repo for any add_task invocation and usages of process_response without assuming the path. Please run:
🏁 Script executed:
#!/bin/bash
# Find any add_task calls that mention process_response
rg -n "add_task.*process_response" -A3
# Locate all references to process_response to see its definition and callers
rg -n "process_response" -A3Length of output: 14544
To confirm how process_response is actually scheduled—positional vs. keyword args—let’s search for the background_tasks.add_task invocation in the routes file:
🏁 Script executed:
#!/bin/bash
# Find where process_response is queued in the responses router
rg -n "background_tasks.add_task" -A5 backend/app/api/routes/responses.pyLength of output: 222
Prefer kw-arg–safe assertions when inspecting the scheduled task
Although process_response is currently invoked with positional args, future changes to keyword invocation would break the existing checks. Let’s extract the request argument from either call_args.kwargs or fallback to call_args.args[0] and then assert against its attributes:
File: backend/app/tests/api/routes/test_responses.py
Lines: 115–122
- # Verify that the background task was scheduled with correct parameters
- mock_process_response.assert_called_once()
- call_args = mock_process_response.call_args
- assert call_args[0][0].assistant_id == "assistant_dalgo"
- assert call_args[0][0].question == "What is Dalgo?"
- assert call_args[0][0].callback_url == "http://example.com/callback"
- assert call_args[0][0].response_id is None
+ # Verify that the background task was scheduled with correct parameters
+ mock_process_response.assert_called_once()
+ call_args = mock_process_response.call_args
+ request_arg = call_args.kwargs.get("request") or call_args.args[0]
+ assert request_arg.assistant_id == "assistant_dalgo"
+ assert request_arg.question == "What is Dalgo?"
+ assert request_arg.callback_url == "http://example.com/callback"
+ assert request_arg.response_id is None📝 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.
| # Verify that the background task was scheduled with correct parameters | |
| mock_process_response.assert_called_once() | |
| call_args = mock_process_response.call_args | |
| assert call_args[0][0].assistant_id == "assistant_dalgo" | |
| assert call_args[0][0].question == "What is Dalgo?" | |
| assert call_args[0][0].callback_url == "http://example.com/callback" | |
| assert call_args[0][0].response_id is None | |
| # Verify that the background task was scheduled with correct parameters | |
| mock_process_response.assert_called_once() | |
| call_args = mock_process_response.call_args | |
| request_arg = call_args.kwargs.get("request") or call_args.args[0] | |
| assert request_arg.assistant_id == "assistant_dalgo" | |
| assert request_arg.question == "What is Dalgo?" | |
| assert request_arg.callback_url == "http://example.com/callback" | |
| assert request_arg.response_id is None |
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_responses.py around lines 115 to 122, the
test asserts the scheduled task's arguments using positional indexing, which can
break if the function is later called with keyword arguments. Modify the code to
first try extracting the request argument from call_args.kwargs by key, and if
not present, fallback to call_args.args[0]. Then perform assertions on the
extracted request object's attributes to ensure the test is robust against
changes in argument passing style.
| response_id="resp_ancestor1234567890abcdef1234567890", | ||
| ) | ||
|
|
||
| # Setup mock assistant |
There was a problem hiding this comment.
make functions for mock setup can be made and re-used that way?
There was a problem hiding this comment.
made requested changes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_responses.py (1)
178-184: Duplicate comment: Use kw-arg-safe assertion for scheduled task parametersThis is the same issue flagged in the previous review. The current code assumes positional argument access (
call_args[0][0]) which could break if the function call switches to keyword arguments.
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_responses.py (1)
144-146: Consider removing unused fixture parametersStatic analysis indicates several test functions have unused
dbanduser_api_keyparameters. These may have been artifacts from refactoring when switching from direct execution to background task testing.Since these tests now mock
process_responseand don't execute the actual background logic, consider removing the unused parameters:def test_responses_endpoint_success( mock_get_conversation_by_ancestor_id, mock_create_conversation, mock_get_ancestor_id_from_response, mock_tracer_class, mock_get_assistant, mock_get_credential, mock_openai, mock_process_response, - db, user_api_key_header: dict[str, str], - user_api_key, ):Apply similar changes to the other affected test functions.
Also applies to: 204-206, 344-346, 404-406, 465-467, 526-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(15 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/api/routes/responses.py (5)
responses(286-360)process_response(127-282)ResponsesAPIRequest(46-53)process_response(127-293)responses(297-376)backend/app/tests/conftest.py (4)
client(46-49)db(18-35)user_api_key_header(71-73)user_api_key(83-85)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
144-144: Unused function argument: db
(ARG001)
146-146: Unused function argument: user_api_key
(ARG001)
204-204: Unused function argument: db
(ARG001)
206-206: Unused function argument: user_api_key
(ARG001)
344-344: Unused function argument: db
(ARG001)
346-346: Unused function argument: user_api_key
(ARG001)
404-404: Unused function argument: db
(ARG001)
406-406: Unused function argument: user_api_key
(ARG001)
465-465: Unused function argument: db
(ARG001)
467-467: Unused function argument: user_api_key
(ARG001)
526-526: Unused function argument: db
(ARG001)
528-528: Unused function argument: user_api_key
(ARG001)
🔇 Additional comments (14)
backend/app/tests/api/routes/test_responses.py (14)
4-4: Good: Import added for direct testing ofprocess_responseThe import of
process_responseenables unit testing of the background processing function directly, which provides better test coverage and isolation.
13-24: Good: Helper function reduces test setup duplicationThe
create_mock_assistantfunction centralizes mock assistant creation with sensible defaults, improving test maintainability.
27-47: Good: Comprehensive mock OpenAI response factoryThe function covers all necessary response attributes including
usage,output, andprevious_response_idfields, providing flexible test data creation.
50-64: Good: Mock file search results helperCreates realistic file search result structures with scores and text content, enabling comprehensive testing of file search functionality.
67-75: Good: Mock conversation factory with key attributesProvides both
response_idandancestor_response_idfields needed for conversation chain testing.
78-124: Excellent: Centralized mock setup reduces duplicationThe
setup_common_mocksfunction significantly reduces test boilerplate by centralizing the setup of common dependencies. The parameterization options (assistant_model, vector_store_ids, conversation_found, response_output) provide good flexibility for different test scenarios.
127-151: Good: Comprehensive patching for endpoint isolationThe test properly patches all external dependencies including
process_response, ensuring the endpoint test focuses only on request validation and background task scheduling.
187-244: Good: Test coverage for different assistant configurationsThe test properly validates behavior when the assistant has no vector store, using the
setup_common_mockswithvector_store_ids=[]to simulate this scenario.
278-324: Good: Comprehensive credential validation testingBoth tests properly validate the different credential failure scenarios (missing credentials vs missing api_key in credentials), ensuring robust error handling.
327-384: Good: File search results testingThe test validates that the endpoint works correctly when file search results are present in the response output, using the
create_mock_file_search_results()helper.
387-506: Good: Comprehensive ancestor conversation testingBoth tests (conversation found vs not found) properly validate the endpoint behavior in different conversation chain scenarios, ensuring the
response_idparameter is handled correctly.
509-566: Good: Complete test scenario coverageThe test validates the endpoint behavior when no
response_idis provided, ensuring the parameter is correctly set toNonein the scheduled task.
569-647: Good: Direct unit testing of background processing logicThis test properly validates the
process_responsefunction's behavior when an ancestor conversation is found, including:
- Correct database query with proper parameters
- OpenAI API call with the found conversation's response_id as previous_response_id
- Proper conversation creation and callback execution
The test isolation is excellent with focused mocking.
650-727: Good: Complete coverage of ancestor conversation scenariosThis test validates the alternate path when no ancestor conversation is found, ensuring the original
response_idis used asprevious_response_idin the OpenAI API call.
There was a problem hiding this comment.
It looks like there’s a lot of mocking happening in these tests.
possibly more than we actually need, which makes the tests harder to follow and maintain.
I think we could simplify this by:
- Only mocking the dependencies that needed.
- Can you check if we can group the patch or use some type of helper fixture for mocking or anyother way to simplify this test cases?
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
backend/app/tests/api/routes/test_responses.py (5)
159-166: Extract scheduled request via kwargs for forward-compatibilityIndexing
call_args[0][0]assumes positional invocation. If the production code switches to keyword args, the test will fail despite correct behavior. Use kwargs-first with positional fallback.- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
217-223: Extract scheduled request via kwargs (same issue as above)Repeat the kwargs-first extraction here.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_123" - assert call_args[0][0].question == "What is Glific?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_123" + assert request_arg.question == "What is Glific?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
357-364: Extract scheduled request via kwargs (same issue as above)Make the assertion resilient to kwarg invocation.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"
419-424: Extract scheduled request via kwargs (same issue as above)Make this robust against changes in argument passing style.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"
479-485: Extract scheduled request via kwargs (same issue as above)Same fix here.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
🧹 Nitpick comments (5)
backend/app/tests/api/routes/test_responses.py (5)
38-46: Initializeusageexplicitly to avoid implicit MagicMock chainsBe explicit to make the intent clear and avoid surprises if MagicMock behavior changes. This also helps IDE/static tools.
mock_response = MagicMock() mock_response.id = response_id mock_response.output_text = output_text mock_response.model = model -mock_response.usage.input_tokens = 10 -mock_response.usage.output_tokens = 5 -mock_response.usage.total_tokens = 15 +mock_response.usage = MagicMock() +mock_response.usage.input_tokens = 10 +mock_response.usage.output_tokens = 5 +mock_response.usage.total_tokens = 15 mock_response.output = output mock_response.previous_response_id = previous_response_id return mock_response
491-509: ImportResponsesAPIRequestonce at module levelMinor cleanup: import
ResponsesAPIRequestat the top and reuse it, avoiding repeated imports inside tests.-from app.api.routes.responses import ResponsesAPIRequest +# Move to the top of the file: +# from app.api.routes.responses import ResponsesAPIRequestOptionally add at the top with the other imports:
from app.api.routes.responses import ResponsesAPIRequestAlso applies to: 581-590
168-176: Unnecessary patches in endpoint testsEndpoint tests patch
get_ancestor_id_from_response,create_conversation, andget_conversation_by_ancestor_id, but these are only used insideprocess_response—which is already patched out. These extra patches add noise and maintenance overhead.
- Drop the patches for
get_ancestor_id_from_response,create_conversation,get_conversation_by_ancestor_idin endpoint-only tests.- Keep them in the
process_responseunit tests where they are asserted.Also applies to: 305-313, 427-435
191-203: Add a focused unit test for vector-store tools behaviorYou cover the “no vector store configured” endpoint path, but the behavior detail (tools absent/present) is exercised inside
process_response. Consider a unit test forprocess_responsethat assertstoolsare included/excluded based onassistant.vector_store_idsand thatmax_num_resultspropagates.If helpful, I can draft this test case.
2-11: Helper to DRY request extraction from scheduled callTo avoid repetition across tests, add a tiny helper that extracts the scheduled request argument safely.
Add this helper near your other helpers:
def get_scheduled_request_arg(mock_process_response): call_args = mock_process_response.call_args return call_args.kwargs.get("request") or call_args.args[0]Then replace repeated extraction/assert blocks with:
request_arg = get_scheduled_request_arg(mock_process_response)Also applies to: 159-166, 217-223, 357-364, 419-424, 479-485
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (3)
backend/app/api/routes/responses.py (3)
responses(297-376)process_response(127-293)ResponsesAPIRequest(46-53)backend/app/tests/conftest.py (4)
client(46-49)user_api_key_header(71-73)db(18-35)user_api_key(83-85)backend/app/tests/api/routes/test_assistants.py (1)
assistant_id(25-26)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
322-322: Unused function argument: db
(ARG001)
324-324: Unused function argument: user_api_key
(ARG001)
383-383: Unused function argument: db
(ARG001)
385-385: Unused function argument: user_api_key
(ARG001)
444-444: Unused function argument: db
(ARG001)
446-446: Unused function argument: user_api_key
(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 (4)
backend/app/tests/api/routes/test_responses.py (4)
13-24: Good factory for assistants; reduces duplicationThe helper cleanly centralizes assistant defaults and improves readability across tests.
61-108: Solid shared mock setupThe
setup_common_mockshelper meaningfully reduces duplication and keeps individual tests focused on behavior.
487-566: Robust coverage of ancestor-found flow in process_responseGood assertions around ancestor lookup, previous_response_id usage, persistence, and callback invocation. This meaningfully validates the branching logic.
568-646: Robust coverage of ancestor-not-found flow in process_responseStrong verification of the alternative path, including previous_response_id derivation and callback behavior.
…h4DevAI/ai-platform into hotfix/response-api-testcases
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_responses.py (1)
159-166: Use kw-arg–safe extraction when asserting scheduled task argumentsCurrent assertions assume positional invocation (
call_args[0][0]) which will break if the route switches to keyword args. Extract the request from kwargs first, then fallback to args. This was noted earlier; applying it here will make tests resilient.For lines 159–166:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is NoneFor lines 217–224:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_123" - assert call_args[0][0].question == "What is Glific?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_123" + assert request_arg.question == "What is Glific?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is NoneFor lines 353–360:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"For lines 412–419:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"For lines 470–476:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is NoneAlso applies to: 217-224, 353-360, 412-419, 470-476
🧹 Nitpick comments (14)
backend/app/tests/api/routes/test_responses.py (14)
1-10: Prefer the shared TestClient fixture; drop the bespoke FastAPI app and router importBuilding a local app/client bypasses dependency overrides from tests/conftest.py (DB/auth), making these tests brittle. Use the
clientfixture and import only what you actually directly invoke in this file.Apply this diff to remove the local app and narrow the import:
-from fastapi import FastAPI -from fastapi.testclient import TestClient -from app.api.routes.responses import router, process_response +from app.api.routes.responses import process_response @@ -# Wrap the router in a FastAPI app instance -app = FastAPI() -app.include_router(router) -client = TestClient(app)Follow-up: add the
clientfixture parameter to tests that callclient.post(...). See suggestions on individual test signatures below.
118-129: Adopt the shared TestClient fixture in this testNow that the bespoke TestClient is removed, take
clientfrom conftest so dependency overrides apply.def test_responses_endpoint_success( @@ - mock_process_response, - user_api_key_header: dict[str, str], + mock_process_response, + user_api_key_header: dict[str, str], + client, ):
176-187: Adopt the shared TestClient fixture here as wellUse the repo’s
clientfixture.def test_responses_endpoint_without_vector_store( @@ - mock_process_response, - user_api_key_header, + mock_process_response, + user_api_key_header, + client, ):
226-233: Update to use theclientfixture once bespoke client is removedThis test posts to the endpoint and should receive
clientvia fixture rather than the module-level client.Example update:
@patch("app.api.routes.responses.get_assistant_by_id") def test_responses_endpoint_assistant_not_found( mock_get_assistant, user_api_key_header, client, ): ...
247-253: Update to use theclientfixture for consistent overridesSame rationale as above.
Example update:
@patch("app.api.routes.responses.get_provider_credential") @patch("app.api.routes.responses.get_assistant_by_id") def test_responses_endpoint_no_openai_credentials( mock_get_assistant, mock_get_credential, user_api_key_header, client, ): ...
277-285: Update to use theclientfixture here tooAligns with removal of bespoke client and ensures DB/auth overrides are applied.
Example update:
@patch("app.api.routes.responses.get_provider_credential") @patch("app.api.routes.responses.get_assistant_by_id") def test_responses_endpoint_missing_api_key_in_credentials( mock_get_assistant, mock_get_credential, user_api_key_header, client, ): ...
311-321: Adopt the shared TestClient fixture in this testAdd the
clientfixture parameter.def test_responses_endpoint_with_ancestor_conversation_found( @@ - mock_process_response, - user_api_key_header: dict[str, str], + mock_process_response, + user_api_key_header: dict[str, str], + client, ):
370-380: Adopt the shared TestClient fixture in this testAdd the
clientfixture parameter.def test_responses_endpoint_with_ancestor_conversation_not_found( @@ - mock_process_response, - user_api_key_header: dict[str, str], + mock_process_response, + user_api_key_header: dict[str, str], + client, ):
429-439: Adopt the shared TestClient fixture in this testAdd the
clientfixture parameter.def test_responses_endpoint_without_response_id( @@ - mock_process_response, - user_api_key_header: dict[str, str], + mock_process_response, + user_api_key_header: dict[str, str], + client, ):
61-108: Nit: setup_common_mocks returns values that are often unused; consider streamliningIn endpoint tests,
mock_clientandmock_assistantare assigned but unused. Either return only what’s required per caller or assign to_to avoid reader confusion.
556-558: Strengthen callback assertions to validate destination URLAsserting only “called once” can miss regressions where a wrong URL is used. Also check the first argument equals the request callback URL with a kwargs fallback.
# Verify send_callback was called - mock_send_callback.assert_called_once() + mock_send_callback.assert_called_once() + callback_url_arg = mock_send_callback.call_args.kwargs.get("callback_url") or mock_send_callback.call_args.args[0] + assert callback_url_arg == request.callback_url
636-638: Strengthen callback assertions to validate destination URL (second test)Mirror the check from the previous test to ensure both branches validate the URL.
# Verify send_callback was called - mock_send_callback.assert_called_once() + mock_send_callback.assert_called_once() + callback_url_arg = mock_send_callback.call_args.kwargs.get("callback_url") or mock_send_callback.call_args.args[0] + assert callback_url_arg == request.callback_url
303-360: Consider parametrizing the ancestor-conversation found vs. not-found testsThe two tests are nearly identical, differing only in the conversation lookup result and expected previous_response_id. Parametrizing would reduce duplication and simplify maintenance.
Also applies to: 362-419
168-224: Optional: Drop unnecessary patches in endpoint scheduling testsSince
process_responseis mocked and not executed, patches forOpenAI,LangfuseTracer,get_ancestor_id_from_response, andcreate_conversationare redundant in these endpoint tests. Removing them will simplify and speed up the tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/api/routes/responses.py (3)
responses(288-379)process_response(128-284)ResponsesAPIRequest(47-54)backend/app/tests/conftest.py (4)
client(46-49)user_api_key_header(71-73)db(18-35)user_api_key(83-85)
⏰ 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 (2)
backend/app/tests/api/routes/test_responses.py (2)
186-203: Scenario coverage for “no vector store” looks goodGood use of vector_store_ids=[] to ensure tool injection paths aren’t assumed by the endpoint.
110-118: Decorator stacking order is correctThe order of injected mocks matches the reverse of the patch decorators. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/api/routes/test_responses.py (1)
154-160: Make scheduled-task assertions robust to kwargs (repeat from earlier review)These assertions assume positional invocation of process_response. If production switches to keyword args, tests will fail despite correct behavior. Extract the request (and other) args via kwargs with positional fallback.
Apply the following diffs:
For Lines 154–160:
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is NoneFor Lines 215–221:
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_123" - assert call_args[0][0].question == "What is Glific?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_123" + assert request_arg.question == "What is Glific?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is NoneFor Lines 364–370 (also assert scheduled ancestor/latest args):
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890" + ancestor_arg = (call_args.kwargs.get("ancestor_id") if call_args.kwargs else call_args.args[6]) + latest_conv_arg = (call_args.kwargs.get("latest_conversation") if call_args.kwargs else call_args.args[7]) + assert ancestor_arg == "resp_latest1234567890abcdef1234567890" + assert latest_conv_arg is not NoneFor Lines 426–432 (no conversation found case):
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890" + ancestor_arg = (call_args.kwargs.get("ancestor_id") if call_args.kwargs else call_args.args[6]) + latest_conv_arg = (call_args.kwargs.get("latest_conversation") if call_args.kwargs else call_args.args[7]) + assert ancestor_arg == "resp_ancestor1234567890abcdef1234567890" + assert latest_conv_arg is NoneFor Lines 487–493 (no response_id provided):
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None + ancestor_arg = (call_args.kwargs.get("ancestor_id") if call_args.kwargs else call_args.args[6]) + latest_conv_arg = (call_args.kwargs.get("latest_conversation") if call_args.kwargs else call_args.args[7]) + assert ancestor_arg is None + assert latest_conv_arg is NoneAlso applies to: 215-221, 364-370, 426-432, 487-493
🧹 Nitpick comments (5)
backend/app/tests/api/routes/test_responses.py (5)
128-136: Drop unused variables from setup_common_mocks in endpoint testsIn these endpoint tests, the returned mock_client and mock_assistant aren’t used. Calling setup_common_mocks for its side-effects is sufficient and avoids unused-variable noise.
Apply these diffs across the listed blocks:
- mock_client, mock_assistant = setup_common_mocks( + setup_common_mocks( mock_get_credential, mock_get_assistant, mock_openai, mock_tracer_class, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, - ) + )Also applies to: 187-198, 336-345, 398-407, 459-468
249-256: Assert process_response is not scheduled when OpenAI credentials are missingThis path returns early; asserting that the background task was not queued hardens the test. Patch process_response and assert not called.
@patch("app.api.routes.responses.get_provider_credential") @patch("app.api.routes.responses.get_assistant_by_id") +@patch("app.api.routes.responses.process_response") def test_responses_endpoint_no_openai_credentials( mock_get_assistant, mock_get_credential, + mock_process_response, user_api_key_header, client, ): @@ response = client.post( "/api/v1/responses", json=request_data, headers=user_api_key_header ) assert response.status_code == 200 response_json = response.json() assert response_json["success"] is False assert "OpenAI API key not configured" in response_json["error"] + # Ensure background task was not scheduled + mock_process_response.assert_not_called()Also applies to: 271-278
280-287: Also assert no scheduling when credentials lack api_keySame rationale as the previous test—verify we do not enqueue work with invalid credentials.
@patch("app.api.routes.responses.get_provider_credential") @patch("app.api.routes.responses.get_assistant_by_id") +@patch("app.api.routes.responses.process_response") def test_responses_endpoint_missing_api_key_in_credentials( mock_get_assistant, mock_get_credential, + mock_process_response, user_api_key_header, client, ): @@ response = client.post( "/api/v1/responses", json=request_data, headers=user_api_key_header ) assert response.status_code == 200 response_json = response.json() assert response_json["success"] is False assert "OpenAI API key not configured" in response_json["error"] + # Ensure background task was not scheduled + mock_process_response.assert_not_called()Also applies to: 302-309
509-517: Silence Ruff ARG001 for db and prefer kwargs when inspecting OpenAI call
- db fixture is intentionally used for DB/engine side-effects; explicitly delete it to satisfy Ruff ARG001.
- Prefer call_args.kwargs over tuple indexing for resilience.
from app.api.routes.responses import ResponsesAPIRequest + + # Silence linter: db fixture is used for DB/engine side-effects + del db @@ - call_args = mock_client.responses.create.call_args[1] - assert call_args["previous_response_id"] == ( + kwargs = mock_client.responses.create.call_args.kwargs + assert kwargs["previous_response_id"] == ( "resp_latest1234567890abcdef1234567890" )Also applies to: 562-565
587-596: Mirror minor fixes in the “no ancestor conversation” variant
- Delete db to satisfy Ruff ARG001.
- Use kwargs for safer assertion of previous_response_id.
from app.api.routes.responses import ResponsesAPIRequest + + # Silence linter: db fixture is used for DB/engine side-effects + del db @@ - call_args = mock_client.responses.create.call_args[1] - assert call_args["previous_response_id"] == ( + kwargs = mock_client.responses.create.call_args.kwargs + assert kwargs["previous_response_id"] == ( "resp_ancestor1234567890abcdef1234567890" )Also applies to: 639-643
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/api/routes/responses.py (3)
responses(288-379)process_response(128-284)ResponsesAPIRequest(47-54)backend/app/tests/conftest.py (4)
user_api_key_header(71-73)client(46-49)db(18-35)user_api_key(83-85)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
505-505: Unused function argument: db
(ARG001)
583-583: Unused function argument: db
(ARG001)
🔇 Additional comments (5)
backend/app/tests/api/routes/test_responses.py (5)
5-16: Helper factory for assistant looks goodGood defaults, avoids mutable default pitfalls via None+assignment. Clear and reusable.
19-39: Mock OpenAI response helper is solidCovers all fields used by production code (id, output_text, model, usage, previous_response_id). This will keep the tests stable.
42-51: Conversation mock helper LGTMConcise and readable. Matches the fields accessed in process_response.
53-101: Centralized mock setup improves readabilityThis shared setup reduces duplication across endpoint tests and keeps intent clear. Nice.
240-247: Custom HTTPException handler wraps errors into the APIResponse envelopeA custom
@app.exception_handler(HTTPException)is registered inbackend/app/core/exception_handlers.py(lines 19–24), returningAPIResponse.failure_response(exc.detail). This ensures 404 errors are rendered as{ success: False, error: … }, so your test assertions are valid. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/app/tests/api/routes/test_responses.py (5)
154-160: Make scheduled-task assertions kwargs-safe (positional indexing is brittle).Accessing the first positional arg via call_args[0][0] will break if the production code switches to keyword-based invocation. Extract the request from kwargs first, with positional fallback.
- # Verify that the background task was scheduled with correct parameters - mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + # Verify that the background task was scheduled with correct parameters + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
216-221: Make scheduled-task assertions kwargs-safe (positional indexing is brittle).Same issue in this test; prefer kwargs with positional fallback.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_123" - assert call_args[0][0].question == "What is Glific?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_123" + assert request_arg.question == "What is Glific?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
373-378: Make scheduled-task assertions kwargs-safe (positional indexing is brittle).Future-proof the assertions by resolving the request via kwargs first.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"
435-440: Make scheduled-task assertions kwargs-safe (positional indexing is brittle).Use kwargs-first extraction for robustness.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890"
496-501: Make scheduled-task assertions kwargs-safe (positional indexing is brittle).Same change here.
- mock_process_response.assert_called_once() - call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + mock_process_response.assert_called_once() + call_args = mock_process_response.call_args + request_arg = call_args.kwargs.get("request") or call_args.args[0] + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
🧹 Nitpick comments (5)
backend/app/tests/api/routes/test_responses.py (5)
188-198: Avoid unused local variables from setup_common_mocks.mock_client and mock_assistant are not used; switch to underscore assignment (or no assignment) to silence F841 and clarify intent.
- mock_client, mock_assistant = setup_common_mocks( + _, _ = setup_common_mocks( mock_get_credential, mock_get_assistant, mock_openai, mock_tracer_class, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, assistant_model="gpt-4", vector_store_ids=[], )
344-353: Avoid unused local variables from setup_common_mocks.Same here; assigned values are not referenced.
- mock_client, mock_assistant = setup_common_mocks( + _, _ = setup_common_mocks( mock_get_credential, mock_get_assistant, mock_openai, mock_tracer_class, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, conversation_found=True, )
468-476: Avoid unused local variables from setup_common_mocks.Same pattern; use underscores to make it explicit.
- mock_client, mock_assistant = setup_common_mocks( + _, _ = setup_common_mocks( mock_get_credential, mock_get_assistant, mock_openai, mock_tracer_class, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, )
513-515: Remove unused fixture parameterdb(Ruff ARG001).db isn’t referenced in the body. Drop it from the signature.
-def test_process_response_ancestor_conversation_found( +def test_process_response_ancestor_conversation_found( mock_send_callback, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, - db, user_api_key, ):
591-593: Remove unused fixture parameterdb(Ruff ARG001).Same as above; not used.
-def test_process_response_ancestor_conversation_not_found( +def test_process_response_ancestor_conversation_not_found( mock_send_callback, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, - db, user_api_key, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/api/routes/responses.py (3)
responses(288-379)process_response(128-284)ResponsesAPIRequest(47-54)backend/app/tests/conftest.py (4)
user_api_key_header(71-73)client(46-49)db(18-35)user_api_key(83-85)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
513-513: Unused function argument: db
(ARG001)
591-591: 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 (2)
backend/app/tests/api/routes/test_responses.py (2)
27-29: Good defensive default for mutable argument.Using output=None and setting output = [] avoids mutable default arg pitfalls. Nice.
245-246: Global HTTPException handler ensures unified error formatI found the global
HTTPExceptionhandler inbackend/app/core/exception_handlers.py(lines 19–22), which usesAPIResponse.failure_response(…)to wrap errors into the{ "success": False, "error": "…" }shape. Your existing assertions are therefore correct and no changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/app/tests/api/routes/test_responses.py (5)
154-160: Make scheduled-task assertions resilient to kwargs (avoid brittle positional indexing)Future refactors may switch to keyword-arg scheduling for
process_response, which would breakcall_args[0][0]. Extract the request via kwargs with a positional fallback.Apply:
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None + + # Optional: assert tracer wiring for completeness + tracer_call = mock_tracer_class.call_args + assert (tracer_call.kwargs.get("response_id") if tracer_call.kwargs else None) is None
215-221: Same here: extractrequestfrom kwargs for robustnessMirror the kwargs-first extraction pattern to avoid brittleness.
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_123" - assert call_args[0][0].question == "What is Glific?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_123" + assert request_arg.question == "What is Glific?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None + + # Optional: sanity-check the assistant passed through + assistant_arg = call_args.kwargs.get("assistant") if call_args.kwargs else call_args.args[2] + assert getattr(assistant_arg, "vector_store_ids", None) == []
372-378: Use kwargs-safe extraction and assert ancestor/latest wiringIn addition to kwargs-safe
requestextraction, assert that the endpoint passes the latest conversation’s response_id asancestor_idand thelatest_conversationinstance when the ancestor is found.- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890" + + ancestor_arg = call_args.kwargs.get("ancestor_id") if call_args.kwargs else call_args.args[6] + latest_conv_arg = call_args.kwargs.get("latest_conversation") if call_args.kwargs else call_args.args[7] + # When a conversation exists, ancestor_id should be set to latest.response_id + assert ancestor_arg == "resp_latest1234567890abcdef1234567890" + assert latest_conv_arg is not None + + # Optional: verify tracer carries the response_id from the request + tracer_call = mock_tracer_class.call_args + assert (tracer_call.kwargs.get("response_id") if tracer_call.kwargs else None) == request_data["response_id"]
434-440: Same: kwargs-safe extraction and verify ancestor passthrough when no conversation foundWhen no conversation is found,
ancestor_idshould remain the originalresponse_idandlatest_conversationshould be None.- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id == "resp_ancestor1234567890abcdef1234567890" + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id == "resp_ancestor1234567890abcdef1234567890" + + ancestor_arg = call_args.kwargs.get("ancestor_id") if call_args.kwargs else call_args.args[6] + latest_conv_arg = call_args.kwargs.get("latest_conversation") if call_args.kwargs else call_args.args[7] + assert ancestor_arg == request_data["response_id"] + assert latest_conv_arg is None
495-501: Repeat: kwargs-safe extraction for scheduled request assertionsMake the request extraction robust across calling styles.
- call_args = mock_process_response.call_args - assert call_args[0][0].assistant_id == "assistant_dalgo" - assert call_args[0][0].question == "What is Dalgo?" - assert call_args[0][0].callback_url == "http://example.com/callback" - assert call_args[0][0].response_id is None + call_args = mock_process_response.call_args + request_arg = (call_args.kwargs.get("request") if call_args.kwargs else call_args.args[0]) + assert request_arg.assistant_id == "assistant_dalgo" + assert request_arg.question == "What is Dalgo?" + assert request_arg.callback_url == "http://example.com/callback" + assert request_arg.response_id is None
🧹 Nitpick comments (4)
backend/app/tests/api/routes/test_responses.py (4)
508-516: Remove unused fixturedb(Ruff ARG001) in direct process_response test
dbis not used in this test. Drop it to satisfy ARG001 and tighten the signature.-def test_process_response_ancestor_conversation_found( +def test_process_response_ancestor_conversation_found( mock_send_callback, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, - db, user_api_key, ):
586-593: Remove unused fixturedb(Ruff ARG001) in the second direct process_response testSame rationale as above;
dbis unused here as well.-def test_process_response_ancestor_conversation_not_found( +def test_process_response_ancestor_conversation_not_found( mock_send_callback, mock_get_ancestor_id_from_response, mock_create_conversation, mock_get_conversation_by_ancestor_id, - db, user_api_key, ):
519-526: Minor: import ResponsesAPIRequest once at module top to avoid repeated intra-test importsThis avoids repetitive in-function imports and keeps imports centralized.
Outside these ranges, at the top of the file, adjust imports to:
from app.api.routes.responses import process_response, ResponsesAPIRequestThen remove the in-function imports in these two tests.
Also applies to: 597-604
183-199: Optional: add a process_response unit test for assistant without vector storeYou already cover the endpoint path for an assistant with no vector store. Consider a direct
process_responseunit test withvector_store_ids=[]to assert thattoolsandincludeare omitted in the call toclient.responses.create. I can draft this test if helpful.Also applies to: 343-353, 405-415, 467-477
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_responses.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (2)
backend/app/api/routes/responses.py (3)
responses(288-379)process_response(128-284)ResponsesAPIRequest(47-54)backend/app/tests/conftest.py (4)
user_api_key_header(71-73)client(46-49)db(18-35)user_api_key(83-85)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_responses.py
513-513: Unused function argument: db
(ARG001)
591-591: 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 (3)
backend/app/tests/api/routes/test_responses.py (3)
5-16: Good test scaffolding: helpers de-duplicate setup across testsThe helper factory functions and the
setup_common_mocksutility substantially reduce duplication and improve readability. Clear defaults, and the optional parameters handle vector-store/no-vector-store cases cleanly.Also applies to: 19-39, 42-50, 53-101
280-281: Nice guard: ensure background task not scheduled when OpenAI credentials are missingThese checks are appropriate and prevent false positives.
Also applies to: 315-316
240-247: Confirmed global HTTPException handler normalizes errors to{"success": False, "error": …}The app calls
register_exception_handlers(app)inbackend/app/main.py, andhttp_exception_handlerinbackend/app/core/exception_handlers.pyusesAPIResponse.failure_response(exc.detail).model_dump(), which produces exactly thesuccess/errorshape your test asserts. No changes needed.
* fixes to testcases * using user_api_key instead * using functions for fixtures * fixing coderabbit suggestions * few more cleanups * removing unnecessary params * cleanups for testcases * coderabbit suggestions * fixing the order
3 participants
Summary
Target issue is #288
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
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit