#144 - SSE를 Game에서 분리한다 #149

waterricecake · 2024-11-07T08:02:50Z

close #144

회의 결과 처럼 의존성 역전으로 구현을 하였습니다.

또한 Repository -> Session으로 이름을 변경 후

common.infra 패키지로 이동시켰습니다.

test의 경우 infra 테스트를 추가하였습니다.

kpeel5839

고생했어요!

코멘트 한번 보시고, 의견 나눠봐용

kpeel5839 · 2024-11-09T10:39:36Z

src/main/java/mafia/mafiatogether/common/aspect/SseAspect.java

+    public static final long HOURS_12 = 43200_000L;
+    private final SseEmitterSession sseEmitterSession;
+
+    @Around("@annotation(mafia.mafiatogether.common.annotation.SseSubscribe)")


이건 패키지를 이동해서 이렇게 추가된 것처럼 나오는건가? 이름바꿔서 그런가. 전에 봤던거랑 완전히 똑같은 것 같은데

패키지 이동하고 이름바꿔서 그런거 같습니다.

kpeel5839 · 2024-11-09T10:43:30Z

src/main/java/mafia/mafiatogether/common/aspect/SseAspect.java

+    public static SseEmitter getSseEmitter(final String name, final Object event) throws IOException {
+        final SseEmitter sseEmitter = new SseEmitter(HOURS_12);
+        SseEventBuilder sseEventBuilder = SseEventPublisher.getSseEventBuilder(name, event);
+        sseEmitter.send(sseEventBuilder);
+        return sseEmitter;
+    }


지금 보니까, aspect인데, 실제로 호출하는게 조금 어색한 것 같아요

달리 생각은 어떠세요? Aspect 말고, 다른 클래스로 분리해야할 것 같다는 생각이 들어요.

차라리 publisher라던가?

나도 뭔가 이부분
public static SseEmitter getSseEmitter
static으로 호출하는부분이 좀 어색했어

SseEventPublisher 처리해줘도 좋을듯

오 확실히 Aspect가져다 쓰는게 어색하기는 하네요.
7d8a0d0

kpeel5839 · 2024-11-09T10:45:33Z

src/main/java/mafia/mafiatogether/common/infra/SseEventPublisher.java

+    public static final long SECOND_30 = 30_000L;
+    private final SseEmitterSession sseEmitterSession;
+
+    public void publishEventToAllSseClient(final String code, final String eventName, final Object event) {


이 부분은 뭔가 정말 SSE 연결된 이들한테 다 보낸다는 느낌이 드네요.

특정 방에 모든 sse client를 가진 이들에게 event를 발생시킨다라는 느낌을 더 살려봐도 괜찮을 것 같아요

이것처럼?

publishSseEventByCode() 로 바꾸었습니다!
7d8a0d0

kpeel5839 · 2024-11-09T10:47:38Z

src/main/java/mafia/mafiatogether/lobby/application/LobbyEventListener.java

-        }
+        final String code = lobby.getCode();
+        final LobbyInfoResponse lobbyInfoResponse = LobbyInfoResponse.of(lobby, participantName);
+        sseEventPublisher.publishEventByCodeAndName(code, participantName, LOBBY_EVENT_NAME, lobbyInfoResponse);


구우욷! 사용하는 부분이 환상적이군 역시

kpeel5839 · 2024-11-09T12:06:50Z

src/test/java/mafia/mafiatogether/common/aspect/SseAspectTest.java

+        final PlayerInfoDto playerInfoDto = new PlayerInfoDto(code, name);
+
+        // when
+        final SseEmitter actual = sseAspectTestService.subscribe(playerInfoDto);


그냥 TestComponent 안만들고, sseAspect의 subscribe 메서드를 바로 테스트하는 것을 생각해보았는데 ProceedingJoinPoint를 모킹하고, 그에대한 동작도 다 컨트롤해줘야해서, 오히려 힘들군요 ㅠ

@InjectMocks private SseAspect sseAspect; @Mock private SseEmitterSession sseEmitterSession; @Test void SSE_구독을_한다() throws Throwable { // given final String code = "code"; final String name = "name"; PlayerInfoDto playerInfoDto = new PlayerInfoDto(code, name); ProceedingJoinPoint joinPoint = mock(ProceedingJoinPoint.class); MethodSignature methodSignature = mock(MethodSignature.class); Method method = GameController.class.getDeclaredMethod("subscribe", PlayerInfoDto.class); given(joinPoint.getSignature()).willReturn(methodSignature); given(methodSignature.getMethod()).willReturn(method); given(joinPoint.getArgs()).willReturn(new Object[]{playerInfoDto}); SseEmitter sseEmitter = mock(SseEmitter.class); given(joinPoint.proceed()).willReturn(sseEmitter); // when SseEmitter actual = (SseEmitter) sseAspect.subscribe(joinPoint); // then verify(sseEmitterSession).save(code, name, sseEmitter); assertThat(actual).isEqualTo(sseEmitter); verify(sseEmitter).onCompletion(any()); verify(sseEmitter).onTimeout(any()); }

그래도 이렇게 할 수 있을 것 같긴해요

저도 처음에는 testComponent로 상황을 만들어서 테스트하지 않고 기존에 사용하는 Game.subscribe 메서드를 활용할까 생각을 했었는데.... 그렇게 할 경우 해당 메서드가 변경되거나 하면 이 테스트도 고쳐야하니까 독립적으로 테스트를 구축하는게 맞지 않나 해서 testComponent를 통해 테스트를 구축하는 것을 선택하였습니다.

SseAspect 테스트를 진행한 이유가 SseAspect#subscribe 를 테스트 하려고 하신거 아니에요?
현재 위 방식은, 딱 SseAspect#subscribe만 테스트 하고 있는데요.

SseAspect 테스트를 진행한 이유가 SseAspect#subscribe 를 테스트 하려고 하신거 아니에요?
만일 이게 맞다면, subscribe가 변경되었을 때, 테스트가 변경되는건 당연하다고 생각해요.

아니면, mocking을 할 경우, 테스트 하려는 메서드가 변경되었을 때, 변경사항이 너무 많다는 것을 말씀하신건가요?

mocking을 하는 것은 상관없지만, 변경 되었을때 영향을 받는거라기 보다는 SseAspect는 common 패키지 범위에서 테스트가 되어야한다고 생각했습니다.

테스트 또한 패키지간 의존성을 따라야한다고 생각했어요.

재연님이 제안해주신 것처럼 subscribe의 메서드 자체를 테스트하는 것이 더 확실한 테스트라고 생각이 들긴한데

Method method = GameController.class.getDeclaredMethod("subscribe", PlayerInfoDto.class);

에서 GameController에서 메서드를 가져올때 common -> game에 의존성이 생긴다고 생각했습니다.

그래서 common test에서는 테스트를 위한 메서드를 만들 필요가 있다고 생각해서 testComponent를 생성했었습니다.

저기 메서드 부분만

Method method = SseAspectTestService.class.getDeclaredMethod("subscribe", PlayerInfoDto.class);

로 한다면 재연님이 제안해주신 부분이 좀더 확실한 테스트가 될것 같습니다.
385376c

아하! GameController쪽에 의존성이 생긴다는 말씀을 하신거군요.

그럼 method를 모킹하면 되지 않을까요?

이랗게하면 뭔가 배보다 배꼽이 더 커지는 느낌이긴한데, 개인적으로 stub을 별로 안좋아해서 그런건가?

근데 오히려 지금이 mocking이랑 stub이 적절히 어우러져있는 것 같기도하고 흐으음

개인적인 생각으로는 그냥 method까지 mocking해서 playerinfo를 반환하게끔하면 더 명확한 테스트가 될 것 같긴해요

아 메소드를 모킹하면 되는구나 맞네요
e29573f

thdwoqor

고생했어 👍

thdwoqor · 2024-11-09T12:23:45Z

src/main/java/mafia/mafiatogether/common/aspect/SseAspect.java

+    public static SseEmitter getSseEmitter(final String name, final Object event) throws IOException {
+        final SseEmitter sseEmitter = new SseEmitter(HOURS_12);
+        SseEventBuilder sseEventBuilder = SseEventPublisher.getSseEventBuilder(name, event);
+        sseEmitter.send(sseEventBuilder);
+        return sseEmitter;
+    }


나도 뭔가 이부분
public static SseEmitter getSseEmitter
static으로 호출하는부분이 좀 어색했어

SseEventPublisher 처리해줘도 좋을듯

waterricecake added 7 commits November 5, 2024 15:08

refactor : SSE 로직 Game에서 분리

6d91132

refactor : SSE event publish 로직 bean을 통해 common으로 이동

68657be

refactor : sse disconnect 로직 common으로 이동

e89d16c

refactor : SseRepository 명 Session 변경 및 infra package로 이동

d2776a7

Test : SseEventPublisher 테스트 작성

1f8eab1

test : SseAspectTest 구현

3fad05c

refactor : 상수 함수 내부 변수로 변경

ef61dec

waterricecake requested review from thdwoqor and kpeel5839 November 7, 2024 08:02

waterricecake self-assigned this Nov 7, 2024

waterricecake added the Backend label Nov 7, 2024

refactor : final 키워드 추가 및 SseEventBuilder import 추가

c908a37

kpeel5839 requested changes Nov 9, 2024

View reviewed changes

thdwoqor approved these changes Nov 9, 2024

View reviewed changes

waterricecake added 3 commits November 10, 2024 17:42

refactor : 메서드 네임 수정 및 Sse 생성 매서드 위치 이동

7d8a0d0

refactor : aspect test 수정

385376c

refactor : aspect test method 모킹

e29573f

kpeel5839 approved these changes Nov 10, 2024

View reviewed changes

refactor : mockito 버전 문제 해결

3c26c0f

waterricecake merged commit a1b3e8b into dev Nov 11, 2024
1 check passed

waterricecake deleted the refactor/#144-sse-dependency-dip branch November 11, 2024 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#144 - SSE를 Game에서 분리한다 #149

#144 - SSE를 Game에서 분리한다 #149

waterricecake commented Nov 7, 2024

kpeel5839 left a comment

kpeel5839 Nov 9, 2024

waterricecake Nov 10, 2024

kpeel5839 Nov 9, 2024

thdwoqor Nov 9, 2024

waterricecake Nov 10, 2024

kpeel5839 Nov 9, 2024

kpeel5839 Nov 9, 2024

waterricecake Nov 10, 2024

kpeel5839 Nov 9, 2024

kpeel5839 Nov 9, 2024

kpeel5839 Nov 9, 2024

waterricecake Nov 10, 2024

kpeel5839 Nov 10, 2024

waterricecake Nov 10, 2024 •

edited

Loading

kpeel5839 Nov 10, 2024

waterricecake Nov 10, 2024

thdwoqor left a comment

thdwoqor Nov 9, 2024

#144 - SSE를 Game에서 분리한다 #149

#144 - SSE를 Game에서 분리한다 #149

Conversation

waterricecake commented Nov 7, 2024

kpeel5839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thdwoqor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Nov 10, 2024 •

edited

Loading