-
Notifications
You must be signed in to change notification settings - Fork 171
[4기 - 소재훈] Week2-1 바우처 관리 애플리케이션 테스트 코드 작성(재 PR 작업) #775
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
base: jay-so
Are you sure you want to change the base?
Conversation
…ntroller, Service, Domain 이동
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
재훈님 수고하셨습니다~
리뷰 확인해주세요~!
} | ||
} | ||
|
||
public void isValidDiscount(VoucherType type, long discount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스에서만 사용하는 메소드이니 private이 좋을것 같네요~
그보다 검증의 책임을 각 엔티티 클래스에서 수행하는게 어떨까요?
생성자 부분에서 검증하면 될것 같아요~
} catch (IllegalArgumentException e) { | ||
System.out.println(e.getMessage()); | ||
throw e; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root 레벨에서 해당 예외를 catch하고 있는데 중간에 한번 더 잡아서 내용을 출력해주는 이유가 있을까요?
public void isValidDiscount(VoucherType type, long discount) { | ||
switch (type) { | ||
case FIXED: | ||
if (discount <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버 잊지 말아주세요~ 적절한 변수명을 가진 상수로 빼주기!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수로 빼줘도 좋고, 전체 라인을 inlineMethod로 추출해도 좋고
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 내용과 관련해서.. 영주님 PR에서 멘토님께서 남겨주신 리뷰를 꼭 읽어보시면 좋을것 같습니다~
#769 (comment)
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
|
||
class FixedDiscountVoucherTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엣지케이스도 테스트 추가해주세요!!
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
|
||
class PercentDiscountVoucherTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기도 엣지케이스도 테스트해주세요~~
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({"10", "30", "50", "70"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ValueSource
로 테스트해도 될것 같네요~
private VoucherController voucherController; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
MockitoAnnotations.openMocks(this); | ||
voucherController = new VoucherController(voucherService); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
voucherController 위에 @InjectMocks
어노테이션을 사용하면 따로 객체를 생성해주지 않아도 됩니다~
@Test | ||
@DisplayName("고정 할인 바우처 생성 테스트") | ||
void createFixedDiscountVoucherTest() { | ||
// given | ||
long discount = 10000; | ||
|
||
// when | ||
voucherService.createVoucher(VoucherType.FIXED, discount); | ||
|
||
// then | ||
verify(voucherRepository, times(1)).insert(voucherCaptor.capture()); | ||
Voucher capturedVoucher = voucherCaptor.getValue(); | ||
|
||
assertThat(capturedVoucher).isInstanceOf(FixedDiscountVoucher.class); | ||
assertThat(capturedVoucher.getDiscount()).isEqualTo(discount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repository의 insert 메소드가 mocking 되지 않았네요~
given을 활용해 stub을 뱉도록 mocking해주세요.
그리고 ArgumentCaptor를 통해 캡처한 객체는 service에서 repository 메소드를 호출할 때 넘긴 파라미터 값입니다.
즉 중간 결과물만 테스트하고 있는것입니다. repository mocking 후 최종 결과물도 검증해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~
log.error("명령어가 잘못 입력되었습니다. ", e.getMessage()); | ||
} catch (Exception e) { | ||
log.error("프로그램에서 오류가 발생하였습니다.", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stackTrace가 있다면 나중에 로그를 볼때 좀더 빠르게 파악할것 같아요.
} | ||
} | ||
} | ||
|
||
private void createVoucher() { | ||
public void createVoucher() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 처음보네요
console.printMessage("바우처가 생성되었습니다!"); | ||
try { | ||
voucherController.createVoucher(voucherType, voucherDiscount); | ||
console.printMessage("바우처가 생성되었습니다!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
과연 생성 되었을까요?
public void isValidDiscount(VoucherType type, long discount) { | ||
switch (type) { | ||
case FIXED: | ||
if (discount <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수로 빼줘도 좋고, 전체 라인을 inlineMethod로 추출해도 좋고
} | ||
|
||
@Test | ||
@DisplayName("바우처 생성 validation 테스트") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른곳 다 동일합니다. 뭘 테스트 하려는지 기대값,목적이 여기선 보이지 않네요.
📌 과제 설명
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
📁 test/domain 패키지
FixedDiscountVoucherTest
PercentDiscountVoucherTest
📁 test/view 패키지
ConsoleApplicationTest
📁 test/service 패키지
VoucherServiceTest
✅ PR 포인트 & 궁금한 점
해당 VoucherService에서 isValidDiscount 메소드로 금액 지정 범위를 구분해도 괜찮은지 궁금합니다.
2. 테스트 작성 시 도메인단 테스트, 서비스 단 테스트, 리포지 테스트단 구분만 생각을 해봤을 뿐, @DisplayName()에 통해 해당 테스트가 보다 어떤 이름을 가지고 명시해야 하는지 이해가 부족한 것 같습니다.
(예시 - BookMark 도메인 테스트, BookMark 서비스단 테스트, BookMark 컨트롤러 단 테스트 등으로 구분을 하여 이해를 하고 있었습니다.) 보다 나은 명시를 위해 작성한 테스트의 @DisplayName()을 보다 알기 쉽게 어떻게 바꿀 수도 있는지 궁금합니다.
항상 감사드리며 리뷰 잘 부탁드립니다!🥰🥰 감사합니다!!