[test] 깃 레포 단위테스트 작성 #45
Conversation
Minjae-An
left a comment
Minjae-An
left a comment
There was a problem hiding this comment.
리뷰드린 사항을 바탕으로 전체 코드 검토를 부탁 드립니다. 중복된 사항에 대해 여러 번 리뷰를 달지 않았습니다. 참고 부탁 드립니다.
| EXCEED_GITHUBREPO_EMAIL(HttpStatus.BAD_REQUEST, "이메일은 5개까지 등록할 수 있습니다"), | ||
| INVALID_GITHUBREPO_URL(HttpStatus.BAD_REQUEST, "잘못된 URL 입니다."), | ||
| INVALID_GITHUBREPO_EMAIL(HttpStatus.BAD_REQUEST, "잘못된 이메일 입니다"), |
There was a problem hiding this comment.
| EXCEED_GITHUBREPO_EMAIL(HttpStatus.BAD_REQUEST, "이메일은 5개까지 등록할 수 있습니다"), | |
| INVALID_GITHUBREPO_URL(HttpStatus.BAD_REQUEST, "잘못된 URL 입니다."), | |
| INVALID_GITHUBREPO_EMAIL(HttpStatus.BAD_REQUEST, "잘못된 이메일 입니다"), | |
| EXCEED_GITHUB_REPO_EMAIL_LIMIT(HttpStatus.BAD_REQUEST, "이메일은 5개까지 등록할 수 있습니다"), | |
| INVALID_GITHUB_REPO_URL(HttpStatus.BAD_REQUEST, "잘못된 URL 입니다."), | |
| INVALID_GITHUB_REPO_EMAIL(HttpStatus.BAD_REQUEST, "잘못된 이메일 입니다"), |
- 코드 컨벤션에 예외명이 어긋나는 듯 보이는데 수정 부탁드립니다
- 예외 상황을 좀 더 명확히 표현할 수 있도록 예외명을 변경하는 것이 어떨까요?
| public GithubRepo findByUserIdAndRepositoryId(Long userId, Long repositoryId) { | ||
|
|
||
| if (userId == null) { | ||
| throw new GeneralException(UserException.USER_NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
이 메서드를 이용할 때 userId는 무조건 설정되어 들어올 것으로 보이는데(인증/인가가 정상적으로 되었다면) 굳이 래퍼타입으로 다루는 이유가 있을까요? 래퍼 타입의 적절한 사용에 대해 고려해보시면 좋겠습니다.
There was a problem hiding this comment.
현재 구조상 userId는 null이 될 가능성이 없다고 생각하기 때문에, 해당 부분을 기본형으로 바꾸거나 null 체크를 생략하는 것도 고려할 수 있겠습니다. 해당 논의를 바탕으로 코드의 의도를 좀 더 명확히 반영할 수 있도록 개선해보겠습니다.
| @Transactional(readOnly = true) | ||
| public GithubRepoPageResponse getGithubRepoInfos(Long userId, int page) { | ||
|
|
||
| if (page < 0) { | ||
| throw new GeneralException(GithubRepoException.INVALID_GITHUBREPO_PAGE); | ||
| } |
There was a problem hiding this comment.
userId를 래퍼 타입으로 다루는 이유가 있을까요? 래퍼 타입의 적절한 사용에 대해 고려해보시면 좋겠습니다page의 범위를 확인하는 검증같은 기본 검증을service가 아닌controller에서 다루는 것은 어떨까요? 검증 코드으로 인해 비즈니스 로직이 수행하는 핵심 작업 이상으로 비대해지는 경향이 있어보입니다. 검토 부탁드립니다
There was a problem hiding this comment.
말씀하신 것처럼, 컨트롤러에서 입력값을 사전에 검증하고 핵심 비즈니스 로직은 Service에서만 담당하도록 분리하는 것이 코드의 역할을 명확히 하고 가독성을 높이는 데 더 적절할 것 같습니다.
이 부분은 Controller 단에서 @RequestParam에 대한 validation을 추가하는 방식으로 개선하는 방향을 검토해보겠습니다.
| @InjectMocks | ||
| private GithubRepoService githubRepoService; |
There was a problem hiding this comment.
위에서 아래로 코드를 읽는 특성을 고려해 @InjectMocks의 대상이 되는 필드를 가장 하단에 배치하는 것은 어떨까요?
There was a problem hiding this comment.
넵 가독성을 위해 필드 하단에 배치하는게 좋을 것 같습니다
| ReflectionTestUtils.setField(mockRepo, "id", 1L); | ||
| ReflectionTestUtils.setField(mockUser, "id", 1L); |
There was a problem hiding this comment.
리플렉션을 사용할 수도 있지만 Mockito.mock()을 활용해 mock 객체를 생성하면 의도하는 값을 제공하도록 설정할 수 있습니다. 리플렉션은 디버깅을 어렵게 만드는 경향이 있습니다. 검토하시고 필요 시 수정 부탁드립니다.
There was a problem hiding this comment.
오 잘 몰랐었는데 감사합니다. 검색해서 찾아보니 리플랙션이
- 은닉된 필드(private)에 접근해서 강제로 값을 설정하기 때문에 캡슐화를 위반합니다.
- 런타임 시점까지 값이 설정되는 방식이기 때문에 디버깅이나 유지보수가 어려워질 수 있습니다.
- IDE의 자동완성, 컴파일 타임 체크 등의 도움을 받기 어렵습니다.
이런 특성을 가진다고 하네요. Mocito.mock을 활용하여 고쳐보겠습니다!
|
|
||
| GithubRepo githubRepo = new GithubRepo(name, url, endDate); | ||
| GithubRepoCreateResponse response = new GithubRepoCreateResponse( | ||
| "owner", |
There was a problem hiding this comment.
"owner"와 같이 메서드내에서 여러번 사용되는 값의 경우 변수로 정의하여 사용하는 것이 어떨까요?
| mockContributorList = List.of( | ||
| new Contributor("[email protected]"), | ||
| new Contributor("[email protected]") | ||
| ); |
There was a problem hiding this comment.
mockContributorList 대신 mockContributors 변수명은 어떨까요? 다른 변수들과 네이밍 형식이 달라 코드의 일관성을 저하시키는 듯 보입니다.
There was a problem hiding this comment.
아 일관성 측면에서 보면 그게 맞는것 같습니다.
| } | ||
|
|
||
| @Test | ||
| @DisplayName("레포를 생성할 수 있다.") |
There was a problem hiding this comment.
레포라는 명칭은 테스트 코드 실행 결과를 확인할 때 작성자가 아닌 사람 입장에서 혼동을 유발할 수 있다고 생각합니다. 레포지토리나 차라리 영문 Repository로 작성하는 것은 어떨까요?
There was a problem hiding this comment.
팀 내에서 github_repo라고 도메인을 설정하고 있어서 레포라고 작성한 것입니다. 혼동을 줄것같으면 깃허브 레포지토리로 바꾸는 방향을 검토해보겠습니다!
| ); | ||
|
|
||
| // when | ||
| GithubRepoGetResponse result = githubRepoService.getGithubRepoInfo(1L, 1L); |
There was a problem hiding this comment.
앞선 리뷰와 마찬가지로 여러 번 사용되는 값의 경우 변수를 통해 의미를 명확히 하면 더 좋을 것 같습니다.
| GithubRepoGetResponse result = githubRepoService.getGithubRepoInfo(1L, 1L); | |
| GithubRepoGetResponse result = githubRepoService.getGithubRepoInfo(userId, repoId); |
| assertThatThrownBy(() -> githubRepoService.createGithubRepo(1L, request)) | ||
| .isInstanceOf(GeneralException.class) | ||
| .hasMessageContaining(GithubRepoException.INVALID_GITHUBREPO_URL.getMessage()); |
There was a problem hiding this comment.
| assertThatThrownBy(() -> githubRepoService.createGithubRepo(1L, request)) | |
| .isInstanceOf(GeneralException.class) | |
| .hasMessageContaining(GithubRepoException.INVALID_GITHUBREPO_URL.getMessage()); | |
| assertThatThrownBy(() -> githubRepoService.createGithubRepo(1L, request)) | |
| .isInstanceOf(GeneralException.class) | |
| .extracting("exception") | |
| .isEqualTo(GithubRepoException.INVALID_GITHUBREPO_URL); |
메시지로 확인할 수도 있지만 직접 예외 ENUM을 비교하여 발생하는 예외를 직접 표현하는 방법도 있습니다. 검토해보시고 필요 시 수정 부탁드립니다.
There was a problem hiding this comment.
현재는 메시지를 기준으로 예외를 확인하고 있는데, 이는 실제 사용자에게 노출되는 메시지의 정확성을 검증하고자 하는 의도가 있었습니다.
다만 말씀주신 것처럼 GeneralException 내부의 enum 값을 직접 비교하면 예외 정책을 보다 명확하게 검증할 수 있고, 메시지 변경에도 영향을 덜 받는 장점이 있다는 점에서 민재님의 방법도 고려해보겠습니다!
📌 작업 개요
✨ 기타 참고 사항
🔗 관련 이슈