test: WebSocket 관련 테스트 코드 작성 #440
Merged
Conversation
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
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (2)
71-73: MockBean 주입 선택 잘 하셨습니다.
- SimpMessagingTemplate를 MockBean으로 주입해 서비스-메시징 경계 테스트가 깔끔해졌습니다.
- 실패 케이스에서 상호작용이 전혀 없음을 추가로 검증해 주면 안전망이 더 단단해집니다.
376-423: 전송 테스트 전반 품질 좋습니다. 추가로 있으면 좋은 테스트 아이디어를 공유드립니다.
- 입력 검증: 빈 문자열/과도한 길이/금지 단어 전송 시 거절.
- 첨부 포함 전송: 이미지/파일 첨부 메시지 전송 흐름과 브로드캐스트 페이로드 구조 검증.
- 경계 케이스: 존재하지 않는 채팅방 ID, 비활성화된 채팅방, 차단/숨김 상태.
- 사이드이펙트: 메시지 영속화 여부, 마지막 메시지/읽지 않은 수 갱신.
- 발신자/수신자 라우팅: 필요 시 convertAndSendToUser 등의 개인 큐 사용 여부 검증.
원하시면 위 시나리오들에 대한 테스트 스켈레톤을 생성해 드리겠습니다.
src/test/java/com/example/solidconnection/websocket/WebSocketStompIntegrationTest.java (2)
70-71: transportErrorQueue는 무제한(또는 여유 있는) 큐로 바꾸는 게 안전합니다.
- ArrayBlockingQueue(1)는 1건 초과 에러 발생 시 add에서 블로킹/실패할 수 있습니다.
- LinkedBlockingQueue로 바꾸면 예기치 못한 추가 에러에도 안전합니다.
- private final BlockingQueue<Throwable> transportErrorQueue = new ArrayBlockingQueue<>(1); + private final BlockingQueue<Throwable> transportErrorQueue = new java.util.concurrent.LinkedBlockingQueue<>();
67-113: 통합 검증을 한 단계 확장할 수 있는 제안입니다.
- CONNECT 이후 SUBSCRIBE/UNSUBSCRIBE까지: 임의 토픽 구독 후 서버에서 테스트용 프레임을 발행해 수신까지 end-to-end 검증.
- 토큰 만료/변조: 만료 토큰, 잘못 서명된 토큰으로 401/403 경계값 검증.
- Origin/CORS 제약: 서버가 오리진 제한을 둔다면 허용/비허용 오리진으로 행위 검증.
- Heartbeat 설정 유효성: 서버/클라이언트 간 heartbeat 협상(미설정 시에도 정상 동작 확인).
필요하시다면 STOMP subscribe/send/receipt를 포함한 시나리오 테스트 템플릿을 제공하겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java(4 hunks)src/test/java/com/example/solidconnection/websocket/WebSocketStompIntegrationTest.java(1 hunks)
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (1)
407-408: senderId의 도메인 의미를 확인해 주세요 (ChatParticipant ID vs SiteUser ID).
- 현재 테스트는 senderId가 ChatParticipant의 ID라고 가정하고 있습니다.
- API 계약이 “사용자 ID”를 의미한다면, 테스트 기대값은 sender.getId()가 되어야 합니다.
가능한 경우 아래와 같이 조정해 주세요.
- () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId()) + () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(sender.getId())src/test/java/com/example/solidconnection/websocket/WebSocketStompIntegrationTest.java (1)
52-58: 전반적인 STOMP 클라이언트 설정이 명확하고 적절합니다.
- SockJS(WebSocket 전용) + Jackson 컨버터 설정이 목적에 부합합니다.
- 테스트 가독성이 좋고, 5초 타임아웃도 현실적입니다.
Comment on lines
+399
to
+409
| // then | ||
| ArgumentCaptor<String> destinationCaptor = ArgumentCaptor.forClass(String.class); | ||
| ArgumentCaptor<ChatMessageSendResponse> payloadCaptor = ArgumentCaptor.forClass(ChatMessageSendResponse.class); | ||
|
|
||
| BDDMockito.verify(simpMessagingTemplate).convertAndSend(destinationCaptor.capture(), payloadCaptor.capture()); | ||
|
|
||
| assertAll( | ||
| () -> assertThat(destinationCaptor.getValue()).isEqualTo("/topic/chat/" + chatRoom.getId()), | ||
| () -> assertThat(payloadCaptor.getValue().content()).isEqualTo(content), | ||
| () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId()) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
검증을 인자 기반(BDD then + eq)으로 바꿔, 호출 횟수/추가 호출에 둔감하게 만듭니다.
- 현재 captor로 destination을 직접 캡처하면, convertAndSend가 추가로 더 호출되는 구조로 변경될 때 테스트가 불필요하게 깨질 수 있습니다.
- 목적지(destination)는 eq로 검증하고, payload만 captor로 캡처해 필드 단언으로 이어지게 하면 테스트 탄성이 좋아집니다.
아래처럼 수정 제안드립니다.
- ArgumentCaptor<String> destinationCaptor = ArgumentCaptor.forClass(String.class);
- ArgumentCaptor<ChatMessageSendResponse> payloadCaptor = ArgumentCaptor.forClass(ChatMessageSendResponse.class);
-
- BDDMockito.verify(simpMessagingTemplate).convertAndSend(destinationCaptor.capture(), payloadCaptor.capture());
-
- assertAll(
- () -> assertThat(destinationCaptor.getValue()).isEqualTo("/topic/chat/" + chatRoom.getId()),
- () -> assertThat(payloadCaptor.getValue().content()).isEqualTo(content),
- () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId())
- );
+ ArgumentCaptor<ChatMessageSendResponse> payloadCaptor = ArgumentCaptor.forClass(ChatMessageSendResponse.class);
+ BDDMockito.then(simpMessagingTemplate)
+ .should()
+ .convertAndSend(
+ org.mockito.ArgumentMatchers.eq("/topic/chat/" + chatRoom.getId()),
+ payloadCaptor.capture()
+ );
+ assertAll(
+ () -> assertThat(payloadCaptor.getValue().content()).isEqualTo(content),
+ () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId())
+ );추가로, eq·any를 자주 쓰신다면 ArgumentMatchers에 대한 static import를 고려해 주세요.
📝 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.
Suggested change
| // then | |
| ArgumentCaptor<String> destinationCaptor = ArgumentCaptor.forClass(String.class); | |
| ArgumentCaptor<ChatMessageSendResponse> payloadCaptor = ArgumentCaptor.forClass(ChatMessageSendResponse.class); | |
| BDDMockito.verify(simpMessagingTemplate).convertAndSend(destinationCaptor.capture(), payloadCaptor.capture()); | |
| assertAll( | |
| () -> assertThat(destinationCaptor.getValue()).isEqualTo("/topic/chat/" + chatRoom.getId()), | |
| () -> assertThat(payloadCaptor.getValue().content()).isEqualTo(content), | |
| () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId()) | |
| ); | |
| // then | |
| ArgumentCaptor<ChatMessageSendResponse> payloadCaptor = ArgumentCaptor.forClass(ChatMessageSendResponse.class); | |
| BDDMockito.then(simpMessagingTemplate) | |
| .should() | |
| .convertAndSend( | |
| org.mockito.ArgumentMatchers.eq("/topic/chat/" + chatRoom.getId()), | |
| payloadCaptor.capture() | |
| ); | |
| assertAll( | |
| () -> assertThat(payloadCaptor.getValue().content()).isEqualTo(content), | |
| () -> assertThat(payloadCaptor.getValue().senderId()).isEqualTo(senderParticipant.getId()) | |
| ); |
🤖 Prompt for AI Agents
In src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java
around lines 399 to 409, change the verification so the destination is verified
with an eq matcher (eg. eq("/topic/chat/" + chatRoom.getId())) instead of
capturing it, and only capture the payload with
ArgumentCaptor<ChatMessageSendResponse>; update the verify/then call to use the
matcher for the first argument and payloadCaptor.capture() for the second so
extra convertAndSend calls won't break the test, and consider adding a static
import for ArgumentMatchers.eq (and any) to keep the test concise.
Comment on lines
+418
to
+422
| // when & then | ||
| assertThatCode(() -> chatService.sendChatMessage(request, nonParticipant.getId(), chatRoom.getId())) | ||
| .isInstanceOf(CustomException.class) | ||
| .hasMessage(CHAT_PARTICIPANT_NOT_FOUND.getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
실패 케이스에서 메시지 브로커 호출이 전혀 없음을 보장하세요.
- 예외 단언 뒤에 no-interaction 검증을 추가하면, 회귀로 인한 오발신을 조기에 잡을 수 있습니다.
assertThatCode(() -> chatService.sendChatMessage(request, nonParticipant.getId(), chatRoom.getId()))
.isInstanceOf(CustomException.class)
.hasMessage(CHAT_PARTICIPANT_NOT_FOUND.getMessage());
+ org.mockito.Mockito.verifyNoInteractions(simpMessagingTemplate);📝 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.
Suggested change
| // when & then | |
| assertThatCode(() -> chatService.sendChatMessage(request, nonParticipant.getId(), chatRoom.getId())) | |
| .isInstanceOf(CustomException.class) | |
| .hasMessage(CHAT_PARTICIPANT_NOT_FOUND.getMessage()); | |
| } | |
| // when & then | |
| assertThatCode(() -> chatService.sendChatMessage(request, nonParticipant.getId(), chatRoom.getId())) | |
| .isInstanceOf(CustomException.class) | |
| .hasMessage(CHAT_PARTICIPANT_NOT_FOUND.getMessage()); | |
| org.mockito.Mockito.verifyNoInteractions(simpMessagingTemplate); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java
around lines 418 to 422, the failure case asserts the CustomException but does
not verify that the message broker was not called; add a no-interaction
verification for the mocked message broker (e.g., verifyNoInteractions or
verify(messageBroker, never()) semantics) immediately after the exception
assertion to ensure no messages were sent on failure and prevent
regression-induced stray calls.
Comment on lines
+60
to
+65
| @AfterEach | ||
| void tearDown() { | ||
| if (this.stompSession != null && this.stompSession.isConnected()) { | ||
| this.stompSession.disconnect(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
리소스 정리: 테스트 종료 시 stompClient.stop() 호출을 권장합니다.
- 내부 스케줄러/스레드가 남아 다른 테스트에 간섭하는 일을 예방합니다.
void tearDown() {
if (this.stompSession != null && this.stompSession.isConnected()) {
this.stompSession.disconnect();
}
+ if (this.stompClient != null) {
+ this.stompClient.stop();
+ }
}📝 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.
Suggested change
| @AfterEach | |
| void tearDown() { | |
| if (this.stompSession != null && this.stompSession.isConnected()) { | |
| this.stompSession.disconnect(); | |
| } | |
| } | |
| @AfterEach | |
| void tearDown() { | |
| if (this.stompSession != null && this.stompSession.isConnected()) { | |
| this.stompSession.disconnect(); | |
| } | |
| if (this.stompClient != null) { | |
| this.stompClient.stop(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/com/example/solidconnection/websocket/WebSocketStompIntegrationTest.java
around lines 60 to 65, the teardown currently only disconnects the stompSession
but does not stop the underlying stompClient, which can leave scheduler/threads
running and interfere with other tests; update tearDown to also call
stompClient.stop() (with a null-check on stompClient and optionally wrapped in
try/catch to avoid masking test failures) after disconnecting the session so the
client's threads are cleaned up between tests.
Comment on lines
+106
to
+112
| assertAll( | ||
| () -> assertThat(thrown) | ||
| .isInstanceOf(ExecutionException.class) | ||
| .hasCauseInstanceOf(HttpClientErrorException.Unauthorized.class), | ||
| () -> assertThat(transportErrorQueue).hasSize(1) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
인증 실패 시 transportErrorQueue 크기 단언을 완화하세요.
- 전송 계층에서 에러가 발생하지 않고 즉시 HTTP 예외로 반환될 수 있어, 큐 크기 1을 정확히 보장하기 어렵습니다.
- 비어있지 않음 정도로만 검증하면 환경차(컨테이너/드라이버)에 덜 민감합니다.
- () -> assertThat(transportErrorQueue).hasSize(1)
+ () -> assertThat(transportErrorQueue).isNotEmpty()📝 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.
Suggested change
| assertAll( | |
| () -> assertThat(thrown) | |
| .isInstanceOf(ExecutionException.class) | |
| .hasCauseInstanceOf(HttpClientErrorException.Unauthorized.class), | |
| () -> assertThat(transportErrorQueue).hasSize(1) | |
| ); | |
| } | |
| assertAll( | |
| () -> assertThat(thrown) | |
| .isInstanceOf(ExecutionException.class) | |
| .hasCauseInstanceOf(HttpClientErrorException.Unauthorized.class), | |
| () -> assertThat(transportErrorQueue).isNotEmpty() | |
| ); |
🤖 Prompt for AI Agents
In
src/test/java/com/example/solidconnection/websocket/WebSocketStompIntegrationTest.java
around lines 106 to 112, the test currently asserts transportErrorQueue
hasSize(1) which is brittle because authentication failures can be returned
immediately as HTTP exceptions without a transport-layer error; change the
assertion to check the queue is not empty (e.g.,
assertThat(transportErrorQueue).isNotEmpty() or hasSizeGreaterThan(0)) so the
test passes when at least one transport error was recorded while remaining
tolerant of environment differences; update the assertAll accordingly to use the
relaxed assertion.
Gyuhyeok99
approved these changes
Aug 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
관련 이슈
작업 내용
WebSocket + STOMP 관련 테스트 코드와 메시지 송신 테스트를 작성하였습니다.
#423 이 머지되면 rebase 하고 머지하겠습니다 !특이 사항
ArgumentCaptor라는 걸 처음 써 봤는데요, 어색한 부분이 있다면 말씀해주세요 ~WebSocket + STOMP 통합 테스트는
chat도메인 하위가 아니라 따로 작성하였습니다.리뷰 요구사항 (선택)
추가할 테스트가 있는지