6주차 : 로그인기능

[tmxhsk99]

안녕하세요!
리뷰 요청드립니다
완성은 못했지만 일단 올리고
시간 날 때 추가하겠습니다
감사합니다.

[hannut91]

아래에서 문자열과 비교하고 있으니 null인지는 확인하지 않아도 될 것 같아요. 그리고 null이 반환될 수 있다는 것 자체가 코드를 이해하기 어렵게 만드는데, 아예 null을 반환하지 않도록 하는게 좋아요. 물론 코틀린처럼 null을 허용하는 문법을 제공한다면 좀 다르긴 하지만요.

[hannut91]

JwtUtil jwtUtil = new JwtUtil(SECRET);은 상수는 아니라서 구분해 주는 게 좋을 것 같아요'

    private final String SECRET = "12345678901234567890123456789010";
    private final String VALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGw";
    private final Long USER_ID = 1L;

    JwtUtil jwtUtil = new JwtUtil(SECRET);

[hannut91]

커밋을 작게 나눌려고 노력하시는 군요. 좋은 커밋을 작성하는 방법 중 하나를 소개해 드리고 싶네요.

커밋은 제목과 본문이 있는데요. 제목은 가장 위에, 본문은 한 줄 띄고 그 다음부터가 본문이죠.

커밋의 제목은 이 커밋을 추가한 이유가 드러나야하고, 본문은 그 이유를 달성하기 위해서 실제로 한 일을 구체적으로 적어주어야 해요.

test : SessionController login 테스트 수정

이 수정이라는 말은 너무 보편적인 말이에요. 실제로 커밋의 한 일을 보면 @SpringBootTest대신에 @WebMvcTest를 사용하도록 바꿨고, authenticationService을 실제 객체를 사용하던 것에서 Mock 객체를 사용하도록 바꿨으며, 응답을 검사할 때 문자열을 검사하는 것에서 jsonPath로 검사하는 것으로 바꿧어요.

위 일들을 의미를 커밋에 담으려면 지금 하신 것처럼 수정이라는 말 밖에 담지 못하게 돼죠. 즉 내가 너무 보편적인 말을 사용하고 있다면 너무 많은 일을 하는 것이 아닌가? 생각해바야 해요. 이건 함수 이름이나 변수 이름을 지을때도 마찬가지에요. 함수의 이름을 짓는게 너무 어렵다면, 너무 많은 일을 하는 것이 아닌가 의심을 해봐야 해요

In other cases, the verb is vague because the operations performed by the routine are vague. - 코드 컴플리트 2 - 7.3 좋은 루틴 이름

그래서 정말 이유가 드러나도록 커밋을 작성한다면 커밋은 다음과 같이 나눌 수 있을것 같아요.

불필요한 통합 테스트 대신 레이어만 테스트하라

세션 컨트롤러에서 통합 테스트를 작성하고 있었습니다.
서비스 레이어에서 통합 테스트로 작성하고 있기 때문에 컨트롤러에서는 
해당 레이어만 테스트만 해도 충분합니다.
따라서 `@SpringBootTest`를 `@WebMvcTest`로 교체하고 `AuthenticationService`를 Mocking하여 테스트를 작성했습니다.

본문은 이런식으로 쓰면 좋아요.
현재는 ~~~~ 하다. 그런데 ~~~~해야 한다.
그래서 ~~~~, ~~~, ~~~ 를 했다.

대부분 사람들이 다른 사람들도 내가 아는 지식을 알 것이라고 가정하는 실수를 하기 때문에 기존이라는 말을 많이 사용하는데요.
이건 위험합니다. 대신에 현재 상황을 짚어주고, 왜 이걸 바꿔야 했는지 설명해주고, 그걸 달성하기 위해서 구체적으로 무엇을 했는지 작성해 주는 것이 좋습니다

[tmxhsk99]

뭔가 커밋 메시지를 작성하면서도 항상 탐탁치 않은 부분이 있었는데
좋은 말씀 감사드립니다!

[hannut91]

로그인한 경우와 로그인 하지 않은 경우를 테스트를 작성해 주셨네요. 위에선는 AUTH_USER_DATA을 미리 만들어 놓고 사용했었는데, 여기서는 주어진 상황이 달라짐을 명시하기 위해서 IS_NOT_EXISTS_USER_DATA 여기서 만들어주신 것 같아요.

테스트용 데이터를 미리 만들어 놓으려면 둘다 미리 만들어 놓던지 아니면은 각 Context마다 만들던지 일관성있게 맞춰주는게 좋을 것 같아요.

@Nested
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class login_메서드는 {
  private AuthenticationService authenticationService = new AuthenticationService(getUserRepository(), getJwtUtil());

  UserLoginData loginData;

  @Nested
  @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
  class 유효한_유저로그인정보_요청를_받으면 {
  @BeforeEach
  void setUp() {
    getUserRepository().deleteAll();
    getUserRepository().save(User.builder()
      .name(AUTH_NAME)
      .email(AUTH_EMAIL)
      .password(AUTH_PASSWORD)
      .build());

    loginData = UserLoginData.builder()
        .email(AUTH_EMAIL)
        .password(AUTH_PASSWORD)
        .build();
  }

  @DisplayName("인증토큰을 반환한다.")
  @Test
  void It_returns_token() {
    String accessToken = authenticationService.login(loginData);
    Assertions.assertThat(accessToken).isEqualTo(VALID_TOKEN);
  }
}

@Nested
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class 유효하지_않은_로그인정보를_받으면 {
  @BeforeEach
  void setUp() {
    loginData = UserLoginData.builder()
        .email("InvalidEmail")
        .password(AUTH_PASSWORD).build();
  }

  @DisplayName("해당 정보의 회원이 존재하지 않으면 UserNotFoundException을 반환한다.")
  @Test
  void It_throws_UserNotFoundException() {
    Assertions.assertThatThrownBy(() -> authenticationService.login(loginData)).isInstanceOf(UserNotFoundException.class);
  }
}

[hannut91]

유효하지 않은 경우도 테스트를 작성해서 스펙을 정의해줘도 좋겠네요

[hannut91]

이렇게 반복으로 사용되는 테스트용 데이터를 위에서 미리 정의해놓고, 여기서는 사용만 하면 이 부분이 좀 더 단순해질 것 같아요.
테스트용 데이터를 fixture라고 부르는데, 믿고 사용할 수 있는 테스트용 데이터를 미리 정의해놓는 거죠.

그런데 이러한 테스트용 데이터도 한계가 생길 수 있는데 지금처럼 테스트에 필요한 데이터가 점점 증가하기 때문이에요.
이메일이 없는 경우
이메일 형식이 잘못된 경우
비밀번호가 틀린 경우
비밀번호 형식이 틀린경우

이렇게 나열하다보면 끝없이 테스트용 데이터를 정의해줘야하고 그러다 보면 구분하기 힘들어질 수도 있어요. 그러면 아마도
테스트 데이터를 만들어주는 헬퍼 함수같은 것을 도입하고 싶은 마음이 생길거예요

[hannut91]

isGetMethod가 삐져나왔네요. GET 메서드일 때도 인증을 확인하는거죠?

[tmxhsk99]

앗 이것은 실수입니다;;

    @Override
    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {

        if (isGetMethod(request)) {
            return true;
        }
        
        if (isPostMethod(request) || isPatchMethod(request) || isDeleteMethod(request)) {
            return checkAccessToken(request);
        }

        return true;
    }

이렇게 하려고 했습니다;;;

[hannut91]

        if (authorization == null) {
            throw new AccessTokenNotFoundException();
        }

한 줄이라도 중괄호를 써주는게 더 보기 좋을 수도 있어요

[hannut91]

try catch문 아래에 코드가 있는 것은 이해하기 어려울 수 있어요.
try 안에 있는 값을 바로 반환하게 하는게 좋을 것 같아요

[hannut91]

실제 db를 지우지 않도록 조심해야겠네요.

저는 테스트를 실행할 때는 Profile을 test로 하게 하고, 설정에서 테스트 DB를 사용하도록 했어요.

// build.gradle.kts
tasks.withType<Test> {
    useJUnitPlatform()
    environment(mapOf("SPRING_PROFILES_ACTIVE" to "test"))
}

spring:
  datasource: ...

---

spring:
  profiles: test
  datasource:
    url: jdbc:mariadb://localhost:3306/codesoomtest
    username: root
    password: root1234
    driver-class-name: org.mariadb.jdbc.Driver

[tmxhsk99]

H2를 보통 테스트 환경에서 쓰다 보니까
인지 못했네요 실제 회사에서는
신경써서 하겠습니다!

[tmxhsk99]

한 주 동안 고생하셨습니다!
평일에 많이 해놔야 되는데 생각보다 그렇게 못했네요;;
윤석님 내일 인프콘 발표하시는 것도 잘 되시기를 바라겠습니다
저도 인프콘 갑니다 기회 되면 인사 드리겠습니다
수고하셨습니다