3단계 - 테스트를 통한 코드 보호 #258
3단계 - 테스트를 통한 코드 보호 #258
Conversation
상품을 포함하는 메뉴의 진열여부 변경 기준이 상품들의 가격을 합산 하지 않고 있었음
fixture 객체를 수정하여 다른 테스트에 영향을 끼쳐 fixture 객체를 직접 수정하지 않도록 변경
fixture 객체를 수정하여 다른 테스트에 영향을 끼쳐 fixture 객체를 직접 수정하지 않도록 변경
mock 대신 fake 객체 활용
mock 대신 fake 객체 활용 DisplayName을 요구사항과 일치하도록 변경
mock 대신 fake 객체 활용 DisplayName을 요구사항과 일치하도록 변경
mock 대신 fake 객체 활용
mock 대신 fake 객체 활용
|
안녕하세요 😃 3단계 리뷰요청이 좀 늦었지만 잘 부탁드립니다 (__) |
There was a problem hiding this comment.
테스트 픽스처, 가짜 객체 등을 사용하여 테스트 코드를 잘 작성해 주셨습니다. 👍
테스트 코드를 작성하는 이유 중에는 '테스트 코드를 통해 요구 사항을 이해할 수 있다'와 '클라이언트가 객체를 사용하는 방법에 관한 적절한 코드 예제를 제공하는 것'이 있습니다.
그 외에도 몇 가지 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.
아래의 글이 도움 되면 좋겠어요.
https://blog.kingbbode.com/52
| @@ -179,4 +180,47 @@ public Order complete(final UUID orderId) { | |||
| public List<Order> findAll() { | |||
| return orderRepository.findAll(); | |||
| } | |||
|
|
|||
|
|
|||
| static class OrderTypeNotExistException extends IllegalStateException { | |||
|
|
||
| static class OrderTypeNotExistException extends IllegalStateException { | ||
| public OrderTypeNotExistException() { | ||
| super("주문 유형이 올바르지 않습니다"); |
| static class OrderTypeNotExistException extends IllegalStateException { | ||
| public OrderTypeNotExistException() { | ||
| super("주문 유형이 올바르지 않습니다"); | ||
| } | ||
| } | ||
|
|
||
| static class OrderLineItemNotExistException extends IllegalStateException { | ||
| public OrderLineItemNotExistException() { | ||
| super("주문 상품이 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| static class OrderLineItemNotMatchException extends IllegalStateException { | ||
| public OrderLineItemNotMatchException() { | ||
| super("등록되지 않은 메뉴는 주문할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| static class OrderInvalidQuantityException extends IllegalStateException { | ||
| public OrderInvalidQuantityException(long quantity) { | ||
| super("최소 주문 수량은 0개 이상입니다. 주문 수량 : " + quantity); | ||
| } | ||
| } | ||
|
|
||
| static class OrderDisplayException extends IllegalStateException { | ||
| public OrderDisplayException() { | ||
| super("진열되지 않은 메뉴는 주문할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| static class OrderLineItemPriceException extends IllegalArgumentException { | ||
| public OrderLineItemPriceException(String menu, long menuPrice, long requestPrice) { | ||
| super("가격이 일치하지 않습니다. 메뉴명: " + menu + ", 메뉴 가격: " + menuPrice + ", 지불 가격: " + requestPrice); | ||
| } | ||
| } | ||
|
|
||
| static class OrderDeliveryAddressException extends IllegalArgumentException { | ||
| public OrderDeliveryAddressException() { | ||
| super("배달 주소가 없습니다."); | ||
| } | ||
| } |
| Optional<MenuGroup> findById(UUID menuGroupId); | ||
| } | ||
|
|
||
| interface JpaMenuGroupRepository extends MenuGroupRepository, JpaRepository<MenuGroup, UUID> { |
There was a problem hiding this comment.
아래의 Google Java Style Guide를 참고해 주세요.
Each top-level class resides in a source file of its own.
| class DefaultMenuGroupServiceTest { | ||
|
|
||
| private final MenuGroupRepository menuGroupRepository = new InMemoryMenuGroupRepository(); | ||
| private DefaultMenuGroupService menuGroupService; |
There was a problem hiding this comment.
인터페이스를 타입으로 사용하는 습관을 길러 두면 프로그램이 훨씬 유연해질 거예요. 인터페이스와 다형성에 대해 알아보고 아래의 글도 참고하면 좋을 것 같아요.
https://stackoverflow.com/questions/11683044/return-arraylist-or-list/11683073
| @Test | ||
| void createMenuGroupNotExistException() { | ||
| //given | ||
| Menu 메뉴_그룹_없는_메뉴 = 메뉴_생성(11_000); |
There was a problem hiding this comment.
테스트 조건이 명확하게 드러나지 않은 것 같아요. 어떤 상황을 테스트하고 싶은지 코드로 표현해 보세요.
| assertThatThrownBy(actual).isInstanceOf(IllegalArgumentException.class); | ||
| } | ||
|
|
||
| @DisplayName("메뉴는 수량이 부족한 상품을 포함할 수 없다.") |
There was a problem hiding this comment.
"메뉴는 수량이 부족한 상품을 포함할 수 없다."라는 이름은 적절할까요?
| @Test | ||
| void create() { | ||
| //given | ||
| OrderTable 신규_테이블 = 식탁_생성("3번"); |
There was a problem hiding this comment.
아래의 코드로 개선해 보면 어떨까요?
| OrderTable 신규_테이블 = 식탁_생성("3번"); | |
| OrderTable 신규_식탁 = 식탁_생성("3번"); |
| @DisplayName("가격을 변경할 수 있다. 가격이 해당 상품을 포함하는 메뉴의 가격보다 크면 메뉴를 진열하지 않는다.") | ||
| @ParameterizedTest(name = "변경할 가격: [{0}], 진열 여부: [{1}]") | ||
| @CsvSource(value = { | ||
| "8000, false", | ||
| "15000, true" | ||
| }) | ||
| void changePrice(long price, boolean expectedDisplayed) { | ||
| //given | ||
| Product 신규_상품 = productRepository.save(상품_생성(12_000)); | ||
|
|
||
| MenuProduct menuProduct = new MenuProduct(); | ||
| menuProduct.setProduct(신규_상품); | ||
| menuProduct.setProductId(신규_상품.getId()); | ||
| menuProduct.setQuantity(1); | ||
|
|
||
| Menu menu = new Menu(); | ||
| menu.setDisplayed(true); | ||
| menu.setMenuProducts(Arrays.asList(menuProduct, 콜라_1개)); | ||
| menu.setPrice(BigDecimal.valueOf(11_000L)); | ||
| menuRepository.save(menu); | ||
|
|
||
| Product 변경할_금액 = 상품_생성(price); | ||
|
|
||
| //when | ||
| Product product = productService.changePrice(신규_상품.getId(), 변경할_금액); | ||
|
|
||
| //then | ||
| assertAll( | ||
| () -> assertThat(product.getPrice()).isEqualTo(BigDecimal.valueOf(price)), | ||
| () -> assertThat(menu.isDisplayed()).isEqualTo(expectedDisplayed) | ||
| ); | ||
| } |
There was a problem hiding this comment.
일반적으로 테스트마다 하나의 논리적 개념을 테스트하는 것이 좋아요. 아래의 글이 도움 되면 좋겠어요.
https://softwareengineering.stackexchange.com/questions/7823/is-it-ok-to-have-multiple-asserts-in-a-single-unit-test
There was a problem hiding this comment.
링크 감사합니다!
레거시 코드의 여러 책임을 테스트 코드에서도 동일하게 검증하고,
리팩토링할 때 테스트 코드도 같이 리팩토링 하면 되지 않을까 생각했었는데, 잘못된 생각이었네요 😭
테스트 코드는 프로덕션 코드를 검증하기도 하지만 리팩토링을 위한 안전 장치이기도 한데
테스트 코드를 레거시 코드와 동일한 형태로 만든다면 무의미한 안전장치가 될 것이라는 생각이 들었습니다. 😀
| import org.springframework.boot.test.context.TestConfiguration; | ||
| import org.springframework.context.annotation.Bean; | ||
|
|
||
| @TestConfiguration |
There was a problem hiding this comment.
@TestConfiguration 👍 이번 기회에 @Configuration과 @TestConfiguration의 차이점을 알아보면 어떨까요?
There was a problem hiding this comment.
SpringBootTest와 같이 Configuration를 읽어 들이는 테스트 환경에서 TestConfiguration은 제외되는 걸 확인했습니다. 😄
Configuration이 필수 설정이라면 TestConfiguration은 선택 설정이라고 할 수 있을 것 같아요.
필수 설정은 모두 적용되지만 선택 설정은 필요한 설정만 골라서 적용하거나, 여러 테스트에서도 적용할 수 있고, 필수 설정의 일부분을 재정의해서 활용할 수 있을 것 같아요!
단위테스트에서도 로드되지 않는 빈을 추가하여 원하는 테스트 환경을 만들 수도 있네요 😄
Controller 테스트에서 가짜객체를 활용해 테스트 해보고 싶었을 뿐인데 피드백 안주셨으면
TestConfiguration은 단순히 빈을 등록하기 위해 사용하는 거라고 생각하고 넘어 갔을 것 같아요.
감사합니다!
순서와 상관없이 elements 일치 여부를 검증할 수 있다.
repository의 save는 수정 기능도 포함하고 있다. ID가 없을 경우에만 부여한다.
enum은 equals로 비교할 필요 없다.
테스트의 의도를 명확하게 표현할 수 있는 변수명으로 변경
테스트의 의도를 명확하게 표현할 수 있는 테스트 이름과 변수명으로 변경
테이블 -> 식탁 일치 하지 않은 용어로 혼란을 줄 수 있는 요소 제거.
테스트는 하나의 논리적 개념만 검증한다. 레거시 코드에서 여러 책임을 수행하고 있더라도 테스트 코드는 하나의 논리적 개념만 검증하도록 해야 한다.
레거시 코드의 순서대로 테스트를 하고 있다. 레거시 코드가 변경되면 테스트의 성공여부를 확신할 수 없다. 즉 안전한 테스트 코드라고 할 수 없다. 레거시 코드가 변경되어도 테스트는 온전한 기능을 해야한다. 순서에 의존하지 않아도 되는 코드에는 선행조건을 만들지 말고 독립적인 실행을 보장해야 한다.
| Optional<Order> findById(UUID orderId); | ||
| boolean existsByOrderTableAndStatusNot(OrderTable orderTable, OrderStatus status); |
There was a problem hiding this comment.
IntelliJ IDEA의 코드 자동 정렬 기능(⌥⌘L, Ctrl+Alt+L)을 사용하면 더 깔끔한 코드를 볼 수 있을 거예요.
| Optional<Order> findById(UUID orderId); | |
| boolean existsByOrderTableAndStatusNot(OrderTable orderTable, OrderStatus status); | |
| Optional<Order> findById(UUID orderId); | |
| boolean existsByOrderTableAndStatusNot(OrderTable orderTable, OrderStatus status); |
| assertAll( | ||
| () -> assertThat(menuGroups).hasSize(2), | ||
| () -> assertThat(menuGroups).containsExactlyInAnyOrder(세트메뉴, 추천메뉴) | ||
| ); |
There was a problem hiding this comment.
아래의 코드로 개선해 보면 어떨까요?
| assertAll( | |
| () -> assertThat(menuGroups).hasSize(2), | |
| () -> assertThat(menuGroups).containsExactlyInAnyOrder(세트메뉴, 추천메뉴) | |
| ); | |
| assertThat(menuGroups).containsExactlyInAnyOrder(세트메뉴, 추천메뉴); |
| import kitchenpos.domain.MenuGroup; | ||
| import kitchenpos.domain.MenuProduct; | ||
|
|
||
| public class MenuBuilder { |
| .withMenuGroup(세트_메뉴) | ||
| .withMenuGroupId(세트_메뉴.getId()) |
There was a problem hiding this comment.
아래 코멘트에 대해 고민해 봐요.
#258 (comment)
| .withMenuGroup(세트_메뉴) | |
| .withMenuGroupId(세트_메뉴.getId()) | |
| .withMenuGroupId(세트_메뉴.getId()) | |
| .withMenuProducts(Collections.emptyList()) |
| MenuGroup 세트_메뉴 = menuGroupRepository.save(세트메뉴); | ||
| productRepository.save(맛초킹); | ||
| productRepository.save(콜라); |
There was a problem hiding this comment.
다른 테스트 코드에도 필요한 테스트 조건인 것 같아요. 아래 코멘트에 대해 고민해 봐요.
#258 (comment)
| @Test | ||
| void createOrderTypeException() { | ||
| //given | ||
| Order order = new Order(); |
There was a problem hiding this comment.
아래 코멘트에 대해 고민해 봐요.
#258 (comment)
containsExactly 는 hasSize를 포함한다.
Preferences - Tools - Actions to Save Reformat code, Optimize imports 활성화
테스트 조건(given)이 명확하지 않아 테스트의 의도를 파악하기 어려움이 있었음. Builder 패턴을 적용하여 시간적 결합과 의도를 모두 표현할 수 있도록 변경
시간적 결합을 제거하고 테스트의 의도를 코드로 표현
|
시간적 결합을 찾기 위해서 주문 생성에서 발생할 수 있는 예외를 커스텀 예외로 분리했습니다. |
안녕하세요 리뷰어님 😄
3단계 리뷰 요청드립니다!
요구사항대로만 테스트 코드 작성하면 되겠지 생각하고 시작했는데 결코 쉽지 않았습니다. 😢
특히 요구사항대로 테스트 코드를 작성하고 실행 후 테스트를 실패하게 되었을 때 어느 부분(레거시 코드, 요구사항, 테스트 코드)에서 문제가 있어서
테스트가 실패한 건지 찾는 과정이 힘들었습니다.
stubbing(given()) 코드가 중복되는 게 많다고 생각되고, Fixture를 활용했으나 유용하게 활용했는지 잘 모르겠습니다.
stubbing, fixture에 대해서도 피드백 주시면 감사하겠습니다. (__)