3단계 - 테스트를 통한 코드 보호 by NNIIE · Pull Request #519 · next-step/ddd-legacy

NNIIE · 2023-08-26T21:11:43Z

이번 미션도 잘부탁드립니다. 🙇🏼‍♂️

wkdchdaud123

안녕하세요 용석님 🙇
꼼꼼하게 테스트 코드를 잘 작성해주셨어요. 💯
생각하고 적용할 것 우선 몇 가지 코멘트 드린 것 전체적으로 확인, 고민, 적용 부탁드려요 😄

wkdchdaud123 · 2023-08-27T14:57:37Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+    @DisplayName("메뉴그룹은 식별키, 이름을 가진다.")
+    void menuGroup() {
+        assertAll(
+                () -> assertThat(menuGroup.getName()).isEqualTo(MENU_GROUP_NAME),
+                () -> assertThat(menuGroup.getId()).isEqualTo(MENU_GROUP_ID)
+        );
+    }


전체적으로 모든 Test Class에 동일하게 있군요. (전체적으로 확인, 고민 부탁드려요)
단순히 getter, setter를 테스트를 하려는 의도인 것 같은데 이 테스트는 정말 필요할까요 필요하다면 어떤 가치가 있을까요?

메뉴 그룹에 식별키와 이름을 가진다는 것은 속성을 얘기하는 것 아닐까요? 😄

wkdchdaud123 · 2023-08-27T15:11:23Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+    @BeforeEach
+    void setUp() {
+        menuGroup = new MenuGroup();
+        menuGroup.setId(MENU_GROUP_ID);
+        menuGroup.setName(MENU_GROUP_NAME);
+    }


모든 테스트를 보면 @BeforeEach에서 필요한 값들을 Setting하고 초기화 하는데 대신에 픽스처 클래스를 분리해서 사용해보면 어떨까요? 😄 전체적으로 확인, 고민 부탁드려요

https://jojoldu.tistory.com/611
https://thalals.tistory.com/375#google_vignette

wkdchdaud123 · 2023-08-27T15:25:30Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+
+        @ParameterizedTest
+        @NullAndEmptySource
+        @DisplayName("이름은 비어있거나 공백이면 예외가 발생한다.")


최대한 요구사항 작성 시 사용한 용어와 비슷하게 가는 것이 추후 확인하기도 좋을 것 같아요 😄

전체적으로 한번 고민하고 확인 후 적용해보시면 좋을 것 같아요.

wkdchdaud123 · 2023-08-27T15:26:34Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+        List<MenuGroup> findAllMenuGroups = menuGroupService.findAll();
+
+        // Then
+        assertThat(findAllMenuGroups.size()).isEqualTo(2);


전체적으로 List를 확인 하는 곳에서도 isEqulaTo를 많이 사용하셨는데 isEqulaTo 대신 hasSize 또는 containsOnly를 사용해볼 수 있을 것 같아요. 😄
테스트 메소드의 의미가 더 명확하지 않나요?

wkdchdaud123 · 2023-08-27T15:30:34Z

src/test/java/kitchenpos/application/OrderTableServiceTest.java

+            assertAll(
+                    () -> assertThat(result.getNumberOfGuests()).isEqualTo(0),
+                    () -> assertThat(result.isOccupied()).isEqualTo(false)
+            );
+        }


isZero와 isFalse를 사용할 수 있겠네요 😄
이것도 전체적으로 이렇게 사용하신 곳이 좀 있네요.
의도하신 걸까용? 😄

의도했다기 보다는.. 급하게 미션 하면서 별 생각 없이 작성했던거 같아요
말씀하신대로 변경 했습니다

wkdchdaud123 · 2023-08-27T15:31:41Z

src/test/java/kitchenpos/application/OrderTableServiceTest.java

+        List<OrderTable> findAllOrderTables = orderTableService.findAll();
+
+        // Then
+        assertThat(findAllOrderTables.size()).isEqualTo(2);


이쪽도 동일하게
isEqulaTo 대신 hasSize 또는 containsOnly를 사용해볼 수 있을 것 같아요. 😄

wkdchdaud123 · 2023-08-27T15:39:33Z

src/test/java/kitchenpos/application/ProductServiceTest.java

+        @Test
+        @DisplayName("미리 존재하는 상품이 아니면 예외가 발생한다.")
+        void changePrice_3() {
+            when(productRepository.findById(eq(PRODUCT_ID))).thenReturn(Optional.empty());


이쪽 파일들을 보면 ArgumentMatchers eq를 사용하는 곳들이 몇 개 있네요.
정말 필요한가요? 😄

필요하지 않습니다!
이부분 제거했어요 😀

… / Order)

wkdchdaud123

피드백 빠르게 잘 반영해주셨어요. 💯 👍
몇 가지 코멘트 남겨두었어요. 😄
확인 부탁드려요

wkdchdaud123 · 2023-08-28T14:22:15Z

build.gradle

+jacocoTestReport {
+    reports {
+        html.enabled true
+        xml.enabled false
+        csv.enabled false
+
+        html.destination file("jacoco/jacocoHtml")
+        xml.destination file("jacoco/jacoco.xml")
+    }
+}
+
+jacocoTestCoverageVerification {
+    violationRules {
+        rule {
+            element = 'CLASS'
+        }
+    }
+}


jacoco 추가를 하셨군요 👍
현재 xml.enabled가 false인 상태라 xml report는 없긴 하겠네요 😄

처음 사용해봤습니다.
html 로 간단하게 사용해봤어요 ㅎㅎ

wkdchdaud123 · 2023-08-28T14:29:09Z

src/test/java/kitchenpos/application/MenuServiceTest.java

+        }
+
+        @Test
+        @DisplayName("메뉴상품의 수량은 0 이하일 수 없다.")


이쪽은 정말 메뉴 상품의 수량은 0 이하일 수 없는지 테스트를 하는게 맞는지 다시 확인해볼 수 있을까요? 😄

wkdchdaud123 · 2023-08-28T15:00:12Z

src/test/java/kitchenpos/application/MenuServiceTest.java

+        void create_2() {
+            // Given
+            Menu menu = createMenu();
+            when(menuGroupRepository.findById(any())).thenReturn(Optional.empty());


전체적으로 이것도 한번 고민해보면 좋을 것 같아요.
Given절에 when이 들어가니 어떠신가요?
대신에 BDDMockito의 given을 사용하면 when절과 구분도 되어 가독성도 더 좋아질 수 있을 것 같은데 어떠신가요? 😄

https://tecoble.techcourse.co.kr/post/2020-09-29-compare-mockito-bddmockito/

wkdchdaud123 · 2023-08-28T15:21:05Z

src/test/java/kitchenpos/application/MenuServiceTest.java

+            @DisplayName("비어있는 경우")
+            void create_1_1() {
+                // When
+                Menu menu = createMenuWithPrice(null);


@NullSource를 사용할 수도 있겠네요 😄 (전체적으로 몇군데 보이긴 합니다)

wkdchdaud123 · 2023-08-28T15:34:36Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+
+        @Test
+        @DisplayName("등록")
+        void create_1() {


지금은 수정을 안하셔도 됩니다만
아무리 DisplayName이 있더라도 메소드 이름도 적절하게 지어주는 것이 좋을 것 같다고 생각해요.

어떠신가요? 실무에서도 비슷한 방식으로 진행하시는지 궁금합니다 😄

Clean Code 1장에서 깨진 창문 이론을 첨부해봤어요.
깨진 창문 이론 : 창문이 깨진 건물은 누구도 신경 쓰지 않는다. 사람들은 관심을 끊고, 창문이 더 깨져도 상관하지 않는다. 마침내 자발적으로 창문을 깨고, 방치하며, 쇠퇴하는 과정이 시작된다.

실무에선 DisplayName과 메서드네이밍 모두 신경써서 사용중입니다.
나름 급하게 미션 진행한다고 이렇게 작성했는데 결국 나중에 손봐야 할곳이 생기게끔 작성했네요 🥲
첨부주신 깨진창문이론은 저도 읽으면서 완전 공감한 내용입니다.
앞으로 미션진행하면서 더 신경써보겠습니다~!!

wkdchdaud123 · 2023-08-28T15:36:38Z

src/test/java/kitchenpos/application/MenuGroupServiceTest.java

+
+@ExtendWith(MockitoExtension.class)
+@DisplayName("MenuGroup")
+class MenuGroupServiceTest {


마지막 단계인 만큼 좀 더 논의를 해보고 싶은데요. 😄
지금까지 작성한 Test Code는 무엇을 위한 Test 인가요?
인수테스트와 통합테스트의 필요성에 대해서는 어떻게 생각하시나요?

인수테스트 : https://www.youtube.com/watch?v=APC-G8EfqLM

제가 작성한 테스트들은 유닛테스트이며 각 클래스의 도메인로직밖에 검증하지 못하는 테스트입니다.

인수테스트와 통합테스트의 필요성에 대해서는 어떻게 생각하시나요?
인수테스트의 필요성은 격하게 느끼고 있으며 통합테스트의 경우도 QA분들이 최소한의 검증은 해주지만 필요성을 느끼고 있습니다.
다만 아직 ATDD 에 대해 잘 모르기 때문에 이번 미션에선 적용하지 않았는데
실무에서 최종적으로는 최소한 인수테스트 까지는 완료 되어야 한다고 생각합니다.

NNIIE added 5 commits August 25, 2023 02:12

test(kitchenpos): 메뉴그룹 테스트 작성

ac7d912

test(kitchenpos): 상품 테스트 작성

da07921

test(kitchenpos): 메뉴 테스트 작성

782888e

test(kitchenpos): 주문테이블 테스트 작성

35fe15a

test(kitchenpos): 주문 테스트 작성

b89658a

wkdchdaud123 requested changes Aug 27, 2023

View reviewed changes

NNIIE added 5 commits August 28, 2023 02:16

test(kitchenpos): getter / setter 테스트 케이스 제거

58fdde8

test(kitchenpos): fixture 생성 (MenuGroup / Product / Menu / OrderTable…

45cf4b2

… / Order)

test(kitchenpos): hasSize 적용

8248a9a

test(kitchenpos): isZero, isFalse 적용

31067fd

test(kitchenpos): DisplayName 변경 (요구사항 작성시 사용한 용어들로)

af2a1d8

NNIIE requested a review from wkdchdaud123 August 27, 2023 21:36

wkdchdaud123 requested changes Aug 28, 2023

View reviewed changes

Conversation

NNIIE commented Aug 26, 2023

Uh oh!

wkdchdaud123 left a comment

Choose a reason for hiding this comment

Uh oh!

wkdchdaud123 Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkdchdaud123 Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkdchdaud123 Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkdchdaud123 Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkdchdaud123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wkdchdaud123 Aug 27, 2023 •

edited

Loading

wkdchdaud123 Aug 27, 2023 •

edited

Loading

wkdchdaud123 Aug 27, 2023 •

edited

Loading

wkdchdaud123 Aug 27, 2023 •

edited

Loading