refactor: Application 관련 통합 테스트 데이터 fixture 메서드로 변경 #329
Conversation
Walkthrough
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
28-30: 메서드 이름이 의도를 충분히 전달하지 못합니다 – 새 빌더 팩토리로 보이도록 수정 제안
application()이라는 이름은 도메인 객체 자체를 반환할 것처럼 보이지만, 실제로는 새로운ApplicationFixtureBuilder를 생성합니다.
- 가독성을 높이려면
newBuilder()혹은builder()처럼 생성 목적을 드러내는 네이밍이 좋습니다.- 해당 메서드가 빌더 인스턴스를 새로 만들어 상태 공유 문제를 피하는 설계는 👍 좋지만, 네이밍이 명확해지면 실수 방지가 더 쉬워집니다.
- public ApplicationFixtureBuilder application() { - return new ApplicationFixtureBuilder(applicationRepository); + public ApplicationFixtureBuilder newBuilder() { + return new ApplicationFixtureBuilder(applicationRepository); }src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java (1)
17-36: 메서드 파라미터 과다 – 호출 편의성 개선을 위한 래핑 방법 제안
- 현재
지원서(...)는 8개의 파라미터를 받습니다. 사용자가 순서를 헷갈리면 테스트 오류로 이어질 수 있습니다.
1. 특히 대학 선택 파라미터에null을 넘기며 위치로 의미를 구분해야 하는 부분이 번거롭습니다.- 두 가지 대안으로 가독성을 높여보세요.
1.지원서(Consumer<ApplicationFixtureBuilder> customizer)형태로 빌더를 콜백에 노출 – 필요한 필드만 설정.
2. 대학 선택을List<UniversityInfoForApply>로 받아 내부에서 1·2·3지망을 매핑.- 이 변경은 테스트 호출부를 간결하게 만들고, 파라미터 순서에 따른 실수를 방지합니다.
public Application 지원서(Consumer<ApplicationFixtureBuilder> customizer) { ApplicationFixtureBuilder builder = applicationFixtureBuilder.newBuilder(); customizer.accept(builder); return builder.create(); }src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java (1)
162-169: 반복 루프 내 상태 의존성 – 테스트 실행 시간 최적화 고려
APPLICATION_UPDATE_COUNT_LIMIT만큼 루프를 돌며 실제 서비스 메서드를 호출합니다.
1. 값이 커질 경우 통합 테스트 속도가 느려질 수 있으므로, 로직 검증용이면 루프를 목으로 대체하거나 리미트 값을 줄인 별도 Bean 설정이 유용합니다.
2. 현재 성능 이슈가 없더라도 미래에 상수값이 변경되면 테스트가 느려질 수 있어 미리 대비를 권장합니다.src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java (2)
114-145: 테스트 데이터 중복 생성 – 헬퍼 메서드로 추출하여 가독성 향상
applicationFixture.지원서(...)호출이 여러 테스트에서 동일 패턴으로 반복됩니다.
1. 중복 코드를 헬퍼 메서드(예:createFirstChoiceApplication(user, univ))로 추출하면 각 테스트의 비즈니스 의도만 돋보여 가독성이 좋아집니다.
2. 추가로null자리 파라미터를 줄일 수 있어 오타·순서 실수도 예방됩니다.
293-303: 삭제 플래그 설정 후 save – 트랜잭션 구간 주의
firstApplication.setIsDeleteTrue(); applicationRepository.save(firstApplication);흐름에서
- 동일한 트랜잭션 내부에서 dirty checking 이 자동 저장될 수 있습니다.
- 별도save()호출은 중복될 수 있으니 생략 가능 여부를 확인해 주세요.
- 필요하다면@Transactional경계 안팎을 테스트에서 명확히 구분해 주세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java(1 hunks)src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java(3 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java (1)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
TestComponent(13-86)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java (1)
TestComponent(11-38)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java (1)
59-61: 테스트 환경 프로퍼티 누락 시 실패 가능성 – 기본값 또는 설정 확인 필요
@Value("${university.term}")로 주입되는 값이 테스트 프로퍼티에 정의되지 않으면null이 주입되어 테스트가 실패합니다.
1.application.yml(test)이나@TestPropertySource에 값을 명시했는지 확인해 주세요.
2. 혹은 안전하게 기본값을 지정해 예기치 않은 NPE를 예방할 수 있습니다.@Value("${university.term:2024-1}") // 기본값 예시 private String term;
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java
Show resolved
Hide resolved
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
Application Fixture 생성 메서드 파라미터가 혹시 너무 많은가요?
많긴 하네요..! 하지만 애초에 Application 자체가 커다란 도메인이라 그런 것 같기도 하고요 🥲
일단 저는 '반드시 변경해야한다'는 아니라 approve 드립니다~
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Show resolved
Hide resolved
...t/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
Outdated
Show resolved
Hide resolved
| private RegionFixture regionFixture; | ||
|
|
||
| @Autowired | ||
| private UniversityInfoForApplyFixture universityInfoForApplyFixture; | ||
|
|
||
| @Autowired | ||
| private LanguageRequirementFixture languageRequirementFixture; | ||
|
|
||
| @Autowired | ||
| private GpaScoreFixture gpaScoreFixture; | ||
|
|
||
| @Autowired | ||
| private LanguageTestScoreFixture languageTestScoreFixture; | ||
|
|
||
| @Autowired | ||
| private ApplicationFixture applicationFixture; |
There was a problem hiding this comment.
사실 이 부분에서 멈칫하긴 했어요..
이렇게 fixture를 몇개씩이나 주입받는게 맞나... 🥹
Application 도메인이 복잡한 만큼 이렇게 하는게 어쩔 수 없는것이겠죠?
그래도 지난 리뷰들과 마찬가지로 다소 아쉬운 부분은 공유를 하고 넘어가야할 것 같아 남깁니다!
There was a problem hiding this comment.
이 Fixture들을 조합하는 곳을 또 만들어야하나라는 생각이 들었지만.. 쉽게 해결될 거 같지 않다는 생각이 들어서.. 계속 고민을 해봐야겠네요..🥲
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java (1)
41-58: 픽스처 클래스 주입 방식이 명확합니다여러 픽스처 클래스를 주입받는 방식으로 구조화되었습니다. 각 도메인 영역별로 분리된 픽스처를 사용하여 테스트 데이터를 생성하는 접근 방식이 좋습니다.
다만, 이전 리뷰 코멘트에서도 언급되었듯이 너무 많은 픽스처 의존성을 주입받는 점에 대해서는 지속적인 고민이 필요해 보입니다. 현재 구조가 복잡한 Application 도메인을 테스트하기 위해 불가피한 선택으로 보이지만, 향후 픽스처 조합을 담당하는 상위 레벨의 픽스처 클래스를 고려해볼 수 있을 것 같습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java(3 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java (14)
8-8: 임포트 변경이 테스트 코드의 개선을 반영합니다
- 수정된 임포트 목록:
- 픽스처 클래스들 추가 (ApplicationFixture, RegionFixture 등)
- TestContainerSpringBootTest로 변경
- 환경설정 값 주입을 위한 @value 추가
이 변경은 테스트 데이터 생성 방식의 개선을 잘 보여줍니다. 픽스처 클래스를 활용해 테스트 데이터 생성을 중앙화하는 접근 방식이 코드 가독성과 유지보수성을 높일 것입니다.
Also applies to: 10-10, 13-14, 16-17, 19-20, 25-25
31-31: 테스트 기반 클래스 변경이 적절합니다BaseIntegrationTest에서 @TestContainerSpringBootTest로 변경된 것이 확인됩니다. 이 변경은 PR의 목적인 테스트 데이터 관리 개선과 일치합니다.
Also applies to: 33-33
59-61: 환경 설정에서 학기 정보 주입이 적절합니다테스트에서 사용할 학기 정보를 환경 설정에서 주입받는 방식으로 변경한 것은 좋은 접근입니다. 이를 통해 환경별로 테스트 설정을 유연하게 변경할 수 있게 되었습니다.
62-77: 테스트 데이터 필드가 잘 구조화되었습니다사용자, 점수, 대학 정보 등 테스트에 필요한 데이터를 위한 필드들이 명확하게 정의되었습니다. 이를 통해 테스트 메서드에서 이러한 데이터를 일관되게 참조할 수 있게 되었습니다.
78-95: setUp 메서드의 구현이 명확하고 효율적입니다각 테스트 실행 전에 픽스처를 사용하여 테스트 데이터를 초기화하는 방식이 체계적입니다. 이를 통해 테스트 간 데이터 독립성을 유지하면서도 코드 중복을 줄일 수 있습니다.
PR 설명에서 언급된 것처럼, ApplicationFixture의 파라미터 수가 많아졌지만 이는 재사용성을 극대화하기 위한 합리적인 트레이드오프로 보입니다.
102-132: 지원서 생성 방식이 개선되었습니다이전 코드에서 직접 생성하던 지원서를 ApplicationFixture를 통해 생성하도록 변경한 점이 좋습니다. 이를 통해:
- 테스트 데이터 생성 로직이 중앙화됨
- 테스트 코드의 가독성이 향상됨
- 테스트 간 데이터 생성 일관성이 유지됨
null 값을 명시적으로 전달하는 부분은 필요한 경우 선택적 파라미터를 처리하기 위한 것으로 이해됩니다.
134-149: 테스트 검증 부분이 픽스처 변경에 맞게 잘 조정되었습니다생성된 지원서 객체를 사용하여 테스트를 검증하는 방식이 일관되게 변경되었습니다. 이전 코드와 비교했을 때 테스트의 의도가 더 명확하게 드러납니다.
154-185: 지역 필터링 테스트의 구현이 개선되었습니다지역별 지원자 조회 테스트에서도 픽스처를 활용한 데이터 생성 방식이 일관되게 적용되었습니다. 특히 사용하지 않는 지원서 객체를 변수에 할당하지 않고 메서드 호출만으로 처리한 부분(175-184줄)이 깔끔합니다.
186-199: regionFixture를 활용한 지역 코드 참조가 적절합니다테스트에서 하드코딩된 지역 코드 대신 regionFixture를 통해 지역 정보를 가져오는 방식으로 변경한 것이 좋습니다. 이를 통해 지역 코드가 변경되더라도 테스트 코드를 수정할 필요가 없어졌습니다.
204-249: 대학 이름 필터링 테스트 구현이 일관성 있게 개선되었습니다대학 이름으로 필터링하는 테스트에서도 픽스처를 활용한 접근 방식이 일관되게 적용되었습니다. assertThat 구문에서 containsExactlyInAnyOrder를 사용하여 순서와 무관하게 정확한 요소들이 포함되어 있는지 검증하는 방식이 적절합니다.
253-278: 이전 학기 지원자 필터링 테스트가 명확합니다이전 학기 지원자가 조회되지 않는 테스트에서 학기 값을 직접 "1988-1"로 지정한 부분이 테스트의 의도를 명확하게 보여줍니다. 픽스처를 사용하면서도 테스트별 특수한 조건을 설정할 수 있는 유연성을 잘 보여주는 예시입니다.
282-318: 최신 지원서 조회 테스트가 적절히 구현되었습니다동일 사용자의 여러 지원서 중 최신 지원서만 조회되는 테스트가 픽스처를 활용하여 잘 구현되었습니다. firstApplication.setIsDeleteTrue() 호출을 통해 이전 지원서를 삭제 처리하는 부분이 테스트 시나리오를 정확하게 표현합니다.
326-366: 경쟁자 목록 조회 테스트가 개선되었습니다경쟁자 목록 조회 테스트에서도 픽스처를 활용한 접근 방식이 일관되게 적용되었습니다. 특히 같은 대학에 지원한 사용자들이 경쟁자로 조회되는 로직을 명확하게 테스트하고 있습니다.
371-419: 미선택 대학이 있는 경우의 경쟁자 목록 테스트가 적절합니다지원한 대학 중 미선택이 있을 때 경쟁자 목록 조회 테스트에서 null 값을 적절히 사용하여 지원 대학의 선택/미선택 상태를 표현한 부분이 좋습니다. 1지망, 2지망, 3지망에 대한 검증이 각각 이루어지고 있어 테스트의 완전성이 높습니다.
PR 설명에서 언급된 "지원한 학기 내 모든 대학을 미선택한 경우의 경쟁자 목록 확인" 테스트 케이스가 제거된 것도 해당 시나리오가 실제로는 불가능하다는 점을 고려할 때 적절한 결정으로 보입니다.
2 participants
관련 이슈
작업 내용
Application 관련 통합 테스트 데이터 fixture 메서드로 변경했습니다!
이번 학기 지원한 대학이 모두 미선택일 때 경쟁자 목록을 조회하는 테스트는 시나리오상 불가능한 경우라 삭제했습니다!
특이 사항
리뷰 요구사항 (선택)
Application Fixture 생성 메서드 파라미터가 혹시 너무 많은가요? 최대한 재사용성 높게 하려다보니 좀 많아졌는데 불편할 정도면 좀 분리해보겠습니다!