3단계 - 테스트를 통한 코드 보호 #823
3단계 - 테스트를 통한 코드 보호 #823
Conversation
|
안녕하세요 규태님 지난 피드백에서 궁금한 내용이 있는데요. |
| @Nested | ||
| class ProductCreator { |
There was a problem hiding this comment.
@Nested를 잘 활용해보셨군요! 기능과 정책이 한 곳에 묶여 가독성이 훨씬 좋아진 것 같아요 😊 💯
| @DisplayName("단일 상품으로 메뉴를 등록할 수 있다") | ||
| @Test | ||
| void createMenuByProduct() { | ||
| MenuProduct chickenBurger = MenuProductFixture.createMenuProduct(BURGER_PRODUCT_ID, 1); | ||
| Menu menu = MenuFixture.createMenu(MENU_GROUP_ID, "치킨버거", new BigDecimal(7000), List.of(chickenBurger)); | ||
|
|
||
| Menu resultMenu = menuService.create(menu); | ||
|
|
||
| assertAll( | ||
| () -> assertThat(resultMenu.getName()).isEqualTo("치킨버거"), | ||
| () -> assertThat(resultMenu.getPrice()).isEqualTo(new BigDecimal(7000)), | ||
| () -> assertThat(resultMenu.isDisplayed()).isFalse(), | ||
| () -> assertThat(resultMenu.getMenuProducts()).hasSize(1), | ||
| () -> assertThat(resultMenu.getMenuGroup()).isNotNull(), | ||
| () -> assertThat(resultMenu.getMenuGroup().getId()).isEqualTo(MENU_GROUP_ID) | ||
| ); | ||
| } | ||
|
|
||
| @DisplayName("여러 상품으로 조합하여 하나의 메뉴를 등록할 수 있다") | ||
| @Test | ||
| void createMenuByProducts() { |
There was a problem hiding this comment.
단일과 여러 상품의 테스트를 분리하신 의도가 궁금해요!
답변남겨주시면 감사하겠습니다 😃
There was a problem hiding this comment.
요구사항 정의할 때, 아래와 같이 정리해서
- 단일 상품으로 이루어진 메뉴를 생성할 수 있다
- 복수 상품으로 이루어진 메뉴를 생성할 수 있다
이 의미가 드러나도록 테스트가 분리하였습니다.
다시 생각해보니 제가 요구사항을 명료하게 정리를 못해서 발생한 문제로 보입니다.
메뉴는 1가지 이상의 상품으로 등록 가능하다로 요약해서 하나의 테스트로 정리 가능할 것 같습니다!!
| import java.util.*; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| public class InMemoryMenuGroupRepository implements MenuGroupRepository { |
There was a problem hiding this comment.
Fake를 활용해보셨군요! 처음 요청주셨을 때 Mock을 활용했었는데 Mock과 비교해서
각각의 장단점을 정리해보시면 좋을 것 같아요! :) 개인적으로 해보셔도 되고, 남겨주셔도 좋습니다!
There was a problem hiding this comment.
제가 생각하기엔
비즈니스 코드는 fake가 좋아보이고
외부 환경 의존도가 높을 경우는 mock이 좋아보입니다.
Fake
-
장점
- 실제 코드를 쓰는 것처럼 테스트 코드 작성 가능하다
- 실제와 유사하게 동작하기 때문에 실제 환경과 흡사한 테스트 가능하다
-
단점
- Fake 객체 내부 구현에 비용이 든다
- repository fake 객체와 같이 데이터를 다루는 경우, 많은 데이터를 다룰 때 memory leak에 주의해야 한다
Mock
-
장점
- Fake 객체처럼 구현 없이 객체의 행위를 검증 가능하다
-
단점
- 내부 코드에 대한 이해가 있어야 제대로 mocking할 수 있다
- 테스트 코드의 복잡도가 높아질 수 있다
- mockito 같은 라이브러리에 의존적이다
There was a problem hiding this comment.
잘 정리해주셨어요💯💯
이제 테스트 코드를 작성하실 때 좀 더 명확한 기준을 가지고 테스트 더블을 선택할 수 있으실 것 같아요!
| import java.math.BigDecimal; | ||
| import java.util.UUID; | ||
|
|
||
| public class FakeKitchenridersClient implements KitchenridersClient { |
There was a problem hiding this comment.
현재 한 패키지 안에 Fake 클래스들과 테스트 클래스들이 함께 있는데, 적절히 패키지를 분리해보면 어떨까요 😄
There was a problem hiding this comment.
PurgomalumClient, KitchenridersClient가 있는 infra 패키지에 fake 클래스 생성했습니다
| Order deliveryOrderRequest = createDeliveryOrder(List.of(orderLineItem), "경기도 고양시..XX동 XXX호", LocalDateTime.now()); | ||
| Order order = orderService.create(deliveryOrderRequest); | ||
|
|
||
| ReflectionTestUtils.setField(order, "status", orderStatus); |
There was a problem hiding this comment.
코멘트 남겨 주신 부분 여기와 함께 말씀드릴게요!
말씀하신대로, 일반적으로 생성자를 통해 객체를 초기화하고, 제한된 행위를 통해서만 스스로의 상태를 관리하는 게 맞습니다. setter를 지양하는 이유가 이러한 무결성을 해친다는 점이죠!
하지만 지금과 같이 전제 조건이 선행된 후 테스트가 가능한 경우들이 있습니다. 이 경우
- 전제 조건들을 순차적으로 진행한다.
- 전제 조건과 같은 상태를 만들어낸다.
위와 같은 방법들이 있고, 이 외에도 더 있을 수 있다고 생각합니다. ㅎㅎ
2번과 같이 상태를 인위적으로 만들어내려면 setter가 필요할텐데, 저는 테스트코드를 위해 프로덕션 코드를 수정하는 행위는 지양해야한다고 생각해요 😄 그래서 제한적으로 테스트 코드 내에서 인위로 상태를 변경해야한다면 ReflectionTestUtils.setField를 사용하는 게 맞다고 생각하고요!
슬기님이 작성해주신 OrderServiceTest쪽은 제가 가리킨 코드 영역과 아래 쪽에 작성된 다음 코드처럼 두 가지를 다 사용하신거로 보여요!
private Order createDeliveredOrder(Order deliveryOrder) {
Order order = orderService.create(deliveryOrder);
Order accept = orderService.accept(order.getId());
Order serve = orderService.serve(accept.getId());
Order startDelivery = orderService.startDelivery(serve.getId());
return orderService.completeDelivery(startDelivery.getId());
}
private Order createServedOrder(Order orderRequest) {
Order takeOutOrder = orderService.create(orderRequest);
Order accept = orderService.accept(takeOutOrder.getId());
return orderService.serve(accept.getId());
}
두 경우를 나눠서 작성하신 의도도 궁금하기도하고, 슬기님은 각각의 트레이드 오프에서 어떤 걸 선택하고 싶으신지도 궁금해요! 해당 부분은 DM으로 답변 주실 수 있을까요? 이야기가 조금 더 길어질 수도 있을 것 같아서요!
There was a problem hiding this comment.
이 부분은 dm에서 자세히 설명드렸습니다.
테스트의 유지보수 위해서 2. 전제 조건과 같은 상태를 만들어낸다.로 통일하도록 하겠습니다
ReflectionTestUtils.setField을 제거하고 orderRepository.save로 orderStatus 셋팅하는 방향으로 수정했습니다.
|
코드 리뷰 다시 부탁드립니다. orderStatus의 변경을 위해서 ReflectionTestUtils.setField는 사용하지 않고 |
| import java.util.*; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| public class InMemoryMenuGroupRepository implements MenuGroupRepository { |
There was a problem hiding this comment.
잘 정리해주셨어요💯💯
이제 테스트 코드를 작성하실 때 좀 더 명확한 기준을 가지고 테스트 더블을 선택할 수 있으실 것 같아요!
| @@ -0,0 +1,43 @@ | |||
| package kitchenpos.application; | |||
There was a problem hiding this comment.
Fake Repository도 fake 패키지로 이동하면 어떨까요? 😃
fake 내 repository 패키지로 별도로 분리해둬도 좋을 것 같아요!
| @DisplayName("메뉴는 1가지 이상의 상품으로 등록 가능하다") | ||
| @Test | ||
| void createMenuByProducts() { |
There was a problem hiding this comment.
잘 통합해주셨어요! 특별한 의도가 없다면 저도 묶는 게 좋을 것 같았거든요 ㅎㅎ
반대로 비어있거나 없을 경우에 대한 실패 테스트도 작성해주시면 좋을 것 같아요!
| - [ ] 메뉴판 전시 여부를 X로 변경한다 | ||
| - [ ] 모든 메뉴를 조회할 수 있다 | ||
| - [x] 메뉴를 등록할 수 있다 | ||
| - [x] 메뉴는 1가지 이상의 상품으로 등록 가능하다 |
| private Order createTakeOutOrder(List<OrderLineItem> orderLineItems, LocalDateTime orderDateTime) { | ||
| return createTakeOutOrder(orderLineItems, null, orderDateTime); | ||
| } | ||
|
|
||
| private Order createDeliveryOrder(List<OrderLineItem> orderLineItems, String deliveryAddress, LocalDateTime orderDateTime) { | ||
| return createDeliveryOrder(orderLineItems, deliveryAddress, null, orderDateTime); | ||
| } | ||
|
|
||
| private Order createEatInOrder(List<OrderLineItem> orderLineItems, UUID orderTableId, LocalDateTime orderDateTime) { | ||
| return createEatInOrder(orderLineItems, orderTableId, null, orderDateTime); | ||
| } | ||
|
|
||
| private Order createTakeOutOrder(List<OrderLineItem> orderLineItems, OrderStatus orderStatus, LocalDateTime orderDateTime) { | ||
| return OrderFixture.createOrder(OrderType.TAKEOUT, orderLineItems, null, null, orderStatus, orderDateTime); | ||
| } | ||
|
|
||
| private Order createDeliveryOrder(List<OrderLineItem> orderLineItems, String deliveryAddress, OrderStatus orderStatus, LocalDateTime orderDateTime) { | ||
| return OrderFixture.createOrder(OrderType.DELIVERY, orderLineItems, deliveryAddress, null, orderStatus, orderDateTime); | ||
| } | ||
|
|
||
| private Order createEatInOrder(List<OrderLineItem> orderLineItems, OrderTable orderTable, OrderStatus orderStatus, LocalDateTime orderDateTime) { | ||
| return OrderFixture.createOrder(OrderType.EAT_IN, orderLineItems, orderTable, orderStatus, orderDateTime); | ||
| } | ||
|
|
||
| private Order createEatInOrder(List<OrderLineItem> orderLineItems, UUID orderTableId, OrderStatus orderStatus, LocalDateTime orderDateTime) { | ||
| return OrderFixture.createOrder(OrderType.EAT_IN, orderLineItems, orderTableId, orderStatus, orderDateTime); | ||
| } |
There was a problem hiding this comment.
OrderFixture로 통일하는 방향으로 수정했습니다.
매장 식사 주문 생성 시, 주문 테이블 참조가 객체 참조 또는 id 참조 두 가지 방식이 혼재되어 있어서
불필요한 메소드가 생겼습니다.
- createEatInOrder(...
OrderTable orderTable) - createEatInOrder(...
UUID orderTableId)
| Order takeoutOrder = orderService.create(takeOutRequest); | ||
| Order eatInOrder = orderService.create(eatInRequest); | ||
| //when | ||
| ReflectionTestUtils.setField(takeoutOrder, "status", orderStatus); | ||
| ReflectionTestUtils.setField(eatInOrder, "status", orderStatus); |
There was a problem hiding this comment.
fixture를 좀 더 활용해보면 어떨까해요!
예를들어 createEatInOrder를 더 세분화해서 createServedEatInOrder를 만들어낼 수도 있을 것 같아요!
다른 코드 영역도 마찬가지입니다!
There was a problem hiding this comment.
fixture를 아래와 같이 수정했습니다.
Delivery, EatIn, TakeOut static inner class를 생성하고
각 static inner class에서 아래 두 메소드를 가지고 있는 방식으로 수정했습니다
- status가 null인 메소드
initializeOrder - status를 set하는 메소드
createOrderByStatus
| order.setId(UUID.randomUUID()); | ||
| order.setType(orderType); |
There was a problem hiding this comment.
Fixture에 사용된 setter도 setField를 적용해보면 어떨까요? 다른 픽스쳐들도 동일합니다!
|
피드백 반영했습니다. |
| ); | ||
| } | ||
|
|
||
| @DisplayName("메뉴를 빈 상품 또는 상품 없이 등록하면 예외가 발생한다") |
|
|
||
| public class OrderFixture { | ||
|
|
||
| public static class Delivery { |
There was a problem hiding this comment.
내부 클래스로 픽스쳐를 풀어보셨군요 👍🏻👍🏻
테스트 코드에서 생성하던 책임을 픽스쳐로 응집하여 가독성이 더 좋아진 것 같아요👀
안녕하세요 규태님
코드 리뷰 잘 부탁드립니다.
테스트코드는 서비스 파일 위주로 작성했으며 아래 항목은 return 도 없고 비어있는 메소드가 테스트 제외했습니다.