[레거시 코드 리팩터링 - 1단계] 테오(최우성) 미션 제출합니다. #457
Conversation
|
|
||
| ## 요구 사항 🚀 |
| // then | ||
| assertThat(menuGroups).hasSize(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 클래스뿐만 아니라 모든 파일에 마지막 개행이 누락 되어있습니다!
해당 글을 통해서 설정 부탁드릴게요~
There was a problem hiding this comment.
인텔리제이 라이센스가 끝나면서 커뮤니티로 넘어왔는데 설정이 초기화되었네요 ㅠㅠ
EOF관련 문제는 해당 링크 토대로 빠르게 고치도록 하겠습니다 👍
| @TestConstructor(autowireMode = AutowireMode.ALL) | ||
| @SpringBootTest | ||
| @Sql("classpath:cleanup.sql") | ||
| public class ServiceTestContext { |
| @DisplayNameGeneration(ReplaceUnderscores.class) | ||
| @TestConstructor(autowireMode = AutowireMode.ALL) | ||
| @SpringBootTest | ||
| @Sql("classpath:cleanup.sql") |
There was a problem hiding this comment.
만약에 테이블이 추가가 된다면 sql문에 delete from ...을 계속 써줘야 할 것 같습니다~
테이블을 알아서 인식하고 sql문 없이도 자동으로 초기화 시켜주는 방법도 있는데 찾아보시면 좋을 것 같습니다!
There was a problem hiding this comment.
감사합니다!
제이가 말씀해주신 것과 같이 테이블의 추가 / 변경이 cleanup.sql에 영향을 줄 수 있을 것 같습니다.
따라서 information_schema.tables 테이블을 사용해 table_schema가 PUBLIC인 테이블들을 찾아 모두 비워주도록 변경했습니다.
참고로, H2 database는 커넥션 연결 시 스키마(데이터베이스) 명을 지정하지 않으면 PUBLIC으로 설정되는 것 같았습니다! (링크)
그리고 굳이 SQL 파일로 실행할 필요는 없을 것 같아서, TestExecutionListener를 활용하도록 변경했습니다. 🙇♂️
| setupOrderLineItem(); | ||
| } | ||
|
|
||
| private void setupOrderTable() { |
There was a problem hiding this comment.
이 부분에서 궁금한게 있습니다!
해당 메서드와 같이 setup을 해주신 이유는 무엇인가요? Fixture를 이용해서 필요한 부분에서만 만들어서 사용할 수 있지 않을까요?
테이블 별로 제약조건이 있기 때문에 현재 클래스에 명시된 Fixture를 사용하는 부분에서는 제약조건에 위배해서 저장하는 것처럼 느껴져서요 ㅎㅎ
이에 대해 테오는 어떻게 생각하시나요?
단순히 궁금해서 여쭤보는 코멘트이니 꼭 바꾸실 필요는 없습니다!
There was a problem hiding this comment.
저는 해당 클래스에서 선언하는 객체들이 더미 데이터로써 활용되고 있기 때문에, Fixture와는 결이 조금 다르다고 생각했었습니다!
상속받아 savedOrderTable 등을 사용하는 클래스는 '이게 더미 데이터이고, 저장은 되어 있겠구나' 라는 확신을 가지고 테스트를 작성할 수 있어서 저런 형태로 작성했습니다!
테이블 별로 제약조건이 있기 때문에 현재 클래스에 명시된 Fixture를 사용하는 부분에서는 제약조건에 위배해서 저장하는 것처럼 느껴져서요 ㅎㅎ
어떤 제약조건을 의미하신건지 조금 더 설명해주실 수 있을까요? 😅 현재 체크해보니 Constraints를 위반하는 경우는 없는 걸로 여겨집니다. 외래키 제약조건도 저장 순서를 지정해줬기 때문에 위반하지 않고 있구요! 아마 위반하는 경우에는 테스트가 돌아가기도 전에 Integrity 관련 예외가 터졌을 것 같아요.
비즈니스 제약조건을 말씀해주신 것이라면, 저도 동감하는 부분입니다! 현재 각 객체들의 속성은 임의로 구성했는데, 비즈니스 제약조건에 맞게 재구성하는게 나을까요?
답변 남겨주시면 빠르게 찾아오겠습니다 ㅎㅎ
There was a problem hiding this comment.
그렇군요~ 생각해보니 해당 클래스로 컨텍스트를 띄우니 문제가 생기면 테오 말대로 이미 예외가 발생했겠네요!
더미로 활용한다는 테오 말을 듣고보니 이렇게 사용한 이유가 충분히 납득이 됐습니다!
좋은 의견 감사합니다 😀
| import java.util.List; | ||
| import kitchenpos.domain.Menu; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
There was a problem hiding this comment.
이미 아시겠지만 이 부분에
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)를 달아주면 DisplayName에 밑줄이 사라진다고 합니다 ㅎㅎ
There was a problem hiding this comment.
감사합니다! 그런데 해당 어노테이션은 ServiceTestContext에 이미 정의해두어서 상속을 받는 테스트 클래스에서는 정의하지 않았습니다! 😄
| Menu createdMenu = menuService.create(menu); | ||
|
|
||
| // then | ||
| assertThat(createdMenu.getId()).isNotNull(); |
There was a problem hiding this comment.
다중 검증을 할 때 assertAll()이나 assertSoftly()를 사용하면 중간에 실패하는 검증이 있더라도 모든 검증을 하고 결과를 반환해줍니다!
현재는 69줄이 실패한다면 70줄의 검증은 시도하지 않을 것 같습니다~
There was a problem hiding this comment.
다중 검증을 수행하는 부분은 SoftAssertions를 사용하도록 모두 변경하겠습니다! 👍
| import org.springframework.test.context.TestContext; | ||
| import org.springframework.test.context.support.AbstractTestExecutionListener; | ||
|
|
||
| public class DatabaseCleaner extends AbstractTestExecutionListener { |
안녕하세요 제이!
마지막 미션을 함께 하게 되어 영광입니다 🙇♂️
이번 단계에서는 모든 Business Object에 대한 테스트코드를 작성했습니다.
비즈니스 로직이라고 판단되는 내용은 모두 검증한 것 같습니다.
README에 기능목록을 명시해두었고, 해당 기능 목록을 기반으로 테스트를 작성했기 때문에
메소드명과 기능목록을 대조하시면서 보시면 리뷰가 수월할 것 같습니다!
이번 단계에서는 Mockito를 사용하지 않았는데,
그러다보니 테스트 코드 구조가 이해하기 조금 어려운 것 같아 추가적으로 보충 설명을 드리면 다음과 같습니다.
ServiceTestContext를 상속합니다.ServiceTestContext는 스프링 컨텍스트의 생성을 담당할 뿐만 아니라, 데이터베이스에 더미 데이터를 삽입합니다.ServiceTestContext를 상속한 Service 테스트 클래스들은 필요에 따라 더미 데이터를 사용합니다. (검증 대상에 따라 더미 데이터를 사용하지 않을 수도 있습니다)이번 리뷰 잘 부탁드립니다! 감사합니다 ~