-
Notifications
You must be signed in to change notification settings - Fork 4
Feat/52 api page and search #104
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
Conversation
…page-and-search' into feat/52-api-page-and-search
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
============================================
- Coverage 84.90% 84.31% -0.59%
- Complexity 135 148 +13
============================================
Files 41 43 +2
Lines 583 676 +93
Branches 15 30 +15
============================================
+ Hits 495 570 +75
- Misses 78 86 +8
- Partials 10 20 +10
☔ View full report in Codecov by Sentry. |
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.
PR이 너무 거대해져서 보기가 힘드네요.
모든 PR은 200 ~ 300줄 내외가 되고 1~2일 이내에 리뷰를 완료하여 리뷰의 병목을 해소할 수 있었습니다 뱅크샐러드
물런 신규 기능 추가에 200 ~ 300 줄은 무리겠지만, 앞으로는 단위를 조금 더 쪼개거나, 한 PR에 담더라도, 중간 리뷰 요청해주세요. (PR의 목적은 단순 기능 추가 보다는 코드 리뷰에 중점이 있다고 생각합니다)
현재도 리뷰 남기지만 전체 흐름 파악은 불가능하고, 그냥 단순히 한줄한줄 단위로 읽었습니다.
전체적 개선사항
- 추가한 기능에 대한 테스트 코드 추가
- 전체적으로 page, sort 관련 테스트 추가해주세요. (coverage rule 관련해서도 추가 테스트 코드가 필요해보입니다)
- empty list의 경우
{ data: [] }
를 리턴한다는 테스트 코드를 컨트롤러테스트로 만들어주세요. - 전체적으로 header에 Setting하는 방식의 자동화가 필요해보입니다. 현재와 같은 방식은 사용불가능해보입니다. (개발자가 직접 기억하고, 필요한 헤더를 세팅해야 한다)
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkControllerTest.kt
Show resolved
Hide resolved
src/test/kotlin/com/group4/ticketingservice/event/EventServiceTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/com/group4/ticketingservice/event/EventControllerTest.kt
Outdated
Show resolved
Hide resolved
src/integrationTest/kotlin/com/group4/ticketingservice/actuator/ActuatorTest.kt
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/EventController.kt
Show resolved
Hide resolved
헤더 관련해서 당장 처리가 힘들다면, 일단 해당 Branch는 Page, Sort 관련해서만 진행했으면 좋겠습니다. (
위에서도 말했지만 현재 작업 내용이 너무 많습니다. |
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.
pageable sort관련 test가 integrationtest에 필요해보입니다
What is this PR?
Key Changes
Test Checklist