-
Notifications
You must be signed in to change notification settings - Fork 8
test: 대학 관련 통합테스트 코드 추가 #148
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
test: 대학 관련 통합테스트 코드 추가 #148
Conversation
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 메서드도 너무 좋네요!!
그리고 UniversityServiceTest가 355줄이 되는걸 보니 UniversityService 가 책임이 많았나봅니다😓
슬랙으로 물어보신게 이걸 말씀하신거였군요.
이렇게 보니 서비스 자체를 분리하는게 적절하다 생각하는데,..
이 PR에서 추가 커밋으로 UniversityService를 조회 / 좋아요 관련된 기능으로 나눠보는건 어떨까요?
별도의 PR이 있으면 또 리뷰, 머지 작업이 필요할 것 같아서, 이건 이 PR에서 해도 좋을 것 같아요.
...test/java/com/example/solidconnection/university/service/UniversityRecommendServiceTest.java
Outdated
Show resolved
Hide resolved
| CustomException exception = assertThrows(CustomException.class, | ||
| () -> universityRecommendService.getPersonalRecommends(invalidEmail)); | ||
|
|
||
| assertThat(exception.getMessage()).isEqualTo(USER_NOT_FOUND.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.
| CustomException exception = assertThrows(CustomException.class, | |
| () -> universityRecommendService.getPersonalRecommends(invalidEmail)); | |
| assertThat(exception.getMessage()).isEqualTo(USER_NOT_FOUND.getMessage()); | |
| assertThatCode(() -> universityRecommendService.getPersonalRecommends(invalidEmail)) | |
| .isInstanceOf(CustomException.class) | |
| .hasMessage(USER_NOT_FOUND.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.
아하 새로 알았네요 감사합니다!
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.
이거 적용해봤는데 CustomException이 아닌 RuntimeException 이 뜬다고 테스트코드가 실패하더라구요 혹시 이유 알고계신가요?
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.
아 getUniversityDetail의 경우 현재 캐싱 AOP가 적용되어 있어서 예외가 바로 CustomException으로 오지 않고 RuntimeException으로 wrapping되어서 오는 거 같습니다
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.
- RuntimeException이 발생하는지 확인
- root cause를 가져와서
- 그게 CustomException인지 확인하고
- 그 예외의 메시지가 기대하는 메시지와 일치하는지 확인
이과정으로 하는 건 괜찮을까요?
@Test
void 존재하지_않는_대학_상세정보_조회시_예외_응답을_반환한다() {
// given
Long invalidUniversityInfoForApplyId = 9999L;
// when & then
assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(() -> universityQueryService.getUniversityDetail(invalidUniversityInfoForApplyId))
.havingRootCause()
.isInstanceOf(CustomException.class)
.withMessage(UNIVERSITY_INFO_FOR_APPLY_NOT_FOUND.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.
네 적절해보입니다😊
src/test/java/com/example/solidconnection/university/service/UniversityServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/service/UniversityServiceTest.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/service/UniversityServiceTest.java
Outdated
Show resolved
Hide resolved
| // when | ||
| UniversityDetailResponse dbResponse = universityService.getUniversityDetail(universityId); | ||
| Object cachedValue = cacheManager.get(cacheKey); | ||
| UniversityDetailResponse cacheResponse = universityService.getUniversityDetail(universityId); | ||
|
|
||
| // then(정말 한 번만 호출하는지 검증하는 로직이 필요한데 아직 구현이 안되었습니다.) | ||
| assertThat(dbResponse).isEqualTo(cacheResponse); | ||
| assertThat(cachedValue).isEqualTo(dbResponse); | ||
| } |
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.
추가로 캐시 테스트를 좀 정확하게 해보고싶었는데 Mock이 아니라 verfiy를 사용할 수 없더라구요 혹시 verify 없이 특정 함수가 몇 번 호출되었는지 확인하거나 아니면 캐시 검증 방법이 있을까요?
캐시가 잘 되는지 테스트 너무 좋습니다~ 🤩
SpyBean을 사용해서 호출 횟수를 검증하는 방법이 있습니다.
그 방식을 적용해보면 될 것 같습니다.
다만 위 코드에서 cacheManager를 사용하고 있는데, 이는 캐시의 구현에 관련된 내용이라 생각해요.
예를 들어 캐시 매니저가 아니라 RedisTemplate을 사용하도록 바뀐다면 위 코드는 깨질것입니다.
우리가 서비스 캐시 테스트에서 검증하고자 하는 것은,
"캐시가 적용되어, 두번 서비스코드를 호출하더라도 한번만 DB 조회를 한다"는 것이므로
cacheManager에 값이 저장되어있는지 확인하는 것은 불필요하다 생각됩니다.
레포지토리 함수 횟수검증만 하도록 바꿔보는건 어떨까요?
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.
- SpyBean는 처음 들어보았는데 이거에 대해서 좀 알아보겠습니다!
- 넵 저도 그게 맞는 거 같습니다! 횟수검증만 되면 굳이 cacheManager에 값이 있는지 확인할 필요는 없을 거 같아요!
src/test/java/com/example/solidconnection/university/service/UniversityServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/service/UniversityServiceTest.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void 존재하지_않는_사용자_조회시_예외를_반환한다() { | ||
| // given | ||
| String invalidEmail = "[email protected]"; | ||
|
|
||
| // when & then | ||
| CustomException exception = assertThrows(CustomException.class, | ||
| () -> universityRecommendService.getPersonalRecommends(invalidEmail)); |
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.
이 부분은 규혁님과 제가 의견이 갈릴수도 있겠네요😓
저는 "존재하지 않는 사용자에 대한 예외 검증"은 이 테스트 코드에서 생략해도 된다 생각해요.
- 이 테스트 코드는 univseritySerivice에 대한 테스트입니다. 존재하지 않는 "대학"에 대한 예외 검증은 필요하겠지만, "사용자"까지 예외 검증하는건 과한 것 같아요.
- 모든 회원 기능에 대해서, "존재하지 않는 사용자 xx시 예외 반환" 테스트 코드를 작성하면, 중복되는 테스트코드가 너무 많아질 것 같습니다. 이미 이 PR 만 하더라도 3개가 있고요.
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.
저도 작성하면서 이게 너무 반복되는 거 같다고 느꼈습니다! 지우도록 하겠습니다!! 감사합니다
|
추가로, feat은 새로운 기능 추가와 관련된 커밋 프리픽스입니다. |
|
앗 엄청난 실수를 했었네요 주의하겠습니다..! 추가로 UniversityService를 조회 / 좋아요 관련된 기능으로 나누는 것도 좋은 것 같습니다! |
- CacheManager를 직접 검증하는 방식에서 SpyBean을 사용한 레포지토리 호출 횟수 검증으로 변경
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.
잘 변경해주셨습니다😄👏
컨벤션 관련 몇가지 리뷰 남겼습니다.
수정 커밋 / push 하신 다음 머지하셔도 될 것 같아요.
추가로, 브랜치 이름도 test/146-university-integration-test 로 변경해주시면 좋을 것 같습니다!
src/main/java/com/example/solidconnection/university/service/UniversityLikeService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/university/service/UniversityQueryService.java
Outdated
Show resolved
Hide resolved
|
|
||
| // then | ||
| assertThat(firstResponse).isEqualTo(secondResponse); | ||
| verify(universityInfoForApplyRepository, times(1)).getUniversityInfoForApplyById(universityId); |
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.
[컨벤션]
BDD Mockito 형식으로 바꿔볼까요?
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.
같은 클레스 142번 라인도 확인해주세요!
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.
한 번만 더 여쭤보겠습니다...!
then(universityInfoForApplyRepository).should(times(1)).getUniversityInfoForApplyById(universityId);
verify를 then sould로 바꿨는데 혹시 times도 쓰면 안되는건지 알 수 있을까요?
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.
레포지토리 함수를 1번 호출했다는게 중요하니,..
BDD 문법은 아니지만 횟수를 명시하려면 쓰는게 좋겠네요!
관련 이슈
작업 내용
대학교 관련 통합 테스트 코드 추가하였습니다.
특이 사항
리뷰 요구사항 (선택)
이번엔 최대한 간결하게 작성해보려하긴 했는데 피드백 부탁드립니다..!
추가로 캐시 테스트를 좀 정확하게 해보고싶었는데 Mock이 아니라 verfiy를 사용할 수 없더라구요 혹시 verify 없이 특정 함수가 몇 번 호출되었는지 확인하거나 아니면 캐시 검증 방법이 있을까요?
예를 들어 제가 하려고 했던 방식은 상세조회 서비스 로직을 두 번 호출했을 때 처음에는universityInfoForApplyRepository.getUniversityInfoForApplyById(universityInfoForApplyId); 를 통해 한 번 조회해오고 다음 조회는 디비가 아닌 레디스에서 조회를 해오니 getUniversityInfoForApplyById가 한 번만 호출되는지 확인을 하면 된다고 생각했는데 검증 방법을 모르겠더라구요 의견주시면 감사하겠습니다.