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

[또링] API 테스트/문서자동화 미션 제출합니다 #2

Merged
merged 39 commits into from
Jun 4, 2020

Conversation

jnsorn
Copy link

@jnsorn jnsorn commented May 26, 2020

잘부탁드립니다😁😁

YebinK and others added 27 commits May 19, 2020 17:06
- 즐겨찾기 추가 인수 테스트 작성
- 단위 테스트(Controller, Service layer) 작성
- 즐겨찾기 조회 인수 테스트 작성
- 단위 테스트(Controller, Service layer) 작성
- 즐겨찾기 삭제 인수 테스트 작성
- 단위 테스트(Controller, Service layer) 작성
- 로그인하지 않은 사용자의 마이페이지, 정보수정, 즐겨찾기 접근 차단
Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

안녕하세요 또링~
리뷰를 맡게 된 재연링입니다!
요구사항에 맞게 잘 구현해주셨습니다 👍
피드백 남겨드렸으니 확인 후 반영해주세요!

import io.jsonwebtoken.Jws;
import io.jsonwebtoken.JwtException;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SignatureAlgorithm;

@Component
public class JwtTokenProvider {
private String secretKey;
private long validityInMilliseconds;

Choose a reason for hiding this comment

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

Duration을 활용한다면 더욱 정확한 의미 전달을 할 수 있어요!

Copy link
Author

@jnsorn jnsorn May 31, 2020

Choose a reason for hiding this comment

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

오 Duration이라는 것이 있었네요! 시간의 길이와 관계된 필드라면 Long보다는 Duration을 이용하는 것이 더욱 정확하겠군요🙂 수정하였습니다!

.filter(favorite -> Objects.equals(favorite.getDepartureId(), departureId))
.filter(favorite -> Objects.equals(favorite.getDestinationId(), destinationId))
.findFirst()
.orElseThrow(AssertionError::new);

Choose a reason for hiding this comment

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

AssertionError로 어떠한 문제가 발생했는지 정확한 의도 전달을 할 수 있을까요?
아래 delete도 같이 고민해보아요.

Copy link
Author

Choose a reason for hiding this comment

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

InvalidFavoriteException을 생성하여 예외 메세지를 통해 정확한 의도를 전달하도록 수정하였습니다!


public FavoriteListResponse findFavorites(Member member) {
Member persistMember = memberRepository.findById(member.getId())
.orElseThrow(IllegalAccessError::new);

Choose a reason for hiding this comment

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

상황에 맞는 정확한 예외처리를 해준다면 어떠한 장점이 있을지 고민해보아요.
이펙티브 자바 아이템 73. 추상화 수준에 맞는 예외를 던지라아이템 75. 예외의 상세 메시지에 실패 관련 정보를 담으라를 참고해주세요!

Copy link
Author

@jnsorn jnsorn May 31, 2020

Choose a reason for hiding this comment

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

상황에 맞게 정확한 예외처리를 해준다면, 이 예외로 인한 다른 예외가 발생하기 전에 전달받은 메세지나 예외 원인들을 통해 알맞은 대처를 할 수 있겠네요🤩

해당 부분에 대해서는 InvalidMemberException를 생성하여 상세메세지를 전달하여 보다 정확한 예외처리를 해주었습니다! 😃

primary key(id)
);


Choose a reason for hiding this comment

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

TRUNCATE TABLE STATION RESTART IDENTITY;
TRUNCATE TABLE LINE RESTART IDENTITY;
TRUNCATE TABLE LINE_STATION RESTART IDENTITY;
TRUNCATE TABLE MEMBER RESTART IDENTITY;
TRUNCATE TABLE FAVORITE RESTART IDENTITY;

현재 테스트가 클린한 DB에서 진행한다는 전제 하에 진행되고 있습니다.
위 조건을 지키기 위해 TRUNCATE를 추가해보면 어떨까요?

가장 좋은 방법은 어떠한 환경에서도 돌아가는 테스트를 만드는 것이 좋지만, 너무 큰 작업이 될 수 있기 때문에 이번 과제에서는 고민만 해보시는게 좋을 것 같습니다 ㅎㅎ

Copy link
Author

@jnsorn jnsorn May 31, 2020

Choose a reason for hiding this comment

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

현재 직접 DB와 통신하는 테스트(인수테스트)를 실행할 때 마다
@Sql("/truncate.sql")를 통해 DB를 초기화 시켜주고 있습니다!

이렇게 따로 파일을 구분한 이유는 실제 서비스가 동작할 때 schema.sql을 실행하여 기본으로 데이터를 셋팅해주고, 실제로 DB와 통신하는 테스트시에는 truncate.sql을 실행하여 DB를 초기화 시켜주기 위함이었습니다!

이러한 상황에서도 재연링은 schema.sql에 truncate sql문을 넣는 방법을 추천하시는 걸까요?😮

Choose a reason for hiding this comment

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

앗 제가 @Sql("/truncate.sql") 코드를 놓쳤군요!
아닙니다~ 또링이 말씀하신 것 처럼 분리하는게 더욱 좋은 방법입니다 👍

build.gradle Outdated
@@ -13,19 +14,40 @@ repositories {
}

dependencies {
compile group: 'com.google.code.gson', name: 'gson'

Choose a reason for hiding this comment

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

gson과 jackson은 동일한 기능을 제공하고 있습니다.
jackson이 있는 상황에서 gson을 추가로 사용하신 이유가 있을까요?

Copy link
Author

@jnsorn jnsorn May 30, 2020

Choose a reason for hiding this comment

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

기존에 Json라이브러리로 Gson을 사용했던 경험이 있어서 무의식적으로 Gson을 사용했습니다.

코멘트를 보고, Jackson과 Gson의 차이에 대해 살펴보다
Jackson이 spring-boot-starter-web에 기본적으로 포함되었음을 알게 되었고,
Spring 프로젝트의 경우 Gson보다는 Jackson을 쓰는 것이 더 적합하다는 생각이 들어
Gson을 이용한 부분들을 수정하였습니다.

더불어, @RestController 사용 시 요청과 응답의 객체 변환 및 직렬화/역직렬화 과정에서 Jackson이 사용되고 있었다는 사실도 알 수 있었습니다. 좋은 지적 감사합니다😊

build.gradle Outdated
Comment on lines 25 to 26
asciidoctor 'org.springframework.restdocs:spring-restdocs-asciidoctor:2.0.4.RELEASE'
testImplementation 'org.springframework.restdocs:spring-restdocs-mockmvc:2.0.4.RELEASE'

Choose a reason for hiding this comment

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

Suggested change
asciidoctor 'org.springframework.restdocs:spring-restdocs-asciidoctor:2.0.4.RELEASE'
testImplementation 'org.springframework.restdocs:spring-restdocs-mockmvc:2.0.4.RELEASE'
asciidoctor 'org.springframework.restdocs:spring-restdocs-asciidoctor'
testImplementation 'org.springframework.restdocs:spring-restdocs-mockmvc'

plugins에서 'io.spring.dependency-management'가 어떠한 역할을 하는지 공부해보세요~

Copy link
Author

Choose a reason for hiding this comment

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

좋은 지적 감사합니다😁
io.spring.dependency-management가 버전을 자동으로 관리해주고 있었네요😂
Springboot를 사용하면서 Springboot의 이점을 제대로 활용하지 못하고 있었다니...😱

Spring dependency manager가 관리해주지 않는 라이브러리들을 제외하고는 명시되어있던 버전을 삭제해주었습니다!

@SpringBootTest
@AutoConfigureMockMvc
@AutoConfigureRestDocs
public class FavoriteControllerTest {

Choose a reason for hiding this comment

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

ControllerTest에서 굉장히 많은 의존성을 가지고 있네요!
Mock 또한 굉장히 많이 사용되고 있는데요.
가능한 Mock을 제거하고 의존성을 줄여보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

JwtTokenProvider에 대한 의존을 줄이기 위해 실제 토큰값을 상수로 관리하였습니다!

ObjectMapper에 대한 의존성도 줄여보기 위해 생각을 해보았는데,
아무리 생각해도 object의 필드를 꺼내서 직접 Json형태로 만들어주는 방향밖에 생각이 안나네요😥
이렇게 된다면 배보다 배꼽이 더 커질 것 같아(테스트 작성시간 보다 Json 문자열 작성 시간이 더 길어질 것 같아)
우선 ObjectMapper를 사용했는데 재연링의 생각은 어떠신지 궁금합니다!

Choose a reason for hiding this comment

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

JwtTokenProvider의 의존도는 사라졌지만, 로직이 변경될 때 마다 토큰을 수정해줘야 할 것 같아요!
토큰이 만료될 수도 있구요 ㅎㅎ
해당 부분은 트레이드 오프해야 하는 부분이 있는데요!

장점

  1. 테스트 대상 범위를 줄여 정확한 부분을 테스트할 수 있다.
  2. 테스트 수행 기간을 줄여 피드백을 빠르게 받을 수 있다.
  3. 코드가 간결해진다.

단점

  1. 테스트를 통과하기가 쉬워진다.
  2. 추후 코드가 변경됐을 때 실패하지 않고 통과할 수 있다.

등이 있을 것 같은데요~
저는 JWT Token을 생성하는 부분이라면 Mock 없이 실제 코드를 사용해도 괜찮지 않을까 했는데 또링의 생각은 어떠신가요!?

Copy link
Author

@jnsorn jnsorn Jun 2, 2020

Choose a reason for hiding this comment

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

오 그런 문제가 있었네요😃 의존성을 줄이는 방법에 대해서만 생각하다보니 토큰을 상수로 저장하는 것에 대한 단점을 생각못했네요!

실제로 전에는 전부 통과했던 테스트 코드가 다시 실행해보니 403에러와 함께 실패하는 걸 확인할 수 있었습니다(아마 상수로 발급받은 토큰의 유효시간이 만료되었겠지요?😮)

프로덕션 코드 변경이 없는 상태에서 그저 시간이 지남에 따라 테스트가 실패하는 것은, 테스트 코드의 목적과 맞지 않는다는 생각이 드네요!

따라서 재연링의 코멘트대로 JWT Token의 메서드를 이용해서 토큰을 생성하는 방향이 더 적합할 것 같아요! ㅎㅎ 자세한 코멘트 감사합니다☺

@ExtendWith(RestDocumentationExtension.class)
@SpringBootTest
@AutoConfigureMockMvc
@AutoConfigureRestDocs
public class MemberControllerTest {

Choose a reason for hiding this comment

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

FavoriteControllerTest와 중복되는 부분을 최대한 줄여보아요~

Copy link
Author

Choose a reason for hiding this comment

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

ControllerTest를 생성하여 공통된 메서드들을 작성하였고, 이를 각 테스트 클래스에서 상속받아 중복되는 부분을 줄여보았습니다!😊

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

수 많은 피드백을 반영하시느라 고생 많으셨습니다 💯
해당 요청은 머지하도록 하겠습니다!

@jaeyeonling jaeyeonling merged commit 916eef7 into woowacourse:jnsorn Jun 4, 2020
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.

None yet

3 participants