[OptimistLabyrinth] 🚀 1단계 - 테스트를 통한 코드 보호 by OptimistLabyrinth · Pull Request #641 · next-step/jwp-refactoring

OptimistLabyrinth · 2022-11-28T10:44:30Z

마크다운 문법 사용해서 표(테이블)로 정리까지 완료했습니다.

1단계 테스트 코드 작성을 진행하기에 앞서서
제가 파악한 요구사항 중에 잘못된 부분이 있는지 짚고 넘어가고 싶습니다.

확인 후에 피드백 부탁드려도 괜찮을까요??

마크다운 문법 사용해서 표(테이블)로 정리

geunhwanlee

요구사항 잘 파악해주셨어요! 👍🏻

소소한 코멘트 드렸으니 확인해주시면 감사하겠습니다. 🙏

테스트코드를 아직 작성하지 않으셔서 Request changes 상태로 돌려놓을게요.

그럼 수정후 재 리뷰요청 주세요!
화이팅하세요. 🔥

geunhwanlee · 2022-11-28T15:20:47Z

README.md

+### 상품 생성하기 (POST /api/products)
+
+- HTTP Response Code : 201 CREATED
+- 가격
+  - 가격은 반드시 있어야 한다
+  - 가격은 0 이상의 정수여야 한다.
+
+### 상품 리스트 조회 (GET /api/products)
+
+- HTTP Response Code : 200 OK


Suggested change

### 상품 생성하기 (POST /api/products)

- HTTP Response Code : 201 CREATED

- 가격

- 가격은 반드시 있어야 한다

- 가격은 0 이상의 정수여야 한다.

### 상품 리스트 조회 (GET /api/products)

- HTTP Response Code : 200 OK

- `상품`을 등록할수 있다

- `상품`의 가격은 0원 이상이어야 한다

- `상품`의 목록을 조회할수 있다

개발자가 아닌 사람들도 이해할수 있는 언어로 작성해보면 좋을것같네요!
위 예시는 참고만 부탁드려요 🙏

geunhwanlee · 2022-11-28T15:33:00Z

README.md

+  - 메뉴 그룹이 실제로 존재하지 않는다면 메뉴를 생성할 수 없다.
+- 상품 리스트
+  - 하나의 상품이라도 실제로 존재하지 않는다면 메뉴를 생성할 수 없다.
+- 메뉴의 가격은 항상 모든 상품 가격의 총합보다 작거나 같아야 한다.


Suggested change

- 메뉴의 가격은 항상 모든 상품 가격의 총합보다 작거나 같아야 한다.

- 메뉴의 가격은 모든 메뉴 상품의 금액의 합을 초과할수 없다

용어사전의 용어를 활용해서 작성하면 좋을것 같네요! 😀
그리고 개인적으로는 작거나 같다보다는 초과할수 없다와 같이 한 단어로 표현하는게 낫지 않을까 생각해요.

geunhwanlee · 2022-11-28T15:33:16Z

README.md

+### 메뉴 생성하기 (POST /api/menus)
+
+- HTTP Response Code : 201 CREATED
+- 가격
+  - 가격은 반드시 있어야 한다
+  - 가격은 0 이상의 정수여야 한다.
+- 메뉴 그룹
+  - 메뉴 그룹이 실제로 존재하지 않는다면 메뉴를 생성할 수 없다.
+- 상품 리스트
+  - 하나의 상품이라도 실제로 존재하지 않는다면 메뉴를 생성할 수 없다.
+- 메뉴의 가격은 항상 모든 상품 가격의 총합보다 작거나 같아야 한다.


메뉴에는 여러 메뉴 상품을 등록할수 있다와 같은 문구를 추가해보는건 어떨까요? 🤔

geunhwanlee · 2022-11-28T15:34:12Z

README.md

+
+- HTTP Response Code : 201 CREATED
+- 주문 항목 리스트
+  - 주문 항목이 반드시 1 개 이상이어야 한다.


Suggested change

- 주문 항목이 반드시 1 개 이상이어야 한다.

- 주문 항목은 필수이다

이렇게 표현하는건 어떤가요? 😀

geunhwanlee · 2022-11-28T15:39:02Z

README.md

+### 주문 생성하기 (POST /api/orders)
+
+- HTTP Response Code : 201 CREATED
+- 주문 항목 리스트
+  - 주문 항목이 반드시 1 개 이상이어야 한다.
+  - 모든 메뉴 실제로 확인
+    - 하나의 메뉴라도 실제로 존재하지 않는다면 주문을 생성할 수 없다.
+    - 주문 테이블이 실제로 존재하지 않는다면 주문을 생성할 수 없다.
+
+### 주문 리스트 조회하기 (GET /api/orders)
+
+- HTTP Response Code : 200 OK
+
+### 주문 상태 변경하기 (PUT /api/orders/{orderId}/order-status)
+
+- HTTP Response Code : 200 OK
+- 주문 아이디
+  - 존재하지 않는 주문에 대해서 상태 변경을 요청할 수 없다.
+- 이전의 주문 상태
+  - 이전의 주문 상태가 "계산 완료" 라면 상태 변경을 요청할 수 없다.


주문테이블이 빈 테이블이면 주문할수 없다 이 내용이 빠진것같네요!
또한 주문 생성시 주문 상태가 조리인것도 표현해주면 좋을것같아요. 😀

geunhwanlee · 2022-11-28T15:40:42Z

README.md

+## 테이블 생성 (POST /api/tables)
+
+- HTTP Response Code : 201 CREATED
+- 새로운 테이블을 생성할 때 기본적으로 테이블 그룹 아이디는 null 로 초기화한다.


테이블 그룹 아이디가 null이라는건 어떤 표현으로 바꿔볼수 있을까요? 🤔

geunhwanlee · 2022-11-28T15:42:26Z

README.md

+- HTTP Response Code : 200 OK
+- 주문 테이블 아이디
+  - 주문 테이블이 실제로 존재하지 않으면 빈 테이블로 표시할 수 없다.
+- 주문 테이블이 테이블 그룹에 포함되어 있다면 빈 테이블로 표시할 수 없다.


Suggested change

- 주문 테이블이 테이블 그룹에 포함되어 있다면 빈 테이블로 표시할 수 없다.

- 단체 지정된 주문 테이블은 빈 테이블로 변경할 수 없다.

이렇게 표현하는건 어떤가요?

OptimistLabyrinth · 2022-12-13T14:30:25Z

위에서 저의 요구사항 분석에 대해서 리뷰해주신 피드백도 반영했습니다 ㅎㅎ

그런데
제가 뭔가를 잘못하고 있는걸까요
이렇게 모든걸 모킹하는게 다소 무의미하게 타이핑 연습만 하고 있는 것처럼 느껴지는 이유가 뭔지 모르겠습니다 😢

geunhwanlee

1단계 테스트코드 잘 작성해주셨어요! 👍🏻

다만 몇가지 아쉬운 점이 있어 코멘트 남겼습니다. 😭

모킹을 사용한 격리테스트는
Service가 의존하고있는 테스트하기 어려운 객체들을 Mock객체로 모킹하고
Service에 존재하는 비즈니스 로직을 테스트하기 위해 작성하는거라고 생각해요. 😀

그러한 목적을 가지고 테스트를 작성하신다면 무의미하다고 느끼지 않을거같아요 👍🏻

다만 테스트를 작성하실때 굉장히 복잡하다고 느끼셨을것 같은데요
그것과 관계된(?) 내용을 코멘트로 남겼으니 한번 고민해보시면 좋을것같아요!

진행하다 어려운 점이 있으시면 언제든 연락주세요.

그럼 한번 시도해보시고 재 리뷰요청 주세요 🙏
화이팅하세요! 🔥

geunhwanlee · 2022-12-14T13:58:07Z

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

+        private MenuGroup setupSuccess(String name) {
+            final MenuGroup mockMenuGroup = new MenuGroup();
+            mockMenuGroup.setName("test menu group");


Suggested change

private MenuGroup setupSuccess(String name) {

final MenuGroup mockMenuGroup = new MenuGroup();

mockMenuGroup.setName("test menu group");

private MenuGroup setupSuccess(String name) {

final MenuGroup mockMenuGroup = new MenuGroup();

mockMenuGroup.setName(name);

빠트리신것 같아요 😀

geunhwanlee · 2022-12-14T14:04:17Z

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

+        private MenuGroup setupSuccess(String name) {
+            final MenuGroup mockMenuGroup = new MenuGroup();
+            mockMenuGroup.setName("test menu group");
+            Mockito.when(menuGroupDao.save(Mockito.any())).thenReturn(mockMenuGroup);


Suggested change

Mockito.when(menuGroupDao.save(Mockito.any())).thenReturn(mockMenuGroup);

BDDMockito.given(menuGroupDao.save(Mockito.any())).willReturn(mockMenuGroup);

BDDMockito를 사용하면 given when then 스타일에 맞게 작성할 수 있어요 😀

geunhwanlee · 2022-12-14T14:10:36Z

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

+                            final Product product = new Product();
+                            product.setId(productId);
+                            product.setPrice(BigDecimal.valueOf(100 * productId));


Suggested change

final Product product = new Product();

product.setId(productId);

product.setPrice(BigDecimal.valueOf(100 * productId));

final Product product = new Product(productId, BigDecimal.valueOf(100 * productId));

도메인 클래스의 생성자를 수정하면 더 간략하게 작성할수 있겠네요 😀

geunhwanlee · 2022-12-14T14:14:07Z

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

+                final List<MenuProduct> menuProducts = new ArrayList<>();
+                for (int i = 0; i < products.size(); ++i) {
+                    final Product product = products.get(i);
+                    final MenuProduct menuProduct = new MenuProduct();
+                    menuProduct.setSeq((long) i + 1);
+                    menuProduct.setMenuId(menuId);
+                    menuProduct.setProductId(product.getId());
+                    menuProduct.setQuantity(product.getId() - 10);
+                    menuProducts.add(menuProduct);
+                }


테스트 픽스처를 만드는데 복잡한 코드가 들어가서 가독성이 낮아지는것같아요. 😭
어떻게 하면 간단하게 픽스처를 생성할수 있을까 고민해보면 좋을것같아요 🤔

만약 가격에 대한 테스트를 한다면

MenuGroup 메뉴그룹1 = menuGroupFixture(5000); Menu = menuFixture(10000, 메뉴그룹1);

이런식으로 관심있는 데이터인 가격만 드러나도록 작성하는것도 한 방법일것 같아요 😀

geunhwanlee · 2022-12-14T14:19:08Z

src/test/java/kitchenpos/ui/MenuGroupRestControllerTest.java

+@WebMvcTest(MenuGroupRestController.class)
+public class MenuGroupRestControllerTest {


Controller에 대한 테스트도 작성해주셨네요 👍🏻

geunhwanlee · 2022-12-14T14:33:43Z

src/test/java/kitchenpos/ui/MenuRestControllerTest.java

+        private Menu setupBadRequestPriceMoreThanSumOfProductPrice() {
+            final Menu menu = new Menu();
+            menu.setName("test menu");
+            menu.setPrice(BigDecimal.valueOf(1500));
+            Mockito.when(menuRestController.create(Mockito.any())).thenReturn(ResponseEntity.badRequest().build());
+            return menu;
+        }


지정한 메뉴 가격이 포함하고 있는 상품의 가격 총합보다 커서 메류를 등록하지 못하면 400 상태 코드를 응답받는다

위 조건에 맞게 테스트가 된걸까요..? 🤔
조건과 상관없이 bad request를 응답하는걸로 보이네요. 😭

geunhwanlee · 2022-12-14T14:38:14Z

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

+        @Nested
+        @DisplayName("메뉴 가격이 포함된 상품의 총합보다 크면 메뉴 등록 실패")
+        public class ErrorMenuPriceExceedsSumOfProductPrice {
+            @Test
+            public void testCase() {
+                // given
+                final Menu menu = setup();
+
+                // when - then
+                assertThrows(IllegalArgumentException.class, () -> menuService.create(menu));
+            }


복잡한 픽스처생성을 setUp() 메서드로 분리해낸건 좋습니다. 👍🏻

다만 너무 많은 흐름을 setUp으로 분리해냈다고 생각이 들어요. 🤔
메뉴 가격이 포함된 상품의 총합보다 크면 메뉴 등록 실패 조건을 테스트하는것이기 때문에
테스트코드의 흐름을 보고도 위 조건을 테스트하는지 쉽게 알수 있어야해요.

그렇게 하기 위해선 테스트의 흐름에서 각각의 상품의 가격과 메뉴가격이 드러나야하고
메뉴가격이 상품가격의 총합보다 크다는게 드러나야합니다.

어떻게 작성해야할지 한번 고민해보시면 좋을것같네요 😀

feat: 요구사항 문서화

f14886a

마크다운 문법 사용해서 표(테이블)로 정리

geunhwanlee requested changes Nov 28, 2022

View reviewed changes

OptimistLabyrinth added 7 commits December 1, 2022 20:21

docs: 문서 내용을 개발자가 아닌 구성원도 이해하기 쉬운 언어로 표현하기

d2529c2

feat: 어떤 머신에서 실행하든 h2 데이터베이스 사용하도록 application.prop 수정

447ea40

test: ProductRestController, ProductService 에 대한 테스트 코드 작성

cfb03ff

test: MenuGroupRestController, MenuGroupService 에 대한 테스트 추가

8f10335

test: 테스트 코드에 대한 설명(display name) 잘못된 부분 고치기

1f49518

test: MenuRestController, MenuService 에 대한 테스트 코드 작성

805ad80

test: TableGroupRestController, TableGroupService 에 대한 테스트 코드 작성

23d9850

OptimistLabyrinth force-pushed the feature/first_phase branch from 90f8335 to 23d9850 Compare December 12, 2022 13:18

OptimistLabyrinth added 2 commits December 13, 2022 00:30

test: OrderRestController, OrderService 에 대한 테스트 코드 작성

a1bf634

test: TableRestController, TableService 에 대한 테스트 코드 추가

8647a96

geunhwanlee requested changes Dec 14, 2022

View reviewed changes

	- 메뉴의 가격은 항상 모든 상품 가격의 총합보다 작거나 같아야 한다.
	- 메뉴의 가격은 모든 메뉴 상품의 금액의 합을 초과할수 없다

	- 주문 항목이 반드시 1 개 이상이어야 한다.
	- 주문 항목은 필수이다

	- 주문 테이블이 테이블 그룹에 포함되어 있다면 빈 테이블로 표시할 수 없다.
	- 단체 지정된 주문 테이블은 빈 테이블로 변경할 수 없다.

	Mockito.when(menuGroupDao.save(Mockito.any())).thenReturn(mockMenuGroup);
	BDDMockito.given(menuGroupDao.save(Mockito.any())).willReturn(mockMenuGroup);

		@WebMvcTest(MenuGroupRestController.class)
		public class MenuGroupRestControllerTest {

Conversation

OptimistLabyrinth commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geunhwanlee 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!

OptimistLabyrinth commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geunhwanlee 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OptimistLabyrinth commented Nov 28, 2022 •

edited

Loading

OptimistLabyrinth commented Dec 13, 2022 •

edited

Loading