-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature: 카테고리별 케이크 이미지 조회 API 구현 #33
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.
고생하셨습니다
Assertions.assertEquals(ReturnCode.SUCCESS.getCode(), response.getReturnCode()); | ||
Assertions.assertEquals(ReturnCode.SUCCESS.getMessage(), response.getReturnMessage()); | ||
Assertions.assertEquals(5, data.cakeImages().size()); | ||
Assertions.assertEquals(6L, data.lastCakeId()); |
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.
lastCakeId가 6L에 대해 검증하는건 테스트 코드 조건에 따라 변경될 수 있는 부분 같은데
CakeDesignCategory.FLOWER에 대한 검증 유무가 더 적합한 테스트라는 생각이 드네요
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.
카테고리에 대한 검증도 추가하겠습니다. 6L에 대한건 페이지네이션 검증이었습니다만, 정말 last id가 6L인지 검증을 추가하겠습니다.
Assertions.assertEquals(ReturnCode.SUCCESS.getCode(), response.getReturnCode()); | ||
Assertions.assertEquals(ReturnCode.SUCCESS.getMessage(), response.getReturnMessage()); | ||
Assertions.assertEquals(5, data.cakeImages().size()); | ||
Assertions.assertEquals(1L, data.lastCakeId()); |
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.
이 부분도 Id 값에 대해 1L라는걸 검증하려면 테스트 queryParam에서 cakeId가 6이니까 제목에서 내림차순 쿼리 정렬이 진행되는게 표현되면 좋을 것 같습니다
혹은 실제 판단하는 목적이 카테고리에 초점이 맞춰지면 더 좋은 테스트이지 아닐까 싶어요!
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.
네, 확인했습니다. 제목 표기는 어떻게 생각하시나요? 태용님이 메서드명에 하시길래 일단 컨벤션을 맞춰보았는데, 아무래도 언더바만 사용한다는 점 등 표기에 제한이 있더라구요. 또 코드상으로 보기엔 너무 가독성이 떨어져서요. (테스트 코드는 문서라는 말이 많은데, 가독성이 떨어지면 문서 역할을 잘 못한다고 생각합니다) 현재 방식 유지가 좋을까요? 아니면 @DisplayName 어노테이션을 활용하는게 좋을까요?? 혹시 메서드 네임으로 하시면서 불편하시진 않았나요??
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.
DisplayName을 활용해볼까요??
메서드 네임으로 하면 저도 한계점이 보이는거 같아요
@SqlGroup({ | ||
@Sql(scripts = { | ||
"/sql/insert-test-user.sql", | ||
"/sql/insert-cake.sql" | ||
}, executionPhase = BEFORE_TEST_METHOD), | ||
@Sql(scripts = "/sql/delete-all.sql", executionPhase = AFTER_TEST_METHOD) | ||
}) |
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.
해당 애노테이션이 추상 클래스에서 선언되는건 안되던가요?? 기억이 잘 안나서 헷갈리네용
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.
스크립트 파일이 테스트마다 다를 것이기에 추상화를 못했습니다
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.
이해했습니다!
#22
카테고리 별 케이크 이미지 조회 API를 구현하였습니다. 통합테스트도 작성해보았는데, Template과 통합테스트 모두 꼼꼼히 보시고 자유롭게 코멘트 남겨주시면 감사하겠습니다.
다음은 커서 페이지네이션 쿼리입니다.