test: add unit tests for conversations REST API endpoints #252
Conversation
WalkthroughA new test module has been introduced to provide unit tests for the Changes
Sequence Diagram(s)sequenceDiagram
participant TestClient as Test Suite
participant API as /conversations Endpoint
participant Config as Config Loader
participant Mapper as Conversation ID Mapper
participant Llama as Llama Stack Client
TestClient->>API: Send GET/DELETE /conversations/{id}
API->>Config: Load configuration
Config-->>API: Return config or raise error
API->>Mapper: Validate and map conversation ID
Mapper-->>API: Return agent ID or raise error
API->>Llama: Retrieve or delete session data
Llama-->>API: Return data or raise error
API-->>TestClient: Return simplified response or error
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (1)
tests/unit/app/endpoints/test_conversations.py (1)
292-292: Fix typo in test method nameThe method name contains a typo:
test_session_retrive_exceptionshould betest_session_retrieve_exception.- def test_session_retrive_exception(self, mocker, setup_configuration): + def test_session_retrieve_exception(self, mocker, setup_configuration):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/app/endpoints/test_conversations.py(1 hunks)
🔇 Additional comments (5)
tests/unit/app/endpoints/test_conversations.py (5)
1-20: Well-structured test module setup!The imports are properly organized and the test constants are clearly defined with descriptive names.
22-124: Comprehensive test fixtures with proper isolation!The fixtures provide excellent test data setup and ensure proper test isolation through the
autousefixture that clears the conversation mapping before and after each test.
126-202: Thorough test coverage for simplify_session_data function!The test class effectively covers the main functionality, edge cases (empty turns), and field filtering behavior. Good use of mocking to simulate the session object.
204-352: Excellent test coverage for GET endpoint!The test class thoroughly covers all error scenarios and the success case with proper mocking and assertions. The test structure follows best practices.
354-496: Comprehensive test coverage for DELETE endpoint!The test class mirrors the GET endpoint tests appropriately, providing thorough coverage of all error scenarios and the success case. The consistent structure between both endpoint test classes enhances maintainability.
eranco74
force-pushed
the
conversations_tests
branch
2 times, most recently
from
fad72a2 to
21d4ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/app/endpoints/test_conversations.py (1)
495-495: Add newline at end of file.Python files should end with a newline character.
Add a newline character after line 494.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/app/endpoints/test_conversations.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_conversations.py
[warning] 155-155: Pylint: "result == []" can be simplified to "not result", if it is strictly a sequence, as an empty list is falsey (use-implicit-booleaness-not-comparison)
🔇 Additional comments (5)
tests/unit/app/endpoints/test_conversations.py (5)
1-20: Well-structured imports and test constants.The imports are properly organized and the test constants are clearly defined with descriptive names.
22-124: Excellent fixture design with proper test isolation.The fixtures are well-structured, especially the
setup_conversation_mappingfixture withautouse=Truethat ensures clean state between tests. The mock data is comprehensive and realistic.
126-202: Comprehensive test coverage for simplify_session_data function.The test methods effectively cover the main scenarios: normal operation, empty data, and field filtering. The test structure and assertions are well-designed.
204-351: Excellent test coverage for GET endpoint with comprehensive error scenarios.The test class thoroughly covers all error paths and the success case. The use of mocking is appropriate and the assertions properly validate both status codes and error messages.
353-494: Comprehensive test coverage for DELETE endpoint matching GET endpoint quality.The test structure effectively mirrors the GET endpoint tests, providing consistent and thorough coverage of all error scenarios and the success case.
Signed-off-by: Eran Cohen <[email protected]>
eranco74
force-pushed
the
conversations_tests
branch
from
21d4ae5 to
12bd947
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/app/endpoints/test_conversations.py (1)
155-155: Good - Previous review comment addressed.The empty list comparison has been properly simplified as suggested in the previous review.
🧹 Nitpick comments (1)
tests/unit/app/endpoints/test_conversations.py (1)
495-495: Add newline at end of file.The file should end with a newline character for proper formatting.
mock_client.agents.session.delete.assert_called_once_with( agent_id=VALID_AGENT_ID, session_id=VALID_CONVERSATION_ID ) -495 +495 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/app/endpoints/test_conversations.py(1 hunks)
🔇 Additional comments (4)
tests/unit/app/endpoints/test_conversations.py (4)
22-124: Well-structured test fixtures!The fixtures provide excellent test isolation with the autouse fixture clearing the conversation mapping, and the mock data fixtures are comprehensive and realistic.
126-202: Comprehensive test coverage for simplify_session_data.The test cases effectively cover the main scenarios including model_dump handling, empty turns, and field filtering. The assertions are clear and validate the expected behavior.
204-351: Excellent test coverage for GET endpoint.The tests comprehensively cover all error scenarios and the success case, with proper mocking of external dependencies and clear assertions on HTTP status codes and error messages.
353-494: Excellent test coverage for DELETE endpoint.The tests comprehensively cover all error scenarios and the success case, maintaining consistency with the GET endpoint tests while appropriately adapting for DELETE operations.
2 participants
Description
Somehow, I left out the tests for the conversations endpoints
This complements #244
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit