Enhancement/write tests#8
Merged
Merged
Conversation
…nd message history
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoves the legacy ChangesController API Refactor and Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (2)
src/test/java/org/example/projectbifrost/service/ChatServiceTest.java (1)
168-168: 💤 Low valueInconsistent: new
List.of(...)instead of the already-definedmessagesvariable.The
messagesvariable defined on line 132 was explicitly introduced to be reused — yet line 168 constructs an identical new list instead.✨ Proposed fix
- String result = chatService.fetchResponseFromLLM(List.of(new OpenRouterRequestDTO.Message("user", "Hello"))); + String result = chatService.fetchResponseFromLLM(messages);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/example/projectbifrost/service/ChatServiceTest.java` at line 168, Replace the ad-hoc list construction in the test with the existing messages variable: in ChatServiceTest, change the argument passed to chatService.fetchResponseFromLLM(...) (currently List.of(new OpenRouterRequestDTO.Message("user", "Hello"))) to use the previously defined messages variable so the test reuses the setup and avoids duplicating the message list used in other assertions.src/test/java/org/example/projectbifrost/storage/ChatSessionStorageTest.java (1)
43-58: ⚡ Quick win
shouldDeleteSessionis misleadingly written —oldSessionis not the pre-deletion session.The variable named
oldSession(line 53) is actually a new session created after the firstdeleteSessioncall, not the original session from line 48. The comment// this session was removedis also wrong — S2 is freshly created at that point.The test is functionally correct (a no-op
deleteSessionwould causeoldSession == newSession, failing the assertion), but the misleading naming creates a false impression that the original instance is being compared.A simpler and clearer expression of the same intent:
♻️ Proposed refactor
`@Test` `@DisplayName`("Should delete session") void shouldDeleteSession() { String sessionId = "goodbye-session"; - storage.getOrCreateChatSession(sessionId); - + ChatSession original = storage.getOrCreateChatSession(sessionId); storage.deleteSession(sessionId); - - // After delete, when calling getOrCreate it should create a new session, so we can check that it's different from the old one - ChatSession oldSession = storage.getOrCreateChatSession(sessionId); // this session was removed - storage.deleteSession(sessionId); - ChatSession newSession = storage.getOrCreateChatSession(sessionId); - - assertThat(newSession).isNotSameAs(oldSession); + ChatSession recreated = storage.getOrCreateChatSession(sessionId); + + assertThat(recreated).isNotSameAs(original); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/example/projectbifrost/storage/ChatSessionStorageTest.java` around lines 43 - 58, The test should capture the pre-deletion session, then delete it, then create a new session to compare; change the sequence in shouldDeleteSession so you call ChatSession originalSession = storage.getOrCreateChatSession(sessionId) first, then call storage.deleteSession(sessionId), then call ChatSession newSession = storage.getOrCreateChatSession(sessionId) and assertThat(newSession).isNotSameAs(originalSession); also rename variables (originalSession/newSession) and update the incorrect comment that claimed the post-delete session was the removed one, referencing the test method shouldDeleteSession and calls to storage.getOrCreateChatSession and storage.deleteSession.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/org/example/projectbifrost/controller/BifrostControllerTest.java`:
- Line 16: The import for the get request builder in BifrostControllerTest is
using org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders;
replace that import so the static get comes from
org.springframework.test.web.servlet.request.MockMvcRequestBuilders (to match
the existing post import and standard MockMvc usage) — update the static import
for get in the BifrostControllerTest class to MockMvcRequestBuilders.get
wherever it currently references RestDocumentationRequestBuilders.get.
In `@src/test/java/org/example/projectbifrost/service/ChatServiceTest.java`:
- Line 69: The two tests in ChatServiceTest (testEmptyAnswerFromLLM and
testServerErrorThrowsException) share the same ChatRequestDTO session id
"session-123" which can leak state via chatWithLLM; update each test to use
distinct session ids (e.g., "session-empty-<unique>" and
"session-servererror-<unique>") when constructing ChatRequestDTO so session
state cannot be shared across tests and rerun in the shared Spring context.
---
Nitpick comments:
In `@src/test/java/org/example/projectbifrost/service/ChatServiceTest.java`:
- Line 168: Replace the ad-hoc list construction in the test with the existing
messages variable: in ChatServiceTest, change the argument passed to
chatService.fetchResponseFromLLM(...) (currently List.of(new
OpenRouterRequestDTO.Message("user", "Hello"))) to use the previously defined
messages variable so the test reuses the setup and avoids duplicating the
message list used in other assertions.
In
`@src/test/java/org/example/projectbifrost/storage/ChatSessionStorageTest.java`:
- Around line 43-58: The test should capture the pre-deletion session, then
delete it, then create a new session to compare; change the sequence in
shouldDeleteSession so you call ChatSession originalSession =
storage.getOrCreateChatSession(sessionId) first, then call
storage.deleteSession(sessionId), then call ChatSession newSession =
storage.getOrCreateChatSession(sessionId) and
assertThat(newSession).isNotSameAs(originalSession); also rename variables
(originalSession/newSession) and update the incorrect comment that claimed the
post-delete session was the removed one, referencing the test method
shouldDeleteSession and calls to storage.getOrCreateChatSession and
storage.deleteSession.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 878b93e9-c641-40a5-bdfb-f275bc253d43
📒 Files selected for processing (6)
src/main/java/org/example/projectbifrost/BifrostController.javasrc/main/java/org/example/projectbifrost/dto/ChatRequestDTO.javasrc/main/java/org/example/projectbifrost/storage/ChatSessionStorage.javasrc/test/java/org/example/projectbifrost/controller/BifrostControllerTest.javasrc/test/java/org/example/projectbifrost/service/ChatServiceTest.javasrc/test/java/org/example/projectbifrost/storage/ChatSessionStorageTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/projectbifrost/BifrostController.java
…proved clarity and error handling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request improves validation, adds comprehensive unit and integration tests, and clarifies session management in the Bifrost chat service. The main changes include enhanced validation for chat requests, expanded test coverage for both the controller and service layers, and improvements to session storage and cleanup.
Validation Improvements
ChatRequestDTOrecord to provide clearer error feedback when required fields are missing or empty.Testing Enhancements
BifrostControllerTestto verify correct HTTP responses for valid and invalid chat requests, including chat history retrieval.ChatServiceTestto cover complete chat flows, empty LLM responses, server errors, and circuit breaker logic, ensuring robust error handling and session management. [1] [2] [3]ChatSessionStorageTestto verify session creation, retrieval, and deletion logic in session storage.Session Management and Cleanup
deleteSessionmethod inChatSessionStorageand added a note about considering automatic cleanup strategies for production use.API Adjustments
/bifrostGET endpoint fromBifrostControllerto clean up the API surface.Summary by CodeRabbit
Breaking Changes
/api/bifrostendpoint; clients must migrate to/api/v1/chatendpoints.Improvements
Tests