-
Notifications
You must be signed in to change notification settings - Fork 240
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
🚀 3단계 - 즐겨찾기 기능 구현 #558
base: hellonayeon
Are you sure you want to change the base?
🚀 3단계 - 즐겨찾기 기능 구현 #558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 나연님 😃
깔끔하게 잘 구현해 주셨네요 👍
몇 가지 코멘트 남겨두었는데 확인 부탁드려요!
public Favorite() { | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엔티티의 기본 생성자는 protected로 선언해 보시는 것도 추천드립니다 😃
@Service | ||
@RequiredArgsConstructor | ||
public class FavoriteService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스프링에서 제공하는 transactional을 적절히 활용해 보셔도 좋을 것 같아요
/** | ||
* Given: 로그인 토큰을 발급받는다. | ||
* When: 즐겨찾기를 등록한다. | ||
* Then: 실패(500 InternalServerError) 응답을 받는다. | ||
*/ | ||
@Test | ||
@DisplayName("[실패] 없는 지하철역을 즐겨찾기로 등록한다.") | ||
void 없는_지하철역을_즐겨찾기로_등록한다() { | ||
// Given | ||
String 로그인_토큰 = 로그인_요청(회원_이메일, 회원_패스워드).as(TokenResponse.class).getAccessToken(); | ||
|
||
// When | ||
ExtractableResponse<Response> 즐겨찾기_생성_응답 = 즐겨찾기_생성_요청(로그인_토큰, new FavoriteRequest(교대역, 9999L)); | ||
|
||
// Then | ||
HTTP_응답_상태코드_검증(즐겨찾기_생성_응답.statusCode(), HttpStatus.BAD_REQUEST); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석의 then절이 코드와 일치하지 않네요 😢(docs의 README에도 500 응답 코드에 대한 내용은 없네요)
인수 테스트는 블랙박스 테스트이니 응답 코드와 같은 구현 상세 내용은 포함하지 않아도 될 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 READM 와 주석 내용이 일치하도록 주석 수정했습니다!
(이전 미션에서는 ErrorCode 와 Exception 을 정의해서 사용했는데, 두 주차 미션을 같이 수행하며 sample 코드를 사용해서 이번 미션에서는 예외를 커스텀하지 않았습니다.)
막연하게 API 성공여부를 확인하기 위해 응답상태코드를 검증해야한다 생각했는데, 용주님 피드백 듣고 많은 생각이 드네요.
말씀처럼 인수테스트는 블랙박스 테스트이니 응답 코드와 같이 구현 상세 내용은 굳이 포함하지 않아도 될 것 같아요 👍
정상적으로 삭제됐는지 확인하고 싶다면 추가로 조회 API를 호출해서 확인하는게 더 정확할 것 같습니다.
또는 실패했을 시 원하는 메시지가 리턴되는지를 확인하기 위해 Exception Message 를 검증하는 방법도 있겠네요.
(예외 메시지 검증을 위한 TC를 새로 정의해도 될 것 같구요.)
노선_응답_실패_검증(구간_생성_응답, SubwayErrorCode.CANNOT_CREATE_SECTION);
public static void 노선_응답_실패_검증(ExtractableResponse<Response> response, SubwayErrorCode errorCode) {
// assertThat(response.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST.value());
ErrorResponse 오류_응답 = response.as(ErrorResponse.class);
assertThat(오류_응답.getMessage()).isEqualTo(errorCode.getMessage());
}
저는 생각해보지 못한 인사이트 공유 감사드립니다 😁
private void 즐겨찾기_목록_조회_검증(ExtractableResponse<Response> response, List<SourceTarget> sourceTargets) { | ||
HTTP_응답_상태코드_검증(response.statusCode(), HttpStatus.OK); | ||
|
||
List<FavoriteResponse> favoriteResponseList = response.jsonPath() | ||
.getList("", FavoriteResponse.class); | ||
|
||
int totalAssertionSourceTarget = sourceTargets.size(); | ||
int matchSourceTarget = 0; | ||
for (FavoriteResponse favoriteResponse : favoriteResponseList) { | ||
for (SourceTarget sourceTarget : sourceTargets) { | ||
if (sourceTarget.source.equals(favoriteResponse.getSource().getId()) | ||
&& sourceTarget.target.equals(favoriteResponse.getTarget().getId())) { | ||
matchSourceTarget++; | ||
} | ||
} | ||
} | ||
assertThat(matchSourceTarget).isEqualTo(totalAssertionSourceTarget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 내부의 if, for 로직에 이슈가 발생하면 테스트를 신뢰하기 어려울 것 같아요
favoriteResponseList의 타입을 List<SourceTarget>
으로 변경해서 sourceTargets와 직접 비교하면 어떨까요?
기능 요구사항
리팩토링 항목