Step1 테스트를 통한 코드 보호 #493

mmtos · 2022-06-25T13:57:52Z

안녕하세요 리뷰어님 ㅎㅎ
잘 부탁드립니다~

ohtaeg

안녕하세요 승훈님!
이번 레거시 코드 리팩토링 미션을 같이 함께 할 리뷰어 오태경이라고 합니다 👋
함께하게 되어 영광이며, 잘 부탁드립니다 🙇

리뷰가 늦어서 죄송합니다 🙇
1단계 테스트 코드 및 요구사항을 잘 작성해주셨어요 👍 👍
전체적으로 요구사항은 잘 만족해주셔서 merge하려 했지만..!
같이 고민해보면 좋을 포인트가 존재하여 Request changes하였습니다 🙏
소소한 코멘트들을 드렸는데 확인해주시면 감사하겠습니다 😄

ohtaeg · 2022-06-26T07:08:08Z

README.md

+### 상품
+- 상품의 이름과 가격을 입력받아 새로운 상품을 생성할 수 있다.  
+  - 가격은 음수가 될 수 없다.
+- 키친포스에서 관리중인 상품의 목록을 조회할 수 있다.


상품 요구사항을 잘 분석해주셨어요 👍
키친포스라는 단어는 용어사전에 없는 단어인데, 없는 용어는 용어사전에 추가해주시거나
다른 용어를 활용해주시면 좋을 것 같은데 어떻게 생각하시나요 :) ?

ohtaeg · 2022-06-26T07:09:14Z

README.md

+### 메뉴그룹
+- 메뉴그룹의 이름을 입력받아 새로운 메뉴그룹을 생성할 수 있다.
+- 키친포스에서 관리중인 메뉴그룹의 목록을 조회할 수 있다.


사소하지만 개인적으로 용어 사전에 단어를 나타내는 경우, 저는 아래와 같이 표기하곤 합니다 😄
단어에 하이라이트를 줌으로써 용어사전에 정의되지 않은 단어와 비교하기 쉬운 장점이 있더라구요 :)
(수정을 위한 리뷰가 아니오니 참고만 부탁드려요!)

Suggested change

### 메뉴그룹

- 메뉴그룹의 이름을 입력받아 새로운 메뉴그룹을 생성할 수 있다.

- 키친포스에서 관리중인 메뉴그룹의 목록을 조회할 수 있다.

### 메뉴그룹

- `메뉴그룹`의 이름을 입력받아 새로운 `메뉴그룹`을 생성할 수 있다.

- 키친포스에서 관리중인 `메뉴그룹`의 목록을 조회할 수 있다.

확실히 좋은 방법이네요 ㅎㅎ 다른 문서 작성할때도 활용해봐야겠어요

ohtaeg · 2022-06-26T07:19:14Z

README.md

+
+### 메뉴
+- 메뉴의 이름, 가격, 메뉴 상품, 메뉴그룹ID를 받아 메뉴그룹 내 새로운 메뉴를 추가할 수 있다.
+  - 메뉴는 반드시 하나의 메뉴그룹에 포함되어야 한다.


메뉴를 구성하는 상품들은 존재해야하는지 상관없는지에 대한 요구사항도 있으면 좋을 것 같아요 :)

final Product product = productDao.findById(menuProduct.getProductId()) .orElseThrow(IllegalArgumentException::new);

ohtaeg · 2022-06-26T07:28:21Z

README.md

+### 주문 테이블
+- 새로운 주문 테이블을 생성할 수 있다.


위에 정리해주신 요구사항들과 일관된 가독성을 위해 주문 테이블은 어떤 필드를 가지고 있는지
설명해주시면 더욱 좋을 것 같아요 :)

### 상품 - 상품의 이름과 가격을 입력받아 새로운 상품을 생성할 수 있다. ### 메뉴 - 메뉴의 이름, 가격, 메뉴 상품, 메뉴그룹ID를 받아 메뉴그룹 내 새로운 메뉴를 추가할 수 있다.

다른 요구사항에도 적용해주시면 감사하겠습니다 🙏

넵 알겠습니다!

ohtaeg · 2022-06-26T07:31:52Z

README.md

+  - 빈 테이블의 경우 인원 수를 변경할 수 없다.
+
+### 테이블 그룹 지정
+- 통합계산을 위해 테이블을 묶어서 한번에 계산할 수 있다.


혹시 이 부분은 어느 코드를 참조하여 만들어주신건지 알려주실 수 있으실까요 :) ?
기존 코드나 테스트 코드에서 따로 확인을 해보았는데 찾아볼수가 없어서요..!
제가 놓쳤을수도 있는 부분이라 실례를 무릅쓰고 여쭤봅니다 🙏

아 이 부분은 코드상엔 없고 용어사전에 단체지정부분에 있어서 썼던것같습니다

ohtaeg · 2022-06-26T08:02:44Z

src/test/java/kitchenpos/application/OrderServiceTest.java

+    @Test
+    void 주문등록_주문테이블이_없는경우() {


개인적으로는 테스트 메서드 명으로만 어떤 테스트하기 위함인지 목적을 알기 어려울 수도 있다고 생각하는데요 :)
가령 테스트 메서드명만 보았을 때 주문 등록시 주문 테이블이 없는 경우에 대한 테스트 상황은 알겠지만,
없는 경우 통과인건지, 없는 경우 무엇을 검증하려는지 내부 테스트 코드를 보지 않는 이상 파악하기가 힘든 것 같아요 😄

번거로울수도 있는 작업이 될 수도 있겠지만 @DisplayName 을 활용하여 테스트를 요약해주시는 것에 대해
승훈님은 어떻게 생각하시나요 :) ?

DisplayName 사용해보겠습니다 ㅎㅎ 테스트명은 아무래도 공백입력이 불가능하다보니 조금 불편하긴 했었습니다.
혹시 displayname을 쓰게되면 메서드명은 어떤식으로 작성을 하는게 좋을지 궁금합니다.

부연 설명을 DisplayName을 통해 해주기 때문에 메서드명은 지금처럼 유지해주셔도 좋다고 생각해요!
만약 메서드명이 부족하다고 느껴지신다면 해당 테스트를 통해 무엇을 확인하고싶은지
조금만 더 추가해주시면 좋지 않을까 생각합니다 😄

개인적으로 테스트 작성시 성공, 실패 케이스를 나눠서 테스트하는 편이라
성공시, 주문등록_주문테이블이_존재하는경우_생성을성공
실패시, 주문등록_주문테이블이_없는경우_예외발생

물론 더 좋은 방법도 있을 수 있기에 승훈님께서 자유롭게 작성해주시는게 정답일 것 같아요 👍

ohtaeg · 2022-06-26T08:04:23Z

src/test/java/kitchenpos/application/OrderServiceTest.java

+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+
+class OrderServiceTest extends ServiceTest {


만약 해당 피드백에 대해 제가 찾지 못하고 실제 코드상에 존재하는 요구사항이라면
방문한 손님수가 0명이더라도 주문을 등록할 수 있다. 에 대한 검증 테스트는 빠진 것 같아요..!
해당 요구사항에 대한 테스트도 작성해주시면 감사하겠습니다 :)

ohtaeg · 2022-06-26T08:16:20Z

src/test/java/kitchenpos/application/TableGroupServiceTest.java

+        ungroupedTables.forEach(orderTable -> {
+            OrderTable foundTable = findOrderTableById(orderTable.getId(), tables);
+            assertThat(foundTable.getTableGroupId()).isNull();
+        });


개인적으로 if, for 키워드를 사용하는 구문은 로직이라고 생각하는데요!
테스트 코드 내에 if, for 구문이 있다면 해당 구문은 로직이며, 로직을 검증하기 위한 테스트 코드가 별도로 필요하다고 생각해요 :)

개인적으로 반복적인 검증이 필요하다면 for 문이 아닌 반복 테스트나 다이나믹 테스트를 이용하여 검증하는 방법을 지향하는 편입니다.
테스트 코드를 작성하면서 if, for문을 지양하는 것에 대해 승훈님은 어떻게 생각하시는지 궁금하네요 :)

https://docs.microsoft.com/ko-kr/dotnet/core/testing/unit-testing-best-practices

와 한번도 이런생각은 해본적이없었는데 충분히 설득력있는 내용인것 같아요.
저부분도 for문을 사용하지 않는 방식으로 코드를 변경해보겠습니다

ohtaeg · 2022-06-26T08:26:44Z

src/test/java/kitchenpos/application/TableGroupServiceTest.java

+    private OrderTable findOrderTableById(Long tableId, List<OrderTable> tables) {
+        return tables.stream()
+                .filter(orderTable -> tableId.equals(orderTable.getId()))
+                .findFirst()
+                .orElseThrow(IllegalArgumentException::new);
+    }


기존 코드에 맞게 새로운 코드를 작성하여 테스트하는 것도 좋지만 만약 프로덕션 코드에서 요구사항이 변경되어
"저장되어 있는 테이블 그룹내에 있는 주문 테이블과 그룹 해체하려는 주문 테이블들이 일치하지 않아도 괜찮다"
라는 요구사항이 억지로 생긴다고 가정했을 때 해당 메서드를 사용하는 테스트 코드들은 다 실패하는 경우가 생길 것 같아요..!

테스트의 목적은 기존 코드를 검증하는 것이 목적이기에 테스트 하는 경우 기존 로직을 최대한 이용하면 어떨까요 :) ?
findOrderTableById() 메서드를 제거하게 되면 어떻게 테스트를 해볼 수 있을지 고민해보시면 좋을 것 같아요 😄

이런부분이 몇개있긴한데 Service에는 저 역할을 하는 메소드가 없어서 저렇게 작성을 했었습니다. 최대한 Dao를 직접 사용하지 않는 테스트를 작성하려는 목적이었는데 , Dao를 직접 사용해도 혹시 문제가 안될까요?

최대한 Dao를 직접 사용하지 않는 테스트를 작성하려는 목적

말씀해주신대로 슬라이스 테스트라서 서비스 테스트 작성시 Dao를 직접 사용하지 않는 목적이 좋은 것 같아요 👍 👍
해당 미션의 제목처럼 테스트를 통한 코드 보호이기 때문에 서비스 레이어에서는 Repository 메서드가 제대로 호출되는지만 확인하면 되므로 실제 Repository 대신 Mock Repository를 사용하면 될 것 같아요!
Mockito나 Fake 객체를 사용하는 방향은 어떨까요? 😄

ohtaeg · 2022-06-26T08:28:23Z

src/test/java/kitchenpos/fixture/MenuGroupFixtureFactory.java

+
+import kitchenpos.domain.MenuGroup;
+
+public class MenuGroupFixtureFactory {


전체적으로 테스트 픽스쳐 생성 💯

- DisplayName 추가 - 누락된 테스트케이스 추가 - 최대한 프로덕션 코드의 로직을 사용해서 테스트를 진행 - for, if와 같은 제어문 제거

mmtos · 2022-06-27T03:47:47Z

리뷰 주신 내용 반영했습니다 ㅎㅎ 고민해봤는데 mock을 사용했을때 테이블 그룹이 해제되었는지 여부를 검증하는 의미가 퇴색되는것 같아서, service 메서드를 추가한 후 리팩토링 진행하였습니다!

ohtaeg

피드백 반영을 잘해주셨어요 👍 👍
소소한 코멘트를 드렸는데 확인해주시면 감사하겠습니다 :)
다음 미션도 화이팅입니다 🔥

ohtaeg · 2022-06-27T03:57:39Z

README.md

+### 메뉴 그룹
+- `메뉴 그룹`의 이름을 입력받아 새로운 `메뉴 그룹`을 생성할 수 있다.
+- `키친포스`에서 관리중인 `메뉴 그룹`의 목록을 조회할 수 있다.


피드백 반영 👍

ohtaeg · 2022-06-27T03:58:36Z

src/test/java/kitchenpos/application/OrderServiceTest.java

+        assertThat(savedOrder.getId()).isNotNull();
+        assertThat(savedOrder.getOrderStatus()).isEqualTo(OrderStatus.COOKING.toString());
+        assertThat(savedOrder.getOrderTableId()).isEqualTo(zeroGuestTable.getId());
+        assertThat(savedOrder.getOrderLineItems()).hasSize(2);


사소하지만 여러 검증문을 하나의 테스트에서 처리해야된다면 assertAll을 활용하는 것도 좋을 것 같아요 :)

사용해보겠습니다!

ohtaeg · 2022-06-27T04:11:41Z

src/main/java/kitchenpos/application/TableService.java

+    public List<OrderTable> findAllByTableGroupId(Long tableGroupId) {
+        return orderTableDao.findAllByTableGroupId(tableGroupId);
+    }


mock을 사용했을때 테이블 그룹이 해제되었는지 여부를 검증하는 의미가 퇴색되는것 같아서, service 메서드를 추가한 후 리팩토링 진행하였습니다!

말씀해주신 의도가 이해가 가네요 👍 저도 꼭 mokito를 사용할 필요는 없다고 생각해요 :)
테스트는 설계를 검증하기 위해 작성되기 때문에 설계에 집중해서 테스트를 해야하는데
Mock을 이용하면 어느 순간 내부 구현을 알아야하는 경우가 있더라구요!
즉, 블랙박스 테스트가 되지 않는 단점도 있는 것 같아요!

물론 정답이 없는 부분이라 참고만 해주시면 감사하겠습니다 :)
제가 감명깊게 본 Mock에 대한 용근님의 코멘트 링크도 같이 첨부드리오니 한번 보시면 좋을 것 같아요!! 😄

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step1 테스트를 통한 코드 보호 #493

Step1 테스트를 통한 코드 보호 #493

mmtos commented Jun 25, 2022

ohtaeg left a comment

ohtaeg Jun 26, 2022

ohtaeg Jun 26, 2022

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022

ohtaeg Jun 26, 2022

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022 •

edited

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022 •

edited

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022 •

edited

mmtos Jun 26, 2022

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022

mmtos Jun 26, 2022

mmtos commented Jun 27, 2022

ohtaeg left a comment

ohtaeg Jun 27, 2022

ohtaeg Jun 27, 2022

mmtos Jun 27, 2022

ohtaeg Jun 27, 2022

mmtos Jun 27, 2022

		### 주문 테이블
		- 새로운 주문 테이블을 생성할 수 있다.


		import kitchenpos.domain.MenuGroup;

		public class MenuGroupFixtureFactory {

Step1 테스트를 통한 코드 보호 #493

Step1 테스트를 통한 코드 보호 #493

Conversation

mmtos commented Jun 25, 2022

ohtaeg 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

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

ohtaeg Jun 26, 2022 • edited

Choose a reason for hiding this comment

ohtaeg Jun 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohtaeg Jun 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohtaeg Jun 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohtaeg Jun 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmtos commented Jun 27, 2022

ohtaeg 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

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022 •

edited

ohtaeg Jun 26, 2022 •

edited