step3: 테스트를 통한 코드 보호 #535

seungjoopet · 2023-08-29T14:15:56Z

기존 PR에서 리뷰 받은대로 가독성을 위해 README를 정리하였습니다.

리뷰어님 괜찮으시다면, Step3 미션을 두번에 나눠서 PR을 올려도 될까요?
(시간이 촉박하기도 하고, 리뷰 받은 내용을 토대로 다음 테스트코드에도 적용하고 싶은 마음입니다. 모든 코드가 완성되었을 때 리뷰받는 것이 원칙이라면 말씀주세요 :))

먼저 menu, menuProduct, product application service에 대한 테스트코드를 작성했습니다.
다음 PR에서 order, orderTable 테스트를 작성할 예정입니다.

- menuGroupService 테스트 생성 - FakeRepository 추가

- productService 테스트코드 추가 - 필요한 fakeRepository 추가 - menuService 테스트코드 추가

hyunssooo

안녕하세요.
이번 step 도 잘 진행해 주셨어요.
아직 다 작성하지 못한 테스트 코드는 step3 브랜치에 계속 push 해주시면 됩니다. :)
몇 가지 코멘트 남겼는데요. 나머지 코드들 작성하시면서 확인 부탁드려요!

hyunssooo · 2023-08-30T11:22:05Z

src/main/java/kitchenpos/domain/MenuGroupRepository.java

+public interface MenuGroupRepository {
+


repository 추상화 👍

hyunssooo · 2023-08-30T11:23:07Z

src/test/java/kitchenpos/application/AbstractApplicationServiceTest.java

+
+public abstract class AbstractApplicationServiceTest {
+


테스트를 위한 객체 생성 메서드들이 모여있는데요. 클래스 이름은 조금 어색한 것 같아요.

이름이 너무 고민이네요 ^_ㅠ
Utils라고 하긴 어렵고
그렇다고 환경설정이나 공통되는 테스트가 들어있는 것도 아니고..

hyunssooo · 2023-08-30T11:23:26Z

src/test/java/kitchenpos/application/MenuFakeRepository.java

+
+public class MenuFakeRepository implements MenuRepository {
+


fake 객체 💯

hyunssooo · 2023-08-30T11:25:21Z

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

+
+    @DisplayName("menuGroup을 생성 후 반환한다.")


요구 사항을 잘 작성해 주셨는데요. 테스트에 대한 설명도 요구 사항을 기반으로 작성해 보는 것은 어떨까요?

요구사항을 기반으로 작성한다는게, 어떤 뜻일지 조금 더 설명 가능하실까요~?
메뉴그룹을 예로 들면, 아래처럼 말씀이실까요?

메뉴그룹을 생성한다

메뉴그룹은 이름을 가지고 있다

hyunssooo · 2023-08-30T11:26:18Z

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

+    private MenuGroup create(final String name) {
+        final MenuGroup group = new MenuGroup();
+        group.setName(name);
+
+        return group;
+    }


AbstractApplicationServiceTest에 있는 메서드와 이 메서드는 어떤 기준으로 위치가 다를까요? 🤔

먼저 테스트 코드 내부에 private으로 맥락에 맞는 객체를 생성하는 게 올바르다 라고 생각하는데요!
현 코드는 모든 서비스 코드에서 도메인 이라고 불리는 객체들을 다 사용중이라서,

일관성을 위해 인자로서 사용되는 모든 프로퍼티를 가진 객체(create***Reqeust)를 생성하는 것만 AbstractApplicationServiceTest 에 넣고,
그걸 활용해서 각 테스트코드에서 private 메소드를 통해 테스트맥락에 맞는 객체를 생성하는 걸 목표로 했습니다~!

hyunssooo · 2023-08-30T11:28:41Z

src/test/java/kitchenpos/application/MenuServiceDisplayTest.java

+
+@ExtendWith(MockitoExtension.class)
+class MenuServiceDisplayTest extends AbstractApplicationServiceTest {
+


MenuServiceDisplayTest 와 MenuServiceTest 는 어떤 기준으로 나눠주셨을까요?

테스트 클래스 길이가 늘어나 나눠주신 것 같기도 하네요 :)
@Nested 를 사용해 보는 것도 방법일 것 같아요. 계층 구조를 활용해 테스트 의도도 좀 더 명확하게 전달 가능하다고 생각해요.

오 junit5가 처음이라 몰랐네요 Nested 라는 게 있었군요!
Nested한번 활용해볼게요 :)

말씀하신대로 테스트 클래스 길이가 늘어나서 클래스의 메소드별로 테스트코드를 나눴습니다.

hyunssooo · 2023-08-30T11:34:24Z

src/test/java/kitchenpos/application/ProductFakeRepository.java

+
+public class ProductFakeRepository implements ProductRepository {
+


fake 객체들을 패키지를 따로 분리해도 좋을 것 같아요. :)

hyunssooo · 2023-08-30T11:35:29Z

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

+    @DisplayName("product의 가격이 없거나 음수이면 예외를 발생시킨다.")
+    @Test
+    void create_invalid_price() {
+
+        // given
+        final Product nullPrice = createProductRequest(TEST_NAME, null);
+        final Product negativePrice = createProductRequest(TEST_NAME, BigDecimal.valueOf(-1L));
+
+        // when & then
+        assertThatThrownBy(() -> service.create(nullPrice))
+            .isInstanceOf(IllegalArgumentException.class);
+
+        assertThatThrownBy(() -> service.create(negativePrice))
+            .isInstanceOf(IllegalArgumentException.class);
+    }


@ParameterizedTest @NullSource @ValueSource(strings = "-1") void create_invalid_price(BigDecimal price) { .. }

이렇게도 가능하겠네요 :)

오 중첩으로 어노테이션 작성이 가능하군요 👍

- Nested를 활용해봄 - 메소드를 테스트 할 때 발생할 수 있는 예외상황을 nested로 표현했는데, 가독성&스펙 기재 면에서는 괜찮은 것 같으나 적절히 사용한지는 모르겠음

@nested

- fakeRepository를 사용하여 테스트 - 테스트 가독성을 위해 함수별로 테스트클래스 따로 생성 - 테스트 메소드는 @nested를 사용하여 테스트 응집도 및 가독성을 완화해봄 - 테스트를 도와주는 abtract class는 용처에 맞게 이름 변경

seungjoopet · 2023-09-06T01:06:37Z

@hyunssooo
안녕하세요! 리뷰 반영하였고 드디어 남은 테스트코드도 작성해보았습니다. (너무 오래걸렸네요 ^_ㅠ)

제안주신 nested와 junit5의 다른 기능들을 좀 찾아서 이용해보았어요!

이번에는 아래와 같은 목표를 가지고 테스트를 작성해보았습니다.
목표에 대한 방식이 적절했는지, 아니면 어떻게 접근했으면 좋을지도 리뷰해주시면 감사하겠습니다 :)

nested를 쓸 때 비문 만들지 않기 (테스트가 모두 돌았을 때 글만 읽고 맥락파악 할 수 있게 하기)
당장 레거시 코드를 위한 테스트를 작성하는 것이 아닌, 추후 새로운 코드로 리팩토링 할 때 도움되기 / 리팩토링 후에도 재사용할 수 있는 테스트코드가 되기
- OrderServiceCreateExceptionTest 생성 : 특히 Order.create()에는 각종 객체의 사전조건들이 나타나있어서 리팩토링시에도 이를 파악하기 위해 분리해보았습니다.
- 배달/배달외 로 나뉘는 조건에도 배달/매장/포장 주문으로 모두 나눠서 테스트코드를 작성했습니다.

이 질문에도 답변주시면 이후 refator를 통해 수정해보겠습니다 :)
(+ 다른 꿀팁과 테스트코드 작성 가이드, 그 외 리뷰들도 많이 부탁드려요! 감사합니다~!)

hyunssooo

안녕하세요.
몇 가지 코멘트 남겼어요! 확인 부탁드립니다!

hyunssooo · 2023-09-06T15:40:31Z

README.md

+        - [ ] 주문상품은 메뉴에 존재해야 한다.
+        - [ ] 주문상품은 메뉴에 노출된 상태여야 한다.
+        - [ ] 주문상품의 가격은 메뉴의 가격과 같아야 한다.
+        - [ ] 배달, 포장 주문이면 주문상품의 수량이 0보다 작으면 안된다.


테스트를 작성 했으면 체크리스트는 체크해도 좋을 것 같아요. 🙂

#535 (comment)

테스트에 대한 설명이 요구 사항을 그대로 반영하면 다른 구성원이 테스트만 보고 비즈니스를 파악할 수 있지 않을까요?

hyunssooo · 2023-09-06T15:44:22Z

src/test/java/kitchenpos/application/OrderTableServiceChangeNumberOfGuestsTest.java

+
+@Nested
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)


최상위에도 @Nested 가 필요했나요??
@DisplayNameGeneration 는 처음보네요! 👍 저도 배워가요!

오잉 최상위 @Nested는 필요없네요!

hyunssooo · 2023-09-06T15:47:49Z

src/test/java/kitchenpos/application/OrderTableServiceChangeNumberOfGuestsTest.java

+
+    @Nested
+    class 메소드_호출시_예외_발생_조건_검사 {
+


클래스 하나에 내부 클래스 하나니 @Nested 를 사용한 의미가 조금 없는 것 같아요.

저는 이렇게 테스트를 볼 수 있길 기대했거든요.

@DisplayName("EatInOrder 는") class EatInOrderTest { @DisplayName("등록할 수 있다") @Nested class 등록할_수_있다 { // 등록에 대한 테스트 } @DisplayName("접수할 수 있다") @Nested class 접수할_수_있다 { // 접수에 대한 테스트 } }

hyunssooo · 2023-09-06T15:54:44Z

src/test/java/kitchenpos/application/OrderServiceCreateExceptionTest.java

+@DisplayName("OrderService의 create메소드 호출시 예외 발생 조건 테스트")
+public class OrderServiceCreateExceptionTest {


예외에 대한 테스트를 모두 모아두셨네요.
이 뜻은 실패하는 경우를 모두 모았다고 생각이 들어요.
다른 사람이 테스트를 통해서 비즈니스를 파악하고 있을 때 성공에 대한 테스트와 실패에 대한 테스트가 파일이 나눠져 있으면 파일을 여러 번 바꿔서 읽어야 해서 좀 어려울 것 같은 느낌이 드는데요. 이 부분은 어떻게 생각하시나요?

#535 (comment)

파일로 나누느냐 계층으로 나누느냐는 장단이 있을 것 같은데요. 개인적으로는 현재 테스트에 대한 파일이 꽤 많은 것 같긴해요! exception에 대한 테스트 또한 나눠 주셔서 더욱 많은 것 같고요. 이 부분은 개인적인 생각이니 참고만 해주세요! 🙂

다른 사람이 테스트를 통해서 비즈니스를 파악하고 있을 때 성공에 대한 테스트와 실패에 대한 테스트가 파일이 나눠져 있으면 파일을 여러 번 바꿔서 읽어야 해서 좀 어려울 것 같은 느낌

이 관점으로 생각해보니 공감되네요!
레거시코드는 한 클래스에 책임이 너무 많아서 테스트코드를 어떻게 짜야할지 고민이 많았는데 뭔가 접근법이 잘못됐던 거 같아요.
좀 더 고민해보겠습니다 :)

하나 궁금한게 있습니다~! 리뷰어님께서는
책임이 많은 레거시코드의 테스트코드를 짤 때

하나의 파일에 모아서

레거시코드의 메소드 흐름순으로

테스트코드를 작성하시는 편일까요?

메소드 흐름순으로 작성하는 것은 공감되지만
하나의 파일에 모든 테스트를 넣는 관점은 수천라인을 가진 파일이 될 것 같아서요!
이미 클래스 자체가 SRP를 어겼지만, 테스트 코드라도 SRP 지키는게 좀 더 적절하지 않을까? 생각하는데요~!
이건 어떻게 생각하실까요~?

hyunssooo · 2023-09-06T15:55:09Z

src/test/java/kitchenpos/application/OrderServiceCreateExceptionTest.java

+    @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+    class 주문_생성_요청의_인자인 {


@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) 이건 최상위에 하나만 있어도 괜찮은 거 같아요!

앗 제거하는걸 놓쳤네요 처리했습니다 :)

hyunssooo · 2023-09-06T15:55:48Z

src/test/java/kitchenpos/application/OrderServiceCreateExceptionTest.java

+
+    private OrderLineItem create(final Menu menu) {
+        return create(menu, TEST_ORDER_LINE_PRICE);
+    }
+
+
+    private OrderLineItem create(final Menu menu, final int quantity) {
+        return create(menu, TEST_ORDER_LINE_PRICE, quantity);
+    }


생성을 위한 메서드들도 별도의 클래스에 모아둘 수 있겠네요!

hyunssooo · 2023-09-06T16:01:05Z

src/test/java/kitchenpos/application/MenuServiceTestRequestUtils.java

+
+public abstract class MenuServiceTestRequestUtils {
+


굳이 상속해서 사용할 필요가 있나요?
파일 이름도 utils이고 메서드들 그냥 public static 으로 선언해서 사용해도 괜찮을 것 같아서요!
혹시 다른 의도가 있었을까요?

hyunssooo · 2023-09-06T16:05:10Z

src/test/java/kitchenpos/application/OrderServiceAcceptTest.java

+
+    private OrderService sut;
+


축약어는 사용하지 않는 것이 좋을 것 같아요.
sut(system under test)라고 이름을 지어주신 이유가 따로 있을까요? orderService를 선택하지 않으신 이유가 궁금해서요.

사용한 이유는 수업시간에 배워봐서 한 번 사용해봤어요 ㅋㅋ!

hyunssooo · 2023-09-06T16:06:14Z

src/test/java/kitchenpos/application/OrderServiceServeTest.java

+    @Test
+    void 주문의_상태가_accepted가_아니면_예외를_발생시킨다() {
+        // given
+        final Order waitingOrder = orderRepository.save(create(OrderStatus.WAITING));
+        final Order servedOrder = orderRepository.save(create(OrderStatus.SERVED));
+
+        // when & then
+        assertThatThrownBy(() -> sut.serve(waitingOrder.getId()))
+            .isExactlyInstanceOf(IllegalStateException.class);
+
+        assertThatThrownBy(() -> sut.serve(servedOrder.getId()))
+            .isExactlyInstanceOf(IllegalStateException.class);
+    }


@EnumSource 를 적용해 보아도 좋을 것 같아요.

hyunssooo · 2023-09-06T16:07:38Z

src/test/java/kitchenpos/application/OrderServiceServeTest.java

+@ExtendWith(MockitoExtension.class)
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+@DisplayName("OrderTableService의 serve메소드 테스트")
+public class OrderServiceServeTest {
+


어떤 테스트는 exception이 분리되어 있고 어떤 테스트는 합쳐져 있는데요. 이 부분은 통일하는 것이 어떨까요?
다양한 시도를 해보신 것은 좋으나 일관성이 어려워 테스트 위치 찾기가 꽤나 힘들 것 같아요.

seungjoopet added 2 commits August 28, 2023 21:38

feat(menuGroupTest) menuGroupService 테스트 생성

6487a50

- menuGroupService 테스트 생성 - FakeRepository 추가

feat(productService) productService 테스트코드 추가

00f88dd

- productService 테스트코드 추가 - 필요한 fakeRepository 추가 - menuService 테스트코드 추가

seungjoopet force-pushed the step3 branch from 434b72e to 00f88dd Compare August 29, 2023 23:46

hyunssooo requested changes Aug 30, 2023

View reviewed changes

refactor(review) 리뷰반영

03fd8a0

seungjoopet force-pushed the step3 branch from 290bc5d to 03fd8a0 Compare September 4, 2023 22:43

seungjoopet added 2 commits September 5, 2023 13:36

feat(orderTable) orderTable 테스트코드 작성

9e7536f

- Nested를 활용해봄 - 메소드를 테스트 할 때 발생할 수 있는 예외상황을 nested로 표현했는데, 가독성&스펙 기재 면에서는 괜찮은 것 같으나 적절히 사용한지는 모르겠음

hyunssooo requested changes Sep 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

step3: 테스트를 통한 코드 보호 #535

step3: 테스트를 통한 코드 보호 #535

seungjoopet commented Aug 29, 2023 •

edited

Loading

hyunssooo left a comment

hyunssooo Aug 30, 2023

hyunssooo Aug 30, 2023

seungjoopet Sep 4, 2023

hyunssooo Aug 30, 2023

hyunssooo Aug 30, 2023

seungjoopet Sep 2, 2023 •

edited

Loading

hyunssooo Aug 30, 2023

seungjoopet Sep 2, 2023 •

edited

Loading

hyunssooo Aug 30, 2023

hyunssooo Aug 30, 2023

seungjoopet Sep 2, 2023 •

edited

Loading

hyunssooo Aug 30, 2023

hyunssooo Aug 30, 2023

seungjoopet Sep 2, 2023

seungjoopet commented Sep 6, 2023 •

edited

Loading

hyunssooo left a comment

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023

seungjoopet Sep 14, 2023 •

edited

Loading

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023

seungjoopet Sep 14, 2023

hyunssooo Sep 6, 2023

seungjoopet Sep 14, 2023

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023

seungjoopet Sep 14, 2023

hyunssooo Sep 6, 2023

hyunssooo Sep 6, 2023


		@ExtendWith(MockitoExtension.class)
		class MenuServiceDisplayTest extends AbstractApplicationServiceTest {


		public class ProductFakeRepository implements ProductRepository {


		@Nested
		@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)


		@Nested
		class 메소드_호출시_예외_발생_조건_검사 {

		@DisplayName("OrderService의 create메소드 호출시 예외 발생 조건 테스트")
		public class OrderServiceCreateExceptionTest {

		@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
		class 주문_생성_요청의_인자인 {

step3: 테스트를 통한 코드 보호 #535

Are you sure you want to change the base?

step3: 테스트를 통한 코드 보호 #535

Conversation

seungjoopet commented Aug 29, 2023 • edited Loading

hyunssooo 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

seungjoopet Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungjoopet Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungjoopet Sep 2, 2023 • edited Loading

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

seungjoopet commented Sep 6, 2023 • edited Loading

hyunssooo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungjoopet Sep 14, 2023 • edited Loading

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

seungjoopet commented Aug 29, 2023 •

edited

Loading

seungjoopet Sep 2, 2023 •

edited

Loading

seungjoopet Sep 2, 2023 •

edited

Loading

seungjoopet Sep 2, 2023 •

edited

Loading

seungjoopet commented Sep 6, 2023 •

edited

Loading

seungjoopet Sep 14, 2023 •

edited

Loading