Use proper Request object to test REST API endpoints handlers #371
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe test files for query and streaming query endpoint handlers were updated to replace Changes
Sequence Diagram(s)No sequence diagram generated, as the changes are limited to test input construction and do not alter control flow or introduce new features. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
use-proper-request-object-to-test-endpoints
branch
from
e286b59 to
c88b383
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: 2
♻️ Duplicate comments (5)
tests/unit/app/endpoints/test_streaming_query.py (5)
153-157: Minimal Request construction is fine here tooSame note as above—this is correct, and a dummy_request fixture could reduce repetition.
269-273: Request construction: OKNo functional issues. Consider reusing a dummy_request fixture to reduce duplication across tests.
1277-1281: Request construction: OKMinimal scope is sufficient since _request is unused. Can be replaced by a fixture for DRY.
1324-1328: Request construction: OKRepeated pattern—consider a single helper fixture to reduce duplication.
1369-1373: Request construction: OKSame DRY suggestion as above.
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_query.py (1)
72-72: Use a tuple (or MOCK_AUTH) for the auth parameter for type consistencyAuth is modeled as a tuple by the handler. Passing a tuple (or the already-declared MOCK_AUTH) improves consistency.
- await query_endpoint_handler(query_request, auth=["test-user", "", "token"]) + await query_endpoint_handler(query_request, auth=MOCK_AUTH)tests/unit/app/endpoints/test_streaming_query.py (1)
121-125: Constructing a minimal Request is correct; consider a helper to avoid repetitionUsing Request(scope={"type": "http"}) is sufficient since the handler ignores _request. To DRY repeated construction, add a small fixture:
# conftest.py or in this module import pytest from fastapi import Request @pytest.fixture def dummy_request(): return Request(scope={"type": "http"})Then use dummy_request in tests to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/app/endpoints/test_streaming_query.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/app/endpoints/test_query.py (2)
src/models/requests.py (1)
QueryRequest(70-212)src/app/endpoints/query.py (1)
query_endpoint_handler(69-126)
tests/unit/app/endpoints/test_streaming_query.py (1)
src/app/endpoints/streaming_query.py (1)
streaming_query_endpoint_handler(383-465)
🔇 Additional comments (6)
tests/unit/app/endpoints/test_query.py (1)
69-71: Good switch to a real QueryRequestCreating a minimal valid QueryRequest here is correct and keeps the test realistic without adding complexity.
tests/unit/app/endpoints/test_streaming_query.py (5)
11-11: Importing Request is necessary for constructing real FastAPI Request objectsThis import aligns with the new usage. LGTM.
275-277: Passing the constructed Request is correctThis aligns the signature to the handler. LGTM.
1283-1283: Handler invocation with Request looks goodThis enables proper context for the streaming handler. LGTM.
1330-1331: LGTMProperly passing Request alongside query_request.
1375-1376: LGTMCorrect usage with the new signature.
| with pytest.raises(HTTPException) as e: | ||
| await streaming_query_endpoint_handler(None, query_request, auth=MOCK_AUTH) | ||
| await streaming_query_endpoint_handler(request, query_request, auth=MOCK_AUTH) | ||
| assert e.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert e.detail["response"] == "Configuration is not loaded" | ||
|
|
There was a problem hiding this comment.
Fix pytest.raises assertions (must assert outside the context and use e.value)
Asserting on e.status_code/e.detail inside the with-block and without using e.value is incorrect and can raise AttributeError. Move assertions outside and use e.value.
with pytest.raises(HTTPException) as e:
await streaming_query_endpoint_handler(request, query_request, auth=MOCK_AUTH)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert e.value.detail["response"] == "Configuration is not loaded"🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_streaming_query.py around lines 127 to 131, the
assertions on the HTTPException are incorrectly placed inside the pytest.raises
context and use e instead of e.value. Move the assertions outside the with-block
and change them to use e.value.status_code and e.value.detail to correctly
access the exception attributes.
| with pytest.raises(HTTPException) as e: | ||
| await streaming_query_endpoint_handler(None, query_request, auth=MOCK_AUTH) | ||
| await streaming_query_endpoint_handler(request, query_request, auth=MOCK_AUTH) | ||
| assert e.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert e.detail["response"] == "Configuration is not loaded" | ||
|
|
There was a problem hiding this comment.
Simulate connection error at the correct call and fix assertions and expected message
Two issues:
- APIConnectionError should be raised by client.models.list(), not by accessing client.models.
- Inside-with assertions and using e instead of e.value are incorrect.
- The expected message for connection failures should be “Unable to connect to Llama Stack”.
# raise the error when listing models
mock_client.models.list.side_effect = APIConnectionError(request=query_request)
with pytest.raises(HTTPException) as e:
await streaming_query_endpoint_handler(request, query_request, auth=MOCK_AUTH)
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
assert e.value.detail["response"] == "Unable to connect to Llama Stack"Also, you can remove one of the duplicate patches:
# Keep just one
mocker.patch("client.AsyncLlamaStackClientHolder.get_client", return_value=mock_client)🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_streaming_query.py around lines 159 to 163, fix
the test to simulate the APIConnectionError correctly by setting the side_effect
on mock_client.models.list() instead of accessing client.models. Move the
assertions outside the with block and use e.value for accessing the exception.
Update the expected error message to "Unable to connect to Llama Stack". Also,
remove the duplicate patch for AsyncLlamaStackClientHolder.get_client, keeping
only one instance.
1 participant
Description
Use proper Request object to test REST API endpoints handlers
Type of change
Summary by CodeRabbit