PR for review #47
PR for review #47
Conversation
lette1394
commented
Sep 3, 2021
•
lette1394
commented
- 리뷰용 PR 입니다
- merge를 위한 PR이 아닙니다
Merge pull request YoutubePlaylist#9 from YoutubePlaylist/seungjae
[ADD] 플레이리스트의 썸네일 이미지 추가
Merge pull request YoutubePlaylist#14 from YoutubePlaylist/seungjae
[FIX] Refresh token redis structure
[FIX] Refresh token redis structure, expireTime
[FIX] Refresh token redis structure, expireTime
[ADD] 비밀번호 변경, PLAY-Channel title 추가
[ADD] 비밀번호 변경, PLAY-Channel title 추가
8월 3주차 merge
[FIX] 비회원 리스트 제한, 토큰 만료 시간
There was a problem hiding this comment.
coding convention이 무엇인가요?
보통 이걸 많이 사용하는 거 같아요~
https://google.github.io/styleguide/javaguide.html
리뷰를 질문 형식으로 많이 남겼어요.
제가 드린 질문에 대해서 많이 고민해보시고, 이리저리 코드도 변경해가면서 생각해보셨으면 좋겠습니다.
테스트 가능한 구조를 만드는 것을 궁극적인 목표로 두고 작성해보세요.
TDD 에 대해서 들어본 적이 있다면 이를 연습해 보는 것도 나쁘지 않구요
고생하셨어요 잘 만드셨네요ㅎㅎ
src/main/java/com/example/youtubedb/config/jwt/JwtAccessDeniedHandler.java
Show resolved
Hide resolved
| } | ||
|
|
||
|
|
||
| private void checkValidPassword(String password) { |
There was a problem hiding this comment.
이 메서드는 단위 테스트를 작성해야 한다는 생각이 드는데요,
private method 네요. 어떻게 해야할까요?
There was a problem hiding this comment.
java.lang.reflect.Method를 사용하여 MemberService의 checkValidPassword 메서드를 받고 setAccessible로 접근 가능하게 설정하여 단위 테스트를 진행하였습니다.
해당 Commit 내에 MemberServiceTest안에 한번 작성해보았습니다!
There was a problem hiding this comment.
테스트 코드도 코드입니다. 최대한 유지보수가 쉽게 만들어야 해요.
리플렉션를 이용해서 테스트 가능하도록 만들면 깨지기 쉬운 테스트코드가 됩니다.
따라서 private method를 테스트하는건 좋은 방법이 아닙니다.
private method를 테스트하고 싶다는 생각이 들면, 클래스/메서드 설계에 문제가 있다는 뜻이에요.
힌트를 조금 드리자면, MemberService가 너무 많은 일을 하고 있기 때문에 이런 현상이 발생하는 겁니다. (설계 문제) MemberService가 password의 유효성 검사도 하고 있기 때문에 private method로 나오게 되는거에요. password의 유효성 검사 책임만 갖는 별도 클래스를 설계해보세요~
There was a problem hiding this comment.
따로 password의 유효성에 관한 책임을 나누기 위해 PasswordValidationService를 따로 만들었습니다! 근데 그러다가 위에 checkValidPassword메서드 뿐만아니라 checkCorrectPassword도 바뀌는 비밀번호에 관한 유효성을 검증하는 로직이라고 생각하여 PasswordValidationService로 빼놓았었습니다. 그리고 테스트코드를 작성하던 중에
public void checkCorrectPassword(Member updateMember, String oldPassword, String newPassword) {
if(!passwordEncoder.matches(oldPassword, updateMember.getPassword())){
throw new DoNotMatchPasswordException();
}
if (oldPassword.equals(newPassword)) {
throw new DoNotChangePasswordException();
}
}checkCorrectPassword의 테스트를 아래 2가지 경우로 짜려고 하였습니다.
oldPassword와updateMember.getPassword()가 다를 경우oldPassword와newPassword가 동일할 경우
그런데 이 경우 이전 코멘트에서 말씀하신 것처럼 해당 테스트는 저희 프로젝트 코드의 테스트가 아닌 String의 equals메서드와 PasswordEncoder의 mathes메서드를 검증하는 것 같아서 해당 메서드에 대한 테스트는 제외하였습니다.
제가 아직 프로젝트에 대한 테스트만 고르는 것이 좀 미숙하여 혹시 제가 맞게 판단하였는지 봐주시면 감사할 것 같습니다!
There was a problem hiding this comment.
@oh980225 코드도 보여주세요ㅎㅎ
그런데 이 경우 이전 코멘트에서 말씀하신 것처럼 해당 테스트는 저희 프로젝트 코드의 테스트가 아닌 String의 equals메서드와 PasswordEncoder의 mathes메서드를 검증하는 것 같아서 해당 메서드에 대한 테스트는 제외하였습니다. 제가 아직 프로젝트에 대한 테스트만 고르는 것이 좀 미숙하여 혹시 제가 맞게 판단하였는지 봐주시면 감사할 것 같습니다!
구현을 하고 테스트를 작성하기보다, 테스트를 먼저 작성하고 구현을 해보세요.
checkCorrectPassword()가 수행해야 할 내용을 먼저 테스트로 작성해보는겁니다.
There was a problem hiding this comment.
넵! 아래 커밋링크입니다!
구현을 하고 테스트를 작성하기보다, 테스트를 먼저 작성하고 구현을 해보세요.
넵넵, 기능을 구현하기 전에 해당 기능에 관한 테스트를 먼저 작성해보는 것을 습관화하도록 해보겠습니다! ㅎ.ㅎ
| + "classpath:application.yaml," | ||
| + "classpath:aws.yaml") | ||
| @Transactional | ||
| class MemberServiceIntegrationTest { |
| import static org.junit.jupiter.api.Assertions.assertAll; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| @SpringBootTest(properties = "spring.config.location=" |
There was a problem hiding this comment.
spring application context를 띄우는데는 시간이 오래 걸립니다.
테스트 속도를 빠르게 할 수 있는 방법은 없을까요?
There was a problem hiding this comment.
공통 부분을 추출하여 추상 클래스로 만들어 상속받아 사용하면 test마다 spring application context를 띄우지 않고 한번만 불러 올 것 같습니다.
추가적으로 @SprongBootTest 보단 @WebMbcTest와 Mock object(모의 객체)를 사용하여 하나의 테스트랑 시간도 줄일 수 있는 unit-test 하는 것이 좋을 것 같습니다.
@lette1394
안녕하세요. 선배님 엄청난 리뷰에 감탄을 금치 못하고 있습니다...승재 덕분에 저도 이렇게 좋은 리뷰를 받아보네요.
다름이 아니라 한 부분만 더 평가해주셨으면 바라는 부분이 있어 이렇게 링크를 달게 되었습니다.
현재 승재와 함께 참여 중인 다음 공모전 프로젝트 테스트 코드입니다.
ApiDocumentationTest 라는 추상 클래스를 선언하고 각 컨트롤러 테스트에서 상속 받아 사용해 spring context를 반복해서 띄우지 않게 하였는데 옳은 방법으로 사용하고 있는지 한번 확인해 주실 수 있으실까요?
daycare - controllerTest
daycare - ApiDocumentationTest
There was a problem hiding this comment.
@JisooPark27
공통 부분을 추출하여 추상 클래스로 만들어 상속받아 사용하면 test마다 spring application context를 띄우지 않고 한번만 불러 올 것 같습니다.
추가적으로 @SprongBootTest 보단 @WebMbcTest와 Mock object(모의 객체)를 사용하여 하나의 테스트랑 시간도 줄일 수 있는 unit-test 하는 것이 좋을 것 같습니다.
엄... 그렇다면 제가 질문을 잘못 한 거 같아요ㅎㅎ 좀 더 풀어서 설명 드릴게요.
테스트 클래스이름을 integration test 라고 하셨지만, 테스트 작성하신 걸 보면 실제로는 unit test를 작성하려고 했다고 이해했어요. 그래서 application context를 띄우지 않고 작성하신 logic만 테스트하는 방법은 없는지 물어보려고 했던게 제 원래 의도였습니다. 전달이 잘 안됐네요ㅎㅎㅋㅋ
또한 제 환경에서 실행하니 이런 에러가 뜨네요.
테스트를 실행하기 위해서 이것저것 셋팅해야 되는건 좋은 테스트가 아닙니다
org.h2.jdbc.JdbcSQLNonTransientConnectionException: Connection is broken: "java.net.ConnectException: Connection refused (Connection refused): localhost:9092" [90067-200]
정말 integration test가 필요하면 test containers library에 대해서 알아보세요.
https://www.testcontainers.org/
There was a problem hiding this comment.
@JisooPark27
하나 더요, 이런 테스트 메서드가 있구요,
@Test
void loginId_조회_존재X() {
// given
String deviceId = "device001";
// when
Exception e = assertThrows(NotExistMemberException.class, () -> memberService.findMemberByLoginId(deviceId));
// then
assertThat(e.getMessage()).isEqualTo(NotExistMemberException.getErrorMessage());
}작성하신 코드는 요런데요,
public Member findMemberByLoginId(String loginId) {
return memberRepository.findByLoginId(loginId).orElseThrow(NotExistMemberException::new);
}public interface MemberRepository {
Member save(Member member);
Optional<Member> findByLoginId(String loginId);
void delete(Member member);
// boolean existsByLoginId(String loginId);
}다음 질문에 답변 부탁드려요.
- 어떤 걸 검증하기 위해서 작성하셨나요?
- 실제로는 무엇을 검증하고 있는걸까요?
- 이런 테스트는 필요할까요?
There was a problem hiding this comment.
사실 이번 프로젝트에 테스트 코드는 대부분 승재가 작성 한 것이라 승재의 생각과 좀 다를 수 있지만 제 생각대로 답변을 해보겠습니다. 그래서 dayCare 프로젝트랑 test가 많이 달랐던 것입니당...ㅜ
- 가입되어 있지 않은 deviceId가 들어 올 때 NotExistMemberException을 던지는지 확인하기 위해 작성 한 것 같습니다.
- NotExistMemberException의 message가 NotExistMemberException message인가 확인하고 있는 것 같습니다.
즉 같은 것을 검증하고 있는 것 같습니다.
// then
assertThat(e.getMessage()).isEqualTo(NotExistMemberException.getErrorMessage());는 제 생각에는 필요하지 않은 테스트라고 생각합니다.
// when
assertThrows(NotExistMemberException.class, () -> memberService.findMemberByLoginId(deviceId));이 것으로 memberService.findMemberByLoginId(deviceId))가 NotExistMemberException을 던지는지 테스트 하는 것은 괜찮다고 생각합니다.
추가적으로 Controller 테스트에서 memberService.findMemberByLoginId(deviceId))가 Exception을 던졌을 때 response의 status 나 body를 테스트 할 것 같습니다.
ps. 승재도 제 생각에 동의한다고 합니다.
There was a problem hiding this comment.
application context를 띄우지 않고 작성하신 logic만 테스트하는 방법은 없는지 물어보려고 했던게 제 원래 의도였습니다.
@ExtendWith(SpringExtension.class)가 기본적으로 포함된 @SpringBootTest를 사용하는 것이 아닌 @ExtendWith(MockitoExtension.class) 을 사용하면 application context를 띄우지 않고 logic만 테스트 가능할 것 같습니다..!
There was a problem hiding this comment.
"가입되어 있지 않은 deviceId가 들어 올 때"를 판단하는 로직은(findByLoginId 부분), 내가 작성한 코드인지 스프링이 작성해준 코드인지도 같이 고민해보시고, 만약 스프링이 작성해준 코드라면 이걸 테스트 해야 하는지까지 생각해보세요
저희 팀의 로직을 테스트 하는게 아니라 Optional::orElseThrow() 과 Repository 를 구현하고 있는 hibernate를 테스트 하고 있었던 것 같습니다.
저희 프로젝트에 로직, 코드를 테스트 하고 있지 않아 필요 없는 테스트를 하고 있었던 것 같습니다.
There was a problem hiding this comment.
아래 코드를 테스트 한다는 건
이 프로젝트의 코드를 테스트하는건가요, Optional::orElseThrow() 를 테스트하는건가요?
프로젝트의 코드를 테스트하기보다는 Optional::orElseThrow()를 테스트한다는 느낌이 강한 것 같습니다!
"가입되어 있지 않은 deviceId가 들어 올 때"를 판단하는 로직은(findByLoginId 부분), 내가 작성한 코드인지 스프링이 작성해준 코드인지도 같이 고민해보시고, 만약 스프링이 작성해준 코드라면 이걸 테스트 해야 하는지까지 생각해보세요
애초에 findByLoginId 부분은 hibernate에 의해 구현되어 있었기에 스프링이 작성해준 코드라고 볼 수 있을 것 같습니다. 즉 저희는 그냥 Optional::orElseThrow()를 테스트하는 코드를 작성했던 것 같습니다. 해당 테스트는 굳이 필요없는 테스트 였던 것 같습니다!
There was a problem hiding this comment.
@JisooPark27
저희 팀의 로직을 테스트 하는게 아니라 Optional::orElseThrow() 과 Repository 를 구현하고 있는 hibernate를 테스트 하고 있었던 것 같습니다.
저희 프로젝트에 로직, 코드를 테스트 하고 있지 않아 필요 없는 테스트를 하고 있었던 것 같습니다.
프로젝트의 코드를 테스트하기보다는 Optional::orElseThrow()를 테스트한다는 느낌이 강한 것 같습니다!
정확히 집어주셨습니다 👍👍👍
우리가 작성한 코드를 테스트 할 수 있도록 주의를 기울여주세요ㅎㅎ
외부 라이브러리를 검증하는 건 좋지만, 여기선 적절하게 사용한게 아닌 것 같아 의견드렸어요
(의도가 그게 아닌거같아서)
| assertAll( | ||
| () -> assertThat(play.getTitle()).isEqualTo(title), | ||
| () -> assertThat(play.getVideoId()).isEqualTo(videoId), | ||
| () -> assertThat(play.getStart()).isEqualTo(start), | ||
| () -> assertThat(play.getEnd()).isEqualTo(end), | ||
| () -> assertThat(play.getThumbnail()).isEqualTo(thumbnail), | ||
| () -> assertThat(play.getSequence()).isEqualTo(1), | ||
| () -> assertThat(play.getChannelAvatar()).isEqualTo(channelAvatar), | ||
| () -> assertThat(play.getPlaylist().getId()).isEqualTo(playlist.getId()), | ||
| () -> assertThat(play.getChannelTitle()).isEqualTo(channelTitle) | ||
| ); |
There was a problem hiding this comment.
해당 코드처럼 작성하면 실제 코드에 변경사항이 와서 Test code를 변경해야할 경우 어려움이 생길 수도 있고, 실제로 해당 코드가 무엇을 검증하는지 명확하지 않을 수 있다고 생각합니다. 즉 assert는 최소한으로 사용하는 것이 좋다고 생각합니다. 해당 부분도 수정하도록 하겠습니다!
There was a problem hiding this comment.
@oh980225
value object에 대해서 알아보시고 이걸 활용해서 테스트해보세요ㅎㅎ
무슨 말인지 잘 이해가 안된다면 질문해주시구요.
There was a problem hiding this comment.
Value Object를 공부한 뒤 우선 PlayServiceIntegrationTest에 void 플레이_추가() 테스트에 적용해보았습니다!
여러개의 assert가 아닌 한 개의 assert문으로 값의 동등성을 검증할 수 있게 만들어보았습니다. 저는 value object를 활용해서 테스트해보라는 comment를 이렇게 이해해서 적용하였는데 한 번 봐주시면 감사할 것 같습니다!
Commit
There was a problem hiding this comment.
@oh980225
@JisooPark27
이 내용은 여기서 설명하기 보다는 언제 한 번 날잡고 같이 보면서 설명드리는게 낫겠군요~
카톡으로 연락주세요ㅋㅋㅎㅎ
| playService.addPlayToPlaylist( | ||
| playlist, | ||
| member.getLoginId(), | ||
| videoId, | ||
| start, | ||
| end, | ||
| thumbnail, | ||
| title, | ||
| channelAvatar, | ||
| channelTitle); |
There was a problem hiding this comment.
많은 테스트메서드에서 비슷하지만 조금씩 다른 객체를 생성합니다.
이건 코드 중복일까요? 중복이라고 생각한다면 어떻게 개선해야 할지 의견을 남겨주세요
token.asJwtFormat() // 이 메서드를 어디에서 호출할까?
|
