Skip to content

Conversation

@max-svistunov
Copy link
Contributor

@max-svistunov max-svistunov commented Nov 21, 2025

Description

Add unit tests for conversations vs2

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue LCORE-729
  • Closes LCORE-729

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Tests

    • Added comprehensive unit tests for conversation management endpoints, covering successful operations, error scenarios, and authentication handling.
    • Enhanced validation of error messaging and response formatting across conversation endpoints.
  • Chores

    • Introduced a new public conversation data structure for improved response consistency.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

Introduces comprehensive unit tests for conversation v2 endpoints covering GET (list and single), DELETE, and PUT operations. Tests validate cache configuration handling, error scenarios, authentication, skip user ID check behavior, message transformation, and document inclusion. Adds ConversationData data structure to models.responses.

Changes

Cohort / File(s) Change Summary
Test Coverage for Conversation Endpoints
tests/unit/app/endpoints/test_conversations_v2.py
Extensive unit tests added for GetConversationsListEndpoint, GetConversationEndpoint, DeleteConversationEndpoint, and UpdateConversationEndpoint, covering cache configuration failures, missing configurations, successful retrievals, cache exceptions, authentication validation, skip_userid_check scenarios, message transformation, document inclusion, and proper HTTP error signaling (400/404/500).
Response Models
src/models/responses
New ConversationData type introduced as a public data structure for constructing conversation list items used in test data and endpoint responses.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Comprehensive test coverage spanning multiple endpoints with varied scenarios (cache failures, auth validation, bypass modes)
  • Tests for both success paths and error conditions with specific assertion on error messages
  • New data structure usage in tests and response construction requires validation of integration
  • Verify all endpoint paths are properly tested (especially UpdateConversationEndpoint topic summary updates)

Possibly related PRs

Suggested reviewers

  • tisnik

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding unit tests for conversations v2 API endpoints.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 062f651 and c1dd7a9.

📒 Files selected for processing (1)
  • tests/unit/app/endpoints/test_conversations_v2.py (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_conversations_v2.py (6)
src/app/endpoints/conversations_v2.py (7)
  • transform_chat_message (315-332)
  • update_conversation_endpoint_handler (236-281)
  • check_valid_conversation_id (284-294)
  • check_conversation_existence (297-312)
  • get_conversations_list_endpoint_handler (119-145)
  • get_conversation_endpoint_handler (150-183)
  • delete_conversation_endpoint_handler (190-231)
src/models/cache_entry.py (1)
  • CacheEntry (7-24)
src/models/requests.py (1)
  • ConversationUpdateRequest (426-449)
src/models/responses.py (3)
  • ConversationUpdateResponse (993-1025)
  • ReferencedDocument (236-248)
  • ConversationData (222-233)
src/cache/postgres_cache.py (3)
  • get (193-244)
  • delete (312-347)
  • set_topic_summary (384-411)
src/cache/sqlite_cache.py (3)
  • get (190-242)
  • delete (310-343)
  • set_topic_summary (381-406)
⏰ 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). (4)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (7)
tests/unit/app/endpoints/test_conversations_v2.py (7)

5-25: LGTM! Clean import additions.

The new imports are well-organized and all properly utilized throughout the test file. The datetime imports support timestamp conversion testing, and the new endpoint handlers and models are appropriately imported for the test coverage.


70-130: Excellent coverage of referenced_documents edge cases.

The test cases properly distinguish between None (key absent), empty list (key present with []), and populated lists. The assertion on line 111 correctly accounts for pydantic's AnyUrl trailing slash normalization.


194-329: Comprehensive test coverage for GET /conversations list endpoint.

The test suite thoroughly covers configuration errors, cache errors, successful retrievals (including empty lists), skip_userid_check propagation, and malformed auth handling. The timestamp conversion logic (lines 240-244) correctly handles ISO string to float conversion.


331-507: Thorough test coverage for GET single conversation endpoint.

The tests properly validate all error scenarios (invalid ID, missing conversation, cache errors) and success paths. The skip_userid_check verification (lines 468-470) correctly ensures the flag is propagated to the cache layer.


609-630: Verify deletion success semantics.

The test correctly verifies that when cache.delete returns False, the response still has success=True (line 629) with message "Conversation can not be deleted". This matches the current implementation, but seems semantically inconsistent—if the deletion was unsuccessful, should success be False?

Please confirm this is the intended behavior.


633-694: Good coverage of DELETE endpoint edge cases.

The skip_userid_check test (lines 651-653) properly verifies flag propagation, and the cache exception test ensures errors surface correctly. The test suite maintains consistency with other endpoint test patterns.


818-882: Complete test coverage for UPDATE endpoint.

The new tests for skip_userid_check (lines 838-840), cache exceptions, and malformed auth bring the UPDATE endpoint test coverage to parity with the other endpoints. The assertions properly verify that the skip flag is propagated through to the cache layer.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tisnik tisnik merged commit 605c218 into lightspeed-core:main Nov 21, 2025
21 of 23 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 23, 2025
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants