Skip to content
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] UserServiceTest 작성 #126

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

packdev937
Copy link
Collaborator

Description

의존성을 최대한 줄이는 테스트 코드 작성

Main Features

  • 직접 생성자를 주입
  • UserRepositoryTest.java 생성 후 임의로 값을 저장

Others

  • JwtTokenManager 내 @value 가 Test시에는 주입 되지 않아 오류가 발생했습니다. 따라서 추가로 setValue() 메서드를 추가해 테스트 시 임의의 값을 추가하여 진행했습니다. ex) SecretKey, AccessTokenExpiration ...

@packdev937 packdev937 linked an issue Aug 23, 2023 that may be closed by this pull request
1 task
import java.util.List;
import java.util.Optional;

public class UserTestRepository implements UserRepository {
Copy link
Member

Choose a reason for hiding this comment

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

생성자를 추가해야할 것 같네요.
이 코드에서는 repository안에 데이터가 없을 겁니다. 비어있는 ArrayList인 것이죠. 생성자에서 필요한 데이터들을 ArrayList에 직접 넣어줘야지 아래의 Override한 메서드들이 제대로 테스트 될 것 같아요


@Test
void createAccessToken이_정상적으로_작동한다() {
Copy link
Member

Choose a reason for hiding this comment

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

createAccessToken을 테스트하는 것은 JwtTokenManager에 대한 Test 클래스에서 진행해야할 것 같아요. UserService에 대한 테스트라면 UserService의 메서드를 테스트하는 것이 좋습니다.


// @Value 값을 채워주기 위한 임시 값 세팅
jwtTokenManager.setValue(secretKey, 1L, 1L);
Copy link
Member

Choose a reason for hiding this comment

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

시크릿 키에 대한 건 BeforeEach나 BeforeAll 어노테이션을 사용해 그냥 초기화 해주는게 더 좋아보이네요

AuthorizationResponseDto authorizationResponseDto = userService.signup(request, secretKey);

// then
Copy link
Member

Choose a reason for hiding this comment

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

추가로 유저의 refreshToken 필드도 갱신이 되었는지 확인을 해보면 더 좋을 것 같습니다!

.platform(Platform.YOURWEATHER)
.build();
jwtTokenManager.setValue(secretKey, 1L, 1L);
Copy link
Member

Choose a reason for hiding this comment

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

이 부분도 마찬가지로 BeforeAll 등의 어노테이션을 사용해서 처음에 초기화해주는게 더 좋을 것 같아요

// then
assertEquals(changePasswordResponseDto.isSuccess(), false);
assertEquals(changePasswordResponseDto.getMessage(),
Copy link
Member

Choose a reason for hiding this comment

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

메시지 같은 부분은 수정되기 쉬운 부분이라서 메시지 자체를 비교한다면 리팩토링 내성이 좀 떨어질 것 같아요.
isSuccess 하나만 비교하도록 하는게 더 좋아보이네용 isSuccess 하나만으로도 요청이 실패했는지 성공했는지 알 수 있으니까요!

Comment on lines +151 to +155
public void setValue(String secretKey, Long accessTokenExpiration, Long refreshTokenExpiration) {
this.secretKey = secretKey;
this.accessTokenExpiration = accessTokenExpiration;
this.refreshTokenExpiration = refreshTokenExpiration;
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 secretKey, accessTokenExpiration, refreshTokenExpiration을 직접 필드로 넣지 말고 인터페이스와 객체를 추가해 @Value 어노테이션에 대한 JwtTokenManager의 의존성을 제거하면 하면 굳이 프로덕션 코드에 테스트 전용 메서드를 안 써도 될 것 같아요 자세한건 나중에 적어드릴게요..!

@shon5544
Copy link
Member

image

PR 확인 잘 했습니다
방향은 이런 식으로 가면 될 것 같네요.
해당 메서드를 사용하는 것에 대한 시나리오를 생각하고 테스트 코드를 쓰는 것 같은데, 그런 경우 @Nested 어노테이션을 사용하는 걸 추천할 게요! 다른 건 아니고 가시성이 훨씬 좋아집니다. 위의 사진처럼요 ㅎㅎ

그런 경우 어떤 방식으로 테스트를 쓸 지 알려드리자면
signup 메서드

  • 회원가입이 정상적으로 작동한다
  • 중복 이메일은 회원가입을 할 수 없다
  • ...
    이런 식으로 가시성을 높일 수 있을 것 같네요 이건 다음 테스트 코드 작성할 때 한번 참고해보세요!

@shon5544
Copy link
Member

shon5544 commented Aug 23, 2023

추가로 하나 요청드리고 싶은 건
JwtTokenManager에서 secretKey 등등 @Value 어노테이션을 사용하는 필드는 따로 클래스로 묶어주셨으면 해요
ex)

public interface SomeInterface {
  String getSecretKey();
  String getAccessTokenExpiration();
  String getRefreshTokenExpiration();
}

// 운영 코드에서 쓰는 클래스
@Component
public class SomeClass implements SomeInterface {
  @Value("${jwt.secretKey}") 
  private String secretKey;

  @Value("${jwt.access.expiration}")
  private Long accessTokenExpiration;

  @Value("${jwt.refresh.expiration}")
  private Long refreshTokenExpiration;

  @Override
  public String getSecretKey(){
  ...
  }
  ...
}

// 테스트 코드에서 쓰는 클래스
public class SomeTestClass implement SomeInterface {
  private String secretKey = "더미값";
  ...

  @Override
  public String getSecretKey() {
  ...
  }
  ...
}
@Component
@Slf4j
public class JwtTokenManager {
    private String secretKey;
    private Long accessTokenExpiration;
    private Long refreshTokenExpiration;
    ...
    public JwtTokenManager(SomeInterface some) {
        secretKey = some.getSecretKey();
        accessTokenExpiration = some.getAccessTokenExpiration();
        refreshTokenExpiration = some.getRefreshTokenExpiration();
    }
    ...
}

@shon5544
Copy link
Member

추가로 하나 요청드리고 싶은 건 JwtTokenManager에서 secretKey 등등 @Value 어노테이션을 사용하는 필드는 따로 클래스로 묶어주셨으면 해요 ex)

public interface SomeInterface {
  String getSecretKey();
  String getAccessTokenExpiration();
  String getRefreshTokenExpiration();
}

// 운영 코드에서 쓰는 클래스
@Component
public class SomeClass implements SomeInterface {
  @Value("${jwt.secretKey}") 
  private String secretKey;

  @Value("${jwt.access.expiration}")
  private Long accessTokenExpiration;

  @Value("${jwt.refresh.expiration}")
  private Long refreshTokenExpiration;

  @Override
  public String getSecretKey(){
  ...
  }
  ...
}

// 테스트 코드에서 쓰는 클래스
public class SomeTestClass implement SomeInterface {
  private String secretKey = "더미값";
  ...

  @Override
  public String getSecretKey() {
  ...
  }
  ...
}
@Component
@Slf4j
public class JwtTokenManager {
    private String secretKey;
    private Long accessTokenExpiration;
    private Long refreshTokenExpiration;
    ...
    public JwtTokenManager(SomeInterface some) {
        secretKey = some.getSecretKey();
        accessTokenExpiration = some.getAccessTokenExpiration();
        refreshTokenExpiration = some.getRefreshTokenExpiration();
    }
    ...
}

이러면 @Value 어노테이션에 대한 의존성이 크게 떨어지니 프로덕션 코드에 setValue 같은 테스트에서만 쓰는 코드를 추가할 이유가 없겠죠?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TEST] UserServiceTest 작성
2 participants