Skip to content

Conversation

@yereumi
Copy link
Collaborator

@yereumi yereumi commented May 15, 2025

📌 작업 개요

  • 사용자 기능 단위테스트 작성

✨ 기타 참고 사항

🔗 관련 이슈

@yereumi yereumi self-assigned this May 15, 2025
@yereumi yereumi added the test label May 15, 2025
Comment on lines 47 to 60
@Test
@DisplayName("유저 아이디로 사용자를 조회할 수 있다")
void findUserByUserId() {

// given
given(userRepository.findById(1L)).willReturn(Optional.of(mockUser));

// when
User result = userService.getUserByUserId(1L);

// then
assertThat(result).isEqualTo(mockUser);
verify(userRepository).findById(1L);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@Test
@DisplayName("유저 아이디로 사용자를 조회할 수 있다")
void findUserByUserId() {
// given
given(userRepository.findById(1L)).willReturn(Optional.of(mockUser));
// when
User result = userService.getUserByUserId(1L);
// then
assertThat(result).isEqualTo(mockUser);
verify(userRepository).findById(1L);
}
@Test
@DisplayName("유저 아이디로 사용자를 조회할 수 있다")
void findUserByUserId() {
// given
long userId = 1L;
given(userRepository.findById(userId)).willReturn(Optional.of(mockUser));
// when
User result = userService.getUserByUserId(userId);
// then
assertThat(result).isEqualTo(mockUser);
verify(userRepository).findById(userId);
}

의미의 명확성을 위해 상수값보다 적절한 변수를 이용하는 것이 어떨까요?

Comment on lines 69 to 72
// when, then
assertThatThrownBy(() -> {
userService.getUserByUserId(1L);
}).isInstanceOf(GeneralException.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// when, then
assertThatThrownBy(() -> {
userService.getUserByUserId(1L);
}).isInstanceOf(GeneralException.class);
// when, then
assertThatThrownBy(() -> userService.getUserByUserId(userId))
.isInstanceOf(GeneralException.class)
.extracting("exception")
.isEqualTo(UserException.USER_NOT_FOUND);
  • 람다로 처리할 수 있는 경우, 람다를 적극 활용하는 것은 어떨까요?
  • 예외를 ENUM으로 관리하는 특성을 고려해 발생한 예외 ENUM까지 확인하면, 테스트 코드의 의도가 더 명확하지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

발생한 예외 ENUM까지 확인을 하는게 좋다고 생각했었는데 알려주셔서 감사합니다!

userService.getUserByUserId(1L);
}).isInstanceOf(GeneralException.class);

verify(userRepository).findById(1L);
Copy link
Collaborator

Choose a reason for hiding this comment

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

예외 케이스에서 굳이 해피 케이스에서 이미 검증한 함수 호출을 고려할 필요는 없을 것 같습니다. 검토 부탁드립니다.


@Test
@DisplayName("유저 아이디로 사용자를 조회할 수 있다")
void findUserByUserId() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

UserService내에서 findById에 대한 테스트를 수행하는데 메서드명에 엔티티를 굳이 포함할 필요는 없을 것 같습니다. 검토하고 필요할 경우 수정 부탁드립니다.

Comment on lines 77 to 95
@Test
@DisplayName("사용자를 삭제할 때 깃허브 토큰과 세션을 삭제한다")
void deletesGithubTokenAndSessionWhenUserIsDeleted() {

// given
given(userRepository.findById(1L)).willReturn(Optional.of(mockUser));

ValueOperations<String, String> ops = mock(ValueOperations.class);
given(redisTemplate.opsForValue()).willReturn(ops);
given(ops.get("github:token:1")).willReturn("fake-access-token");

// when
userService.deleteUser(1L);

// then
verify(githubOAuth2WebClient).unlink("fake-access-token");
verify(redisTemplate).unlink("github:token:1");
verify(userRepository).deleteById(1L);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

앞선 리뷰와 같은 맥락에서 상수값을 사용하는 것보다 userId, accessToken 등 변수를 활용해 값의 의미를 더 명확히 나타내는 것이 어떨까요?

Comment on lines 104 to 109
// when, then
assertThatThrownBy(() -> {
userService.deleteUser(1L);
}).isInstanceOf(GeneralException.class);

verify(userRepository).findById(1L);
Copy link
Collaborator

Choose a reason for hiding this comment

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

앞선 리뷰와 같은 맥락에서 람다의 활용, 상세 예외 확인을 고려하여 필요할 경우 코드 수정 부탁드립니다.

@yereumi yereumi requested a review from Minjae-An May 15, 2025 05:47
public String home(Model model, @SessionAttribute("userId") Long userId) {

User user = userService.getUserByUserId(userId);
User user = userService.getUser(userId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
User user = userService.getUser(userId);
User user = userService.findById(userId);

service의 메서드명을 repository에 맞추자는 예시는 위같은 형식을 의미했습니다. getUser는 어떤 파라미터를 바탕으로 조회하는 지 의미를 표현함에 있어 메서드명이 모호하다고 생각합니다.

.extracting("exception")
.isEqualTo(UserException.USER_NOT_FOUND);

verify(userRepository).findById(userId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

해피 케이스에서 이미 확인한 로직의 동작을 예외 케이스에서 부가적으로 확인할 필요가 있을 까요? 검토 부탁드립니다.


@Nested
@DisplayName("사용자를 삭제할 때")
class DeleteUser {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이너 테스트 클래스의 경우 DeleteUserTest와 같이 Test postfix를 붙여 테스트라는 것을 명확히 나타내는 것은 어떨까요?

@yereumi yereumi requested a review from Minjae-An May 15, 2025 06:30
@yereumi yereumi merged commit 1dc1e90 into dev May 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[test] 사용자 기능 단위 테스트 작성

4 participants