Skip to content
111 changes: 111 additions & 0 deletions src/test/java/com/specialwarriors/conal/UserServiceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package com.specialwarriors.conal;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

import com.specialwarriors.conal.common.auth.oauth.GithubOAuth2WebClient;
import com.specialwarriors.conal.common.exception.GeneralException;
import com.specialwarriors.conal.user.domain.User;
import com.specialwarriors.conal.user.repository.UserRepository;
import com.specialwarriors.conal.user.service.UserService;
import java.util.Optional;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.data.redis.core.RedisTemplate;
import org.springframework.data.redis.core.ValueOperations;

@ExtendWith(MockitoExtension.class)
public class UserServiceTest {

@Mock
private UserRepository userRepository;

@Mock
private RedisTemplate<String, String> redisTemplate;

@Mock
private GithubOAuth2WebClient githubOAuth2WebClient;

@InjectMocks
private UserService userService;

private User mockUser;

@BeforeEach
void setUp() {
mockUser = new User(1, "홍길동", "fdsf");
}

@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에 대한 테스트를 수행하는데 메서드명에 엔티티를 굳이 포함할 필요는 없을 것 같습니다. 검토하고 필요할 경우 수정 부탁드립니다.


// 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);
}

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


@Test
@DisplayName("유저 아이디로 사용자 조회 시 존재하지 않으면 예외를 던진다")
void throwsExceptionWhenUserNotFoundById() {

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

// 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까지 확인을 하는게 좋다고 생각했었는데 알려주셔서 감사합니다!


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 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 등 변수를 활용해 값의 의미를 더 명확히 나타내는 것이 어떨까요?


@Test
@DisplayName("사용자를 삭제하려 할 때 사용자가 존재하지 않으면 예외를 던진다")
void throwsExceptionWhenDeletingNonexistentUser() {

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

// 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.

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

}
}