-
Notifications
You must be signed in to change notification settings - Fork 8
test: 지원서 관련 통합테스트 코드 추가 #178
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
...t/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void 동일한_대학을_중복_선택하면_예외_응답을_반환한다() { | ||
| // given | ||
| GpaScore gpaScore = createApprovedGpaScore(테스트유저_1); |
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.
코드를 읽다가 든 생각인데,
이런건 SpringValidation 써서 Dto에서 검증하게 하는게 좋았겠네요🤔
일단 이슈 파뒀습니다!
#182
...t/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
| UniversityApplicantsResponse.of(괌대학_A_지원_정보, | ||
| List.of(ApplicantResponse.of(테스트유저_3_괌대학_A_괌대학_B_그라츠공과대학_지원서, false))), | ||
| UniversityApplicantsResponse.of(괌대학_B_지원_정보, | ||
| List.of(ApplicantResponse.of(테스트유저_2_괌대학_B_괌대학_A_린츠_카톨릭대학_지원서, true))), |
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.
이걸 보면 테스트 데이터에 사용될 유저가 1이 아니라 2를 의도하신게 맞나요?
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.
아 제가 BaseIntegrationTest 고치면서 실수한 거 같네요 유저1이 맞습니다 😅 false로 수정했습니다!
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.
이 테스트가 조금 더 유의미하려면 isMine 까지 포함하는게 좋을 것 같아요,!
그래서 테스트 대상 자체를 유저2로 하고, 유저2의 지원 정보에 isMine true 를 검증하도록 바꾸는건 어떤가요?
@Test
void 이번_학기_전체_지원자를_조회한다() {
// when
ApplicationsResponse response = applicationQueryService.getApplicants(
테스트유저_2.getEmail(),
"",
""
);
// then
assertThat(response.firstChoice()).containsAll(List.of(
UniversityApplicantsResponse.of(괌대학_B_지원_정보,
List.of(ApplicantResponse.of(테스트유저_2_괌대학_B_괌대학_A_린츠_카톨릭대학_지원서, true))),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.
반영했습니다!
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| void 이번학기_지원한_대학이_모두_미선택일_때_경쟁자_목록을_조회한다() { | ||
| //when | ||
| ApplicationsResponse response = applicationQueryService.getApplicantsByUserApplications( | ||
| 테스트유저_6.getEmail() | ||
| ); | ||
|
|
||
| // then | ||
| assertThat(response.firstChoice()).isEmpty(); | ||
| assertThat(response.secondChoice()).isEmpty(); | ||
| assertThat(response.thirdChoice()).isEmpty(); | ||
| } | ||
| } |
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 record UniversityChoiceRequest(
@NotNull(message = "1지망 대학교를 입력해주세요.")
Long firstChoiceUniversityId,
Long secondChoiceUniversityId,
Long thirdChoiceUniversityId) {
}지원서 제출 요청이 "첫번째 대학은 무조건 지정하도록" 되어있거든요!
그런데 서비스 코드에서 이걸 허용하니 테스트가 통과하겠죠,,?
일단 테스트 자체는 서비스 코드를 잘 따르고 있으니, 테스트는 잘 작성하신 것 같아요.
그런데 뭘 따라야할지는 이야기해봐야겠네요.
- 1지망 대학도 선택 안해도 되는지?
- 1지망 대학은 무조건 선택해야 할지?
@wibaek @Gyuhyeok99
여기 코멘트에서 정하고, 따로 PR 파서 정해진 방식대로 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.
기획 의도가 혹시 1지망이 있어야 2지망 선택이 가능하고, 2지망이 있어야 3지망 선택이 가능한 방식인가요?
추가로 지금 로직 보면 1지망이 있어야만 2지망, 3지망을 선택할 수 있지만 2지망 없이 1지망 + 3지망을 선택하는 게 가능해 보이는데, 원래 이런 케이스도 허용되는 게 맞나요? 🤔
만약 1지망 → 2지망 → 3지망 순서대로 선택해야 하는 게 맞다면,
1지망을 @NotNull로 필수 선택하도록 하는 게 더 좋을 것 같습니다.
최소한 1개 대학은 지원해야 하는 게 자연스럽고, DTO에서 검증하면 예외 처리가 단순해질 거 같아요.
이렇지 않으면 1지망 없이 2, 3지망만 선택하는 걸 서비스에서 따로 막아야 해서 불필요하게 복잡해질 거 같아요.
@NotNull을 붙이면 2지망이 있어야만 3지망을 선택할 수 있다는 예외처리만 있으면 될 거 같아요.
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.
추가로 지금 로직 보면 1지망이 있어야만 2지망, 3지망을 선택할 수 있지만 2지망 없이 1지망 + 3지망을 선택하는 게 가능해 보이는데, 원래 이런 케이스도 허용되는 게 맞나요?
지지난 학기까지는 인하대학교에서 교환학생 학교 지원을 1, 2지망만 가능하게 했는데,
지난 학기에서 1,2,3지망까지 선택하도록 바뀌었어요.
그래서 이 코드가 작성될 당시에는 "이후의 지망을 선택하려면 1지망은 반드시 선택해야한다!"는 의도였는데,
3지망까지 확장된 지금, 코드 변경을 안해서 "1, 3지망 선택" 하는 시나리오가 가능해졌네요🫨
이게 바로 장기 프로젝트 코드의 특징 같습니다..
1지망을 NotNull로 필수 선택하도록 하는 게 더 좋을 것 같습니다.
UniversityChoiceRequest.class 코드를 보시면 지금도 그렇게 되어있긴 합니다..!
그래도 이걸로는 1,3지망 조합을 못막으니 규혁님 말씀대로 DTO 검증이 필요해보입니다😓
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.
네 NotNull 은 유지하는게 좋겠습니다👍
이 부분도 이슈에 추가해뒀어요!
src/test/java/com/example/solidconnection/support/integration/BaseIntegrationTest.java
Show resolved
Hide resolved
|
테스트 케이스들을 잘 설정해주신 것 같습니다~ 그리고 왜 서비스 코드에서 제출과 동시에 VerifyStatus.APPROVED로 세팅하는지 모르겠네요.. 🥲 |
nayonsoso
left a comment
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.
반영된 것 확인했습니다😁👍
추가 코멘트 하나 남겼는데요, #178 (comment)
수정 후 빠르게 머지하시라고 approve 드려요.
수고하셨습니다~ 🥳
application이 APPROVE로 자동으로 설정되는 이유는 기존에는 gpa/language 인증을 받은 후에 APPROVE 해줬는데요, 이제 성적 인증 절차와 지원서 절차가 분리되었기 때문에 지원서에 제출되는 성적이 이미 '인증' 되어 있기 때문에 별도의 APPROVE가 필요없다고 가정해서 그렇게 된 것 같습니다. |
관련 이슈
작업 내용
지원서 관련 통합 테스트 코드 추가하였습니다.
리뷰 요구사항 (선택)
조회 관련해서는 시나리오를 어떤식으로 갈지 조금 헷갈려서 기존 e2e테스트를 따라갔습니다. 혹시 더 필요한 테스트나 놓친 거 말씀해주시면 추가하겠습니다!
추가로 VerifyStatus.APPROVED는 나중에 없애든 승인로직을 추가하든 해야겠네요