[레거시 코드 리팩터링 - 1단계] 도기(김동호) 미션 제출합니다. #519
Conversation
| @Target({ElementType.TYPE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @MockTest | ||
| public @interface ControllerTest { |
There was a problem hiding this comment.
커스텀 어노테이션으로 필요한 어노테이션들을 모아주셨군요!! 좋은 것 같아요!
혹시 ControllerTest와 MockTest 모두 설정이 동일한데 별도의 어노테이션으로 만든 이유가 있나요???
There was a problem hiding this comment.
확장성까지 고려해서 ControllerTest 안에 MockTest를 가지는 식으로 구현했습니다.
근데 다시 생각해보니 불필요한 것 같네요 😅
ControllerTest 어노테이션은 나중에 필요에의해 만드는 쪽으로 하겠습니다!
| - [ ] 주문에 주문 항목이 없으면 예외를 발생한다. ✅ | ||
| - [ ] 요청된 주문에 메뉴가 DB에 저장되어 있지 않으면 예외를 발생한다. ✅ | ||
| - [ ] 요청된 주문 테이블이 DB에 저장되어 있지 않으면 예외를 발생한다. ✅ | ||
| - [ ] 주문 테이블이 비어있는 상태이면 예외를 발생한다. ✅ |
There was a problem hiding this comment.
코드를 작성하다 이해한 부분을 수정해준 것도 좋네요!! 커밋단위가 잘 쪼개져있다고 느껴져요
| } | ||
|
|
||
| // TODO: 현재는 LocalDateTime.now() 호출로 Mocking 불가. 프로덕션 코드 수정 후 테스트 적용 | ||
| @Test |
There was a problem hiding this comment.
따로 Todo와 Disable을 빼주신 것이 좋네요!!
혹시 LocaldDateTime.now()같은 staticMethod를 모킹하는게 어려우신거라면,
StaticMock을 찾아보시는 것도 좋을 것 같아요.
There was a problem hiding this comment.
근데 그와 별개로, LocalDateTime.now()를 모킹할 일이 있나요? 저는 통합테스트로 진행해서 그러한 문제를 겪진 못해서요.
There was a problem hiding this comment.
StaticMock 키워드 공유해주셔서 감사합니다!
그런데 staticMock을 하려면 의존성을 추가해야하네요..
굳이 그렇게까지 하고 싶진 않아서, 일단 여기는 이대로 둘까합니다. 괜찮을까요?
OrderService#create 메서드의 로직을 따라가다보면, order.setOrderedTime(LocalDateTime.now());이 있습니다.
이 부분을 테스트하고자 했어요!
There was a problem hiding this comment.
아 staticMock 적용여부는 자유입니다. 그거는 도기가 편하신대로 해주시면 될 것 같아요!!
아 근데, 제가 잘못 알려드렸네요, StaticMock이 아니라 MockedStatic이었어요 해당 클래스는 mockito라이브러리에 있어서 별도의 의존성을 추가하진 않아도 될거에요.
와우 값까지 검증하려고 하셨던 거군요 멋지네요.
There was a problem hiding this comment.
현재 의존하는 Mockito 4 버전대는 추가로 mockito-inline이라는 의존성을 추가해줘야 하나봐요! (Reference)
어차피 리팩터링할거고, 리팩터링으로 테스트를 쉽게 만드는 게 더 유의미하다 생각해서 다음 단계에서 해당 부분 테스트하겠습니다!
| OrderTable result = tableService.create(orderTable); | ||
|
|
||
| // then | ||
| assertThat(result.getId()).isNull(); |
There was a problem hiding this comment.
테이블을 저장하는데, getId와, getTableGroupId가 null이면 동작을 검증하기 어려울 것 같아요.
어떻게 검증할 수 있을까요?
| orderTable.setNumberOfGuests(10); | ||
|
|
||
| when(orderTableDao.findById(orderTableId)).thenReturn(Optional.of(orderTable)); | ||
| when(orderTableDao.save(any(OrderTable.class))).thenReturn(orderTable); |
There was a problem hiding this comment.
여기서 findById를 하는 객체가 실제 파라미터로 들어간 객체와 동일하군요.
이미 반환될 떄 부터 setNumberOfGuest가 두 객체가 동일해서 제대로 검증이 어려울 것 같아요.
만약에, 이 tableService의 changeNumberOfGuests에서 orderTalbe.setNumberOfGuests가 내부적으로 호출되지 않아도, 이 테스트는 통과할 것처럼 보여요.
도기는 어떻게 생각하시나요?
There was a problem hiding this comment.
오.. 진짜 정확하게 보셨군요....!
변명아닌 변명을 대자면, 시간에 쫓겨 반복적인 테스트. 그것도 mock 테스트를 진행하다보니 이런 실수가 조금 있는 것 같네요 😅
꼼꼼하게 봐주셔서 정말 감사합니다 홍실 👍
| when(orderTableDao.findAllByIdIn(orderTableIds)).thenReturn(tableGroup.getOrderTables()); | ||
|
|
||
| when(tableGroupDao.save(any(TableGroup.class))).thenReturn(tableGroup); | ||
| when(orderTableDao.save(orderTable1)).thenReturn(orderTable1); |
There was a problem hiding this comment.
이 부분은 꼭 stubbing을 해주지 않아도 될 것 같아요.
혹시 도기가 따로 stubbing을 해주신 이유가 있나요?
There was a problem hiding this comment.
음.. 제가 봐도 불필요한데 왜 했을까요..?
제거하겠습니다 🤣
|
|
||
| // then | ||
| assertThat(result.getNumberOfGuests()).isEqualTo(orderTable.getNumberOfGuests()); | ||
| assertThat(result.getNumberOfGuests()).isNotEqualTo(before.getNumberOfGuests()); | ||
| assertThat(result.getNumberOfGuests()).isEqualTo(after.getNumberOfGuests()); |
There was a problem hiding this comment.
이번 리뷰를 하면서 느끼지만, 도기는 mocking을 이용해서 테스트를 정말 읽기 좋게 짜주시는 것 같아요.
공백이 진짜 적절해서 무엇을 검증하려는 지 바로 보이네요
안녕하세요 홍실! 이렇게 리뷰어-리뷰이로 오랜만에 뵙게 됐네요!!
요구 사항에 충실히 1단계 진행해보았습니다.
서비스 계층과 컨트롤러 모두 테스트 작성했고, 모두 Mocking을 적용했습니다.
그럼 마지막 미션 잘 부탁드려요 ㅎㅎ