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

[레거시 코드 리팩터링 - 1단계] 썬(윤선용) 미션 제출합니다. #260

Merged
merged 22 commits into from
Oct 26, 2022

Conversation

syoun602
Copy link

안녕하세요 토닉!
마지막 미션에서 만나게되서 반갑습니다!!!
기존 프로덕션 코드는 변경하지 않고 기능 요구사항과 테스트만 추가했습니다!

그럼 따끔한 리뷰 부탁드려요 😊

@syoun602 syoun602 self-assigned this Oct 24, 2022
Copy link

@tonic523 tonic523 left a comment

Choose a reason for hiding this comment

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

안녕하세요 썬!
코드리뷰로 뵙게 되어 반갑습니다 ㅎㅎ
BDD를 Nested로 깔끔하게 작성해주셔서 test 읽기 편했어요! 그런데 제가 실행하는 환경이 다른건지 깨지는 테스트가 몇개 있네요 ㅠㅠ 코멘트에 남겨놨으니 확인부탁드립니다.
image

추가로 TableGroupService, TableService에 대한 테스트도 작성해주시면 좋을 것 같아요!

Comment on lines +32 to +36
void 저장한다() {
final MenuProduct savedMenuProduct = menuProductDao.save(menuProduct);

assertThat(savedMenuProduct.getSeq()).isNotNull();
}

Choose a reason for hiding this comment

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

제가 테스트 코드를 돌렸을 때 여기서 참조 무결성 제약조건이 위반됐다고 에러가 뜨네요
menuProduct를 생성할 때 1L로 id 값을 넣어주는데 1L에 해당하는 menuId와 productId는 어디에서 넣어주고 있나요?

org.springframework.dao.DataIntegrityViolationException: PreparedStatementCallback; Referential integrity constraint violation: "FK_MENU_PRODUCT_MENU: PUBLIC.MENU_PRODUCT FOREIGN KEY(MENU_ID) REFERENCES PUBLIC.MENU(ID) (1)"; SQL statement:
INSERT INTO menu_product (MENU_ID, PRODUCT_ID, QUANTITY) VALUES(?, ?, ?) [23506-200]; nested exception is org.h2.jdbc.JdbcSQLIntegrityConstraintViolationException: Referential integrity constraint violation: "FK_MENU_PRODUCT_MENU: PUBLIC.MENU_PRODUCT FOREIGN KEY(MENU_ID) REFERENCES PUBLIC.MENU(ID) (1)"; SQL statement:
INSERT INTO menu_product (MENU_ID, PRODUCT_ID, QUANTITY) VALUES(?, ?, ?) [23506-200]

Choose a reason for hiding this comment

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

MenuProductDaoTest에 있는 테스트들 전부 위 에러가 발생하네요 ㅠㅠ

Comment on lines +14 to +15
@JdbcTest(includeFilters = @ComponentScan.Filter(type = FilterType.ANNOTATION, classes = {Repository.class}))
public @interface DaoTest {

Choose a reason for hiding this comment

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

Repository 어노테이션이 붙은 빈만 띄울 수 있는 어노테이션을 구현하셨군요! 👍

Comment on lines +114 to +117
void 모든_주문을_반환한다() {
final List<Order> orders = orderService.list();
assertThat(orders).isEmpty();
}

Choose a reason for hiding this comment

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

모든 테스트를 돌렸을 때 이 테스트가 깨지네요
image
디버그를 돌려보니 Order하나가 조회가 됩니다!

Comment on lines 16 to 21
@Nested
class create_메서드는 {


@Nested
class 메뉴_그룹이_입력되면 {

Choose a reason for hiding this comment

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

bdd 방식으로 테스트를 구현해주셨네요! 👍

Copy link

@tonic523 tonic523 left a comment

Choose a reason for hiding this comment

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

썬 빠르게 반영해주셨네요! 😄
Nested로 작성해주시니 테스트 코드를 실행했을 때 읽기도 좋고 찾기도 편했어요 ㅎㅎ
컨벤션과 사용하지 않는 코드 등에 대해서 코멘트 남겼습니다!

Comment on lines 31 to 37
@Override
public String toString() {
return "MenuGroup{" +
"id=" + id +
", name='" + name + '\'' +
'}';
}

Choose a reason for hiding this comment

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

toString을 재정의하신 이유가 있나요?

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 +1 to +4
SET FOREIGN_KEY_CHECKS=0;

truncate table orders;
alter table orders alter column id restart with 1;

Choose a reason for hiding this comment

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

sql 어노테이션을 통해 테스트간의 멱등성을 지켜주셨네요!

  • truncate table orders restart identity;

h2에서는 위 처럼 하면 table을 truncate하면서 identity도 초기화시켜주는 것 같아요!

Copy link
Author

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.

앗 그런데 이렇게 변경하니까 테스트가 깨지네요,,?

Choose a reason for hiding this comment

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

h2database/h2database#2978
에서 제가 말한 방법이 레거시라고 하네요.. ㅎㅎ mysql 모드에서만 사용가능하다고 되있긴하네요!

@Nested
class 메뉴_그룹이_입력되면 {
class 메뉴_그룹이_입력되면 extends ServiceTest {

Choose a reason for hiding this comment

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

@Test 메서드를 호출하는 클래스에 ServiceTest를 상속받게 한 이유가 있나요?
제일 외부 클래스에서 선언해도 실행되는 것 같아서요!

Copy link
Author

Choose a reason for hiding this comment

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

넵 저도 그러고 싶었습니다 😢
다만 Springboot 버전 이슈로 그게 안되는거 같아요...
자세한 내용은 아래에 있습니다!
spring-projects/spring-framework#19930

Choose a reason for hiding this comment

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

오 nested는 스프링 테스트 컨텍스트를 상속받지 못하는 이슈가 있나보네요.. 참고해주신 자료 좀 더 확인해보겠습니다 감사합니다!

@SuppressWarnings("NonAsciiCharacters")
@DisplayName("MenuService 클래스의")
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class MenuServiceTest {

Choose a reason for hiding this comment

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

메뉴 가격이 상품들 가격의 합보다 작아야 한다 에 대한 테스트가 빠져있네요!

Copy link
Author

@syoun602 syoun602 Oct 26, 2022

Choose a reason for hiding this comment

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

와 이걸 어떻게 찾았죠? 👀
ㅋㅋㅋㅋㅋ 추가했습니다!!

}

@Nested
class 단체_지정이_null이면 extends ServiceTest {

Choose a reason for hiding this comment

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

단체 지정이 null이 아니면으로 수정해야할 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!!

}

@Nested
class 주문_테이블이_2개_이하라면 extends ServiceTest {

Choose a reason for hiding this comment

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

2개 이하 -> 2개 미만

Copy link
Author

Choose a reason for hiding this comment

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

헉 감사합니다

README.md Outdated
### 단체 지정 (Table Group)
- 생성 날짜, 주문 테이블들로 `단체 지정`을 등록할 수 있다.
- 주문 테이블들이 null이거나 2개 이하일 경우 예외가 발생한다.
- 빈 테이블이거나 단체 지정이 등록되지 않은 주문 테이블일 경우 예외가 발생한다.

Choose a reason for hiding this comment

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

빈 테이블 -> 비어있지 않은 테이블

Comment on lines 202 to 204
@Test
void ungroup() {
}

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.

헉 바로 지우겠습니다

Comment on lines +16 to +19
public class TestFixtures {

private TestFixtures() {
}

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.

넵 맞아요!! 훨씬 깔끔하더라구요 😊

Comment on lines +1 to +3
spring:
flyway:
enabled: false

Choose a reason for hiding this comment

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

flyway 실행하지 않도록 설정할 수 있네요! 제 미션에도 바로 반영하러가겠습니다 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link

@tonic523 tonic523 left a comment

Choose a reason for hiding this comment

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

피드백 반영하시느라 고생하셨어요!
썬 코드리뷰하면서 여러 기능들과 테스트 방법들을 배우고 가네요.
다음 단계도 화이팅입니다!! 😄

@tonic523 tonic523 merged commit 9640e68 into woowacourse:syoun602 Oct 26, 2022
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