Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Step3 테스트를 통한 코드 보호 #87

Open
wants to merge 10 commits into
base: ku-lee
Choose a base branch
from

Conversation

ku-lee
Copy link

@ku-lee ku-lee commented Feb 15, 2020

요구사항

  • 정리한 키친포스의 요구 사항을 토대로 테스트 코드를 작성한다.
  • 모든 Business Object에 대한 테스트 코드를 작성한다.
  • @SpringBootTest를 이용한 통합 테스트 코드 또는 @ExtendWith(MockitoExtension.class)를 이용한 단위 테스트 코드를 작성한다.
  • Controller에 대한 테스트 코드 작성은 권장하지만 필수는 아니다.
  • 미션을 진행함에 있어 아래 문서를 적극 활용한다.
    https://www.baeldung.com/spring-boot-testing

수정 사항

  • step1 리뷰 반영

    • CalcNumber 상수 - 변수 순서 변경
    • CalcNumber Default 생성자 수정
  • step2 요구 사항 수정

    • 잘못 정의 한 요구사항 수정
  • step3 테스트 코드 작성


안녕하세요 재성님 ! :)
리뷰 해 주시느라 고생 많으십니다! 👍
수업시간에 말씀 해 주셨던 '의식적으로 mock 사용하지 않기' 방법으로 테스트 코드를 작성 해 보았습니다.
피드백 부탁 드립니다! ㅎㅎ

Copy link
Contributor

@wotjd243 wotjd243 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빌더 패턴, 가짜 객체 등을 사용하여 테스트 코드를 잘 작성해 주셨습니다. 👍
고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

README.md Outdated
@@ -34,10 +34,10 @@

- 테이블 그룹
- [ ] 새로운 테이블 그룹을 생성 할 수 있다.
- [ ] 테이블 그룹 생성 시 최소 2개 이상의 주문 테이블이 있어야 한다.
- [ ] 테이블 그룹 생성 시 최소 1개 이상의 주문 테이블이 있어야 한다.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 요구 사항에 대해 고민해 봐요.

Suggested change
- [ ] 테이블 그룹 생성 시 최소 1개 이상의 주문 테이블이 있어야 한다.
- [ ] 테이블 그룹 생성 시 최소 2개 이상의 주문 테이블이 있어야 한다.

import java.math.BigDecimal;
import java.util.List;

public final class MenuBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빌더 패턴 👍

import java.time.LocalDateTime;
import java.util.Arrays;

public class Fixture {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 픽스처(test fixture) 👍

public class Fixture {
static MenuGroup 야식() {
return MenuGroupBuilder.aMenuGroup()
.withId(1L)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매직 넘버는 의미를 나타낼 수 있는 상수로 치환하여 코드의 가독성을 높여 보는 것은 어떨까요?

@Test
@DisplayName("새로운 메뉴를 등록 할 수 있다.")
void create() {
//given
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 코드를 조금 더 이해하기 쉽게 구분해 주는 좋은 습관 👍

assertThatIllegalArgumentException().isThrownBy(() -> menuBo.create(actual));
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 코멘트에 대해 고민해 봐요.
#46 (comment)

Comment on lines 77 to 96
@Test
@DisplayName("메뉴 가격은 메뉴 제품의 가격 총합을 초과 할 수 없다.")
void create3() {
//given
Menu actual = 치맥셋트();

BigDecimal sumOfProductsPrice = actual.getMenuProducts().stream()
.map(i -> {
Product p = productDao.findById(i.getProductId())
.orElseGet(this::getZeroPriceProduct);

BigDecimal price = p.getPrice();
BigDecimal quantity = BigDecimal.valueOf(i.getQuantity());

return price.multiply(quantity);
}).reduce(BigDecimal.ZERO, BigDecimal::add);

assertThat(actual.getPrice())
.isLessThan(sumOfProductsPrice);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Menu에 대한 테스트 코드입니다.

@DisplayName("전체 메뉴 리스트를 조회 할 수 있다.")
void listAll() {
//given
Menu createdMenu = 치맥셋트();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현시점에 굳이 필요 없다고 생각하는 코드가 있다면 아까운 마음을 버리고 가차 없이 삭제해 보세요.

Comment on lines 119 to 124
() -> assertThat(expected.stream().anyMatch(i -> {
Long expectedId = i.getId();
Long actualId = actual.get(0).getId();

return expectedId.equals(actualId);
}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectEnumerableAssert#containsExactlyInAnyOrderElementsOf(Iterable)을 찾아보고 그 기능을 사용해 보면 어떨까요?

import java.util.*;
import java.util.stream.Stream;

public class TestMenuDao implements MenuDao {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

가짜 객체 👍

- 테이블 그룹 잘못 된 정의 수정
- fixture : ID 상수 추가
- expect-actual 의미 혼용 수정
- list 비교 시 containsExactlyInanyOrderElementsOf 사용
- 필요 없는 테스트 코드 제거
- 전역 변수 -> 지역 변수 사용
- size 1인 ArrayList Collections.singletonList 로 변경
- 불필요한 공백 제거
@ku-lee
Copy link
Author

ku-lee commented Feb 25, 2020

안녕하세요 ! 리뷰 내용 수정 사항 반영 했습니다.
(늦어서 죄송합니다 😭)

아래는 수정 내용입니다.

README.md

  • 테이블 그룹 생성 시 최소 "2"개 이상 주문테이블 로 수정

테스트코드 수정

  • fixture : ID 상수 추가
  • expect-actual 의미 혼용 수정
  • list 비교 시 containsExactlyInanyOrderElementsOf 사용
  • 필요 없는 테스트 코드 제거
  • 전역 변수 -> 지역 변수 사용
  • size 1인 ArrayList Collections.singletonList 로 변경
  • 불필요한 공백 제거

상세하게 리뷰 해 주셔서 잘못 사용 하고 있던 점 & 유용한 메소드 등 많은 배움을 얻어갑니다.
수정 사항 리뷰도 잘 부탁 드립니다!🙇‍♀️

감사합니다!

Copy link
Contributor

@wotjd243 wotjd243 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백을 잘 반영해 주셨습니다. 👍
추가로 고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

import java.util.Arrays;

public class Fixture {
static final long MENU_GROUP_ID_야식 = 1L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixture 외에 사용하는 곳이 없어 보이네요. private으로 바꾸면 어떨까요?

Suggested change
static final long MENU_GROUP_ID_야식 = 1L;
private static final long MENU_GROUP_ID_야식 = 1L;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정했습니다!

.build();
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 코멘트에 대해 고민해 봐요.
#46 (comment)

return TableGroupBuilder
.aTableGroup()
.withId(TABLE_GROUP_ID_단체2)
.withOrderTables(Arrays.asList(비어있는_삼번테이블(), 비어있는_사번테이블()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'단체 지정된 빈 테이블'은 실제로 존재할 수 있을까요?

Copy link
Author

@ku-lee ku-lee Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 조금 더 고민이 필요 할 것 같습니다.!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단체로 지정되어 있는 빈 테이블 이라는 말이 말씀하신 대로 말이 돼지 않는 것 같습니다,
TableGroupBoTest 에서 create 시 공통으로 사용하려고 만든 목적이었는데,
이 부분은 수정하겠습니다!

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;

public class MenuBoTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"메뉴 가격은 메뉴 제품의 가격 총합을 초과할 수 없다."에 대한 테스트 코드를 작성해 보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가 완료!

//then
Assertions.assertAll(
() -> assertThat(actual).isNotNull(),
() -> assertThat(actual.getId()).isEqualTo(expected.getId()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 actualnull이라면 어떤 위험이 발생할까요? 그 위험을 피할 수 있는 방법도 찾아보면 좋을 것 같아요.

@DisplayName("주문 상태를 수정 할 수 있다.")
void changeOrderStatus() {
//given
Order registerdOrder = orderBo.create(order);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오타 발견!

Suggested change
Order registerdOrder = orderBo.create(order);
Order registeredOrder = orderBo.create(order);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엇 오타가 있었군요
수정 했습니다! :)


@Test
@DisplayName("새로운 테이블 그룹을 생성 할 수 있다.")
void create() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실패하는 테스트 코드입니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 했습니다 :) 감사합니다

}

@Test
@DisplayName("테이블 그룹 생성 시 1개 이상의 주문 테이블이 있어야 한다.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요구 사항과 이름이 다르네요.

Suggested change
@DisplayName("테이블 그룹 생성 시 1개 이상의 주문 테이블이 있어야 한다.")
@DisplayName("테이블 그룹 생성 시 최소 2개 이상의 주문 테이블이 있어야 한다.")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 했습니다 !
테스트 코드도 수정 했습니다, *

Comment on lines 68 to 77
//given
TableGroup expected = TableGroupBuilder
.aTableGroup()
.withId(1L)
.withOrderTables(Collections.singletonList(만석인_일번테이블()))
.withCreatedDate(LocalDateTime.now())
.build();

//when then
assertThatIllegalArgumentException().isThrownBy(() -> tableGroupBo.create(expected));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의도한 테스트와는 다른 테스트 코드입니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 의도한 곳에서가 아닌 다른곳에서 IllegalArgumentException이 발생했네요!
수정했습니다.


@Test
@DisplayName("테이블 그룹을 삭제 할 수 있다.")
void delete() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실패하는 테스트 코드입니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정 했습니다!

- assertThat null / validation 분리
- 오타 수정
- MenuBoTest 테스트 케이스 추가 (메뉴 가격이 제품 가격을 초과 하는 경우)
- 모순되는 Fixture 삭제 : 비어있는 테이블로 구성 된 단체 테이블2 삭제
- Fixture ID private 변환
- 실패 하는 테스트 코드 수정
@ku-lee
Copy link
Author

ku-lee commented Mar 5, 2020

안녕하세요 !
리뷰 주신 내용 토대로 테스트 코드를 수정 했습니다.

수정 된 내용은 아래와 같습니다.

test(kitchenpos) 테스트 코드 수정

  • assertThat null / validation 분리
  • 오타 수정
  • MenuBoTest 테스트 케이스 추가 (메뉴 가격이 제품 가격을 초과 하는 경우)
  • 모순되는 Fixture 삭제 : 비어있는 테이블로 구성 된 단체 테이블2 삭제
  • Fixture ID private 변환
  • 실패 하는 테스트 코드 수정

피드백 부탁드립니다!
감사합니다. 🙇‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants