refactor: 대학 관련 통합 테스트 데이터 정의 구조 개선 #286
Conversation
Walkthrough
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (13)
✨ 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: 3
🧹 Nitpick comments (4)
src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java (1)
114-114: 하드코딩된 regionCode 값"AMERICAS"와 같은 문자열 값을 하드코딩하기보다 상수나 열거형을 사용하는 것이 좋습니다. 다른 테스트나 코드에서 동일한 값이 필요할 때 일관성을 유지하기 어렵습니다.
상수나 열거형을 사용하여 regionCode를 참조하는 것이 더 안전합니다:
- String regionCode = "AMERICAS"; + String regionCode = RegionCode.AMERICAS.getCode(); // 예시 - 실제 코드에 맞게 수정 필요src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureHelper.java (3)
22-34: 스프링 컴포넌트 설정이 적절합니다.
컴포넌트 구성
@Component어노테이션으로 스프링 빈으로 등록하여 테스트에서 쉽게 사용할 수 있습니다.- 필요한 모든 픽스처 의존성이 잘 주입되었습니다.
설정값 활용
@Value("${university.term}")어노테이션으로 외부 설정값을 활용한 점이 좋습니다.단,
term필드가 public으로 설정되어 있습니다. 캡슐화를 위해 private으로 변경하고 필요시 getter를 제공하는 것이 좋습니다.- public String term; + private String term;
78-93: 언어 요구사항 생성 로직에 개선 여지가 있습니다.
현재 구현
- 각 언어 요구사항을 생성하고 Set에 추가한 후, 대학 정보에도 추가하는 방식입니다.
- 양방향 관계를 관리하기 위한 좋은 접근법입니다.
개선 가능성
- 자바 스트림 API를 활용하면 코드를 더 간결하게 만들 수 있습니다.
languageRequirementMap매개변수에 대한 null 체크가 없습니다.다음과 같이 스트림 API를 활용하여 코드를 간결하게 개선할 수 있습니다:
- Set<LanguageRequirement> languageRequirements = new HashSet<>(); - for (Map.Entry<LanguageTestType, String> entry : languageRequirementMap.entrySet()) { - LanguageRequirement languageRequirement = languageRequirementFixture.languageRequirement() - .languageTestType(entry.getKey()) - .minScore(entry.getValue()) - .universityInfoForApply(universityInfoForApply) - .create(); - languageRequirements.add(languageRequirement); - universityInfoForApply.addLanguageRequirements(languageRequirement); - } + Set<LanguageRequirement> languageRequirements = languageRequirementMap.entrySet().stream() + .map(entry -> { + LanguageRequirement languageRequirement = languageRequirementFixture.languageRequirement() + .languageTestType(entry.getKey()) + .minScore(entry.getValue()) + .universityInfoForApply(universityInfoForApply) + .create(); + universityInfoForApply.addLanguageRequirements(languageRequirement); + return languageRequirement; + }) + .collect(Collectors.toSet());또한
languageRequirementMap에 대한 null 체크를 추가하는 것이 안전합니다:+ if (languageRequirementMap == null) { + throw new IllegalArgumentException("languageRequirementMap must not be null"); + } Set<LanguageRequirement> languageRequirements = new HashSet<>(); for (Map.Entry<LanguageTestType, String> entry : languageRequirementMap.entrySet()) {
1-103: 전체적인 코드 품질에 대한 제안사항
문서화 개선
- 클래스와 메서드에 JavaDoc 주석을 추가하면 다른 개발자들이 이 클래스의 목적과 사용법을 더 쉽게 이해할 수 있습니다.
예외 처리
- 픽스처 생성 중 발생할 수 있는 예외에 대한 처리가 없습니다. PR 설명에서 언급된 것처럼 사용자 정의 예외를 도입하는 것을 고려해볼 수 있습니다.
테스트 커버리지
- 이 픽스처 헬퍼 자체에 대한 단위 테스트를 추가하는 것도 고려해보세요.
예시 JavaDoc 주석:
/** * 대학 관련 테스트 픽스처를 생성하는 헬퍼 클래스입니다. * 대학, 지역, 국가, 대학 정보, 언어 요구사항 등의 연관 엔티티를 함께 생성합니다. */ @Component @RequiredArgsConstructor public class UniversityFixtureHelper { /** * 괌대학(A형) 테스트 데이터를 생성합니다. * 이 메서드는 미국 괌대학의 기본 언어 요구사항(TOEFL iBT 80, TOEIC 800)을 포함한 * 전체 대학 관련 엔티티를 생성합니다. * * @return 생성된 모든 대학 관련 엔티티를 포함하는 UniversityData 객체 */ public UniversityData 괌대학_A_생성() { // 기존 코드 } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build.gradle(1 hunks)src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/LanguageRequirementFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixture.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityData.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureHelper.java(1 hunks)src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
Component(8-38)
src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java (2)
src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
Component(9-45)src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
Component(8-38)
src/test/java/com/example/solidconnection/support/fixture/LanguageRequirementFixture.java (4)
src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
Component(9-45)src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
Component(8-38)src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java (1)
Component(10-66)src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixture.java (1)
Component(14-56)
🔇 Additional comments (19)
build.gradle (1)
61-62: 테스트 환경에 Lombok 의존성 추가에 대한 긍정적인 평가테스트 코드에 Lombok을 추가한 변경사항이 좋아 보입니다! 이 변경을 통해 다음과 같은 이점이 있습니다:
테스트 코드에서도 Lombok 기능 사용 가능
@RequiredArgsConstructor와 같은 생성자 자동화- Getter/Setter 자동 생성으로 테스트 코드 간소화
테스트 고유 어노테이션 프로세싱 지원
- 테스트 빌드 과정에서 Lombok 어노테이션 처리
- 테스트 환경에서 독립적인 Lombok 설정 가능
테스트 fixture 클래스들이 이 의존성을 활용하여 더 깔끔한 코드를 작성할 수 있게 되었습니다.
src/test/java/com/example/solidconnection/university/fixture/UniversityData.java (1)
1-18: 대학 데이터 그룹화를 위한 레코드 도입 적절함새로 추가된
UniversityData레코드가 매우 유용해 보입니다! 대학 관련 데이터를 효과적으로 캡슐화하고 있습니다.
레코드 사용의 장점
- 불변 데이터 구조로 테스트 데이터의 일관성 보장
- 자동 생성되는 getter, equals, hashCode 메서드로 코드 간소화
- 데이터 전달 객체(DTO) 역할에 최적화된 구조
필드 구성의 명확성
- 한글 필드명으로 도메인 언어 일관성 유지
- 관련 엔티티들을 논리적으로 그룹화
개선점 제안
- 필요하다면 레코드에 유틸리티 메서드를 추가하여 데이터 검증이나 변환 기능을 제공할 수 있습니다
이 레코드는 테스트 데이터 관리와 가독성 향상에 큰 도움이 될 것입니다.
src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
1-45: 국가 테스트 데이터 생성을 위한 빌더 패턴 구현 적절함
CountryFixture클래스가 잘 설계되었습니다! 빌더 패턴을 활용한 유연한 테스트 데이터 생성 방식이 인상적입니다.
구조적 장점
@Component와@RequiredArgsConstructor를 통한 깔끔한 의존성 주입- 내부 빌더 클래스를 통한 직관적인 메서드 체이닝 지원
- 필수 속성(code, koreanName, region)을 명확하게 설정 가능
사용성 측면
country()메서드로 빌더 인스턴스 쉽게 생성- 각 속성별 설정 메서드가 자기 자신(this)을 반환하여 체이닝 지원
create()메서드에서 저장소를 통한 영속화까지 자동 처리개선 제안
- 필수 속성에 대한 유효성 검사 로직을 추가하면 더 안정적인 테스트 환경을 구축할 수 있습니다
- 자주 사용되는 국가 데이터에 대한 정적 팩토리 메서드 추가를 고려해보세요 (예:
createKorea(),createUSA()등)이 fixture 클래스는 테스트 코드의 가독성과 유지보수성을 크게 향상시킬 것입니다.
src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
1-38: 지역 테스트 데이터 생성을 위한 빌더 패턴 구현 우수함
RegionFixture클래스가 매우 깔끔하게 설계되었습니다! 다른 fixture 클래스들과 일관된 패턴을 유지하고 있어 좋습니다.
구조적 특징
- Spring
@Component와 Lombok@RequiredArgsConstructor를 통한 간결한 의존성 주입- 내부 빌더 클래스를 통한 유연한 객체 생성 지원
- 필요한 속성(code, koreanName)에 대한 설정 메서드 제공
메서드 체이닝 구현
- 각 setter 메서드가 자기 자신(this)을 반환하여 자연스러운 체이닝 가능
region()메서드로 빌더 인스턴스를 쉽게 생성 가능create()메서드에서 영속화까지 한번에 처리하는 편의성개선 가능한 부분
CountryFixture와 마찬가지로 자주 사용되는 지역에 대한 정적 팩토리 메서드 추가 고려 (예:createAsia(),createEurope()등)- 필수 필드에 대한 유효성 검사 로직 추가를 검토해보세요
전체적으로 테스트 코드의 가독성과 재사용성을 높이는 훌륭한 구현입니다.
src/test/java/com/example/solidconnection/support/fixture/LanguageRequirementFixture.java (1)
1-51: 새로운 LanguageRequirementFixture 클래스가 잘 구현되었네요!개선 사항:
코드 구조 및 패턴
- 빌더 패턴이 일관성 있게 구현되었습니다
- 메서드 체이닝이 적절히 활용되었습니다
- 다른 Fixture 클래스들과 일치하는 패턴을 사용하고 있습니다
의존성 주입
@RequiredArgsConstructor를 통한 의존성 주입이 깔끔하게 처리되었습니다- Repository 사용이 적절합니다
테스트 데이터 생성을 위한 좋은 보조 클래스입니다. PR 목표에 맞게 잘 구현되었습니다.
src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java (1)
1-66: 대학 Fixture 클래스가 효과적으로 구현되었습니다!개선 사항:
코드 구조 및 패턴
- final 클래스로 선언하여 확장 방지를 고려한 점이 좋습니다
- 빌더 패턴이 일관되게 구현되었습니다
- 메서드 체이닝이 적절히 사용되었습니다
고정 값 처리
- URL 필드들에 대한 고정 값 처리가 테스트 목적에 적합합니다
- 실제 테스트에 중요한 필드들만 커스터마이징 가능하도록 설계된 점이 좋습니다
PR 목표에 언급된 "비중요 검증 데이터는 고정값으로 설정"한다는 방침에 맞게 잘 구현되었습니다.
src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixture.java (1)
1-56: UniversityInfoForApply Fixture 클래스가 잘 구현되었습니다!개선 사항:
코드 구조 및 패턴
- 빌더 패턴이 일관되게 구현되었습니다
- 필요한 필드만 커스터마이징할 수 있도록 설계되었습니다
- 다른 Fixture 클래스들과 일치하는 패턴을 사용하고 있습니다
Enum 값 처리
- 열거형 상수를 위한 정적 임포트가 깔끔하게 사용되었습니다
HOME_UNIVERSITY_PAYMENT와ONE_SEMESTER같은 상수 사용이 적절합니다고정 값 처리
- 테스트에 중요하지 않은 필드들은 고정 값으로 설정하여 단순화했습니다
- HashSet 초기화가 적절히 사용되었습니다
PR 목표에 맞게 잘 구현되었으며, 메서드 체이닝 접근 방식이 효과적으로 적용되었습니다.
src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java (8)
3-5: TestContainerSpringBootTest 애노테이션을 사용한 좋은 리팩토링입니다BaseIntegrationTest를 상속하는 대신 @TestContainerSpringBootTest 애노테이션을 사용하는 방식으로 변경하셨네요. 이는 테스트 설정을 더 유연하게 만들고 명시적으로 보여주는 좋은 접근 방식입니다.
9-10: UniversityData와 UniversityFixtureHelper 활용이 좋습니다새로운 fixture 헬퍼 클래스와 데이터 구조체를 임포트하여 테스트 데이터 생성 과정을 단순화했습니다. PR 목표에서 언급된 "도메인별로 fixture를 분리"하는 접근 방식이 잘 구현되었습니다.
13-13: @beforeeach 설정이 적절합니다테스트 데이터 준비를 위해 @beforeeach 애노테이션을 사용한 것은 좋은 방식입니다. 각 테스트마다 필요한 데이터가 초기화됩니다.
27-29: TestContainerSpringBootTest 적용과 클래스 선언 변경이 적절합니다BaseIntegrationTest 상속 대신 @TestContainerSpringBootTest 애노테이션을 사용하고, class 앞의 public 제거는 최신 Java 코딩 스타일에 맞는 좋은 변경입니다.
34-48: 테스트 데이터 설정 리팩토링이 잘 되었습니다UniversityFixtureHelper를 주입받고 @beforeeach 메서드에서 테스트 데이터를 생성하는 방식으로 변경한 것은 테스트 코드를 더 읽기 쉽고 유지보수하기 좋게 만듭니다. 한글 변수명을 사용하여 의미를 명확히 한 점도 좋습니다.
52-59: 대학 상세정보 테스트 코드가 간결해졌습니다fixture 헬퍼를 통해 생성된 데이터를 사용하여 테스트 코드가 훨씬 간결해졌습니다. 이제 테스트의 의도가 더 명확하게 보입니다.
95-108: 테스트 검증부의 주석 처리된 부분에 대한 의견대부분의 검증 코드가 주석 처리되어 있습니다. 이것이 리팩토링 과정의 일시적인 상태인지, 아니면 의도적인 변경인지 확인이 필요합니다. 리팩토링이 완료되면 필요한 검증을 다시 활성화하는 것이 좋겠습니다.
리팩토링 과정에서 일시적으로 주석 처리된 코드인지, 아니면 의도적으로 하나의 fixture만 검증하도록 변경한 것인지 확인해주시겠어요?
140-146: 지역 필터링 테스트의 주석 처리된 검증 코드이 테스트에서도 대부분의 검증 코드가 주석 처리되어 있습니다. 현재는 하나의 fixture만 검증하고 있는데, 이는 테스트 범위가 축소될 수 있습니다. 리팩토링 완료 후 테스트 커버리지를 복원하는 것이 좋겠습니다.
다양한 대학 데이터에 대한 검증이 필요한지, 아니면 현재와 같이 단일 데이터로 충분한지 검토해주시겠어요?
src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureHelper.java (4)
1-21: 전반적인 클래스 구조가 잘 설계되었습니다.
패키지 구조와 임포트
- 대학 관련 픽스처를 위한 전용 패키지를 만든 점이 좋습니다.
- 필요한 의존성들을 명확하게 임포트했습니다.
Lombok 활용
@RequiredArgsConstructor를 사용하여 의존성 주입을 간결하게 처리했습니다.
35-46: 메서드 이름과 매개변수가 명확합니다.
한국어 메서드 이름
괌대학_A_생성()과 같은 한국어 메서드 이름은 팀의 컨벤션을 따른다면 문제 없습니다.- 테스트 목적에 맞게 구체적인 이름을 사용했습니다.
매개변수 전달
- 필요한 모든 정보를 명확하게 전달하고 있습니다.
48-77: 대학 생성 메서드의 전반부가 잘 구성되었습니다.
메서드 매개변수
- 필요한 모든 정보가 명시적인 매개변수로 전달되어 가독성이 좋습니다.
엔티티 생성 체이닝
- Region, Country, University 객체를 빌더 패턴과 메서드 체이닝을 통해 생성하는 방식이 깔끔합니다.
- 각 엔티티 간의 관계가 명확하게 설정되었습니다.
95-102: 반환 객체 생성이 깔끔합니다.
- UniversityData 반환
- 생성된 모든 엔티티를 하나의 데이터 객체로 묶어 반환하는 방식이 좋습니다.
- 테스트에서 필요한 모든 관련 엔티티에 쉽게 접근할 수 있습니다.
src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
고민 많이 하신 흔적이 보이는 코드네요. 고생 많으셨습니다👏
몇 가지 피드백과 함께, 제가 생각해본 대안도 같이 정리해봤습니다.
UniversityData와 UniversityFixtureHelper 관련
UniversityData를 추가해서 대학 도메인들을 더 편하게 다룰 수 있도록 해봤는데, 앞으로도 이런 식으로 다른 대학을 쭉쭉 만들면 괜찮을 것 같아요. 한 번 검토 부탁드려요!
테스트 전용 도메인을 따로 설계하는 것도 방법이지만,
가능하면 실제 도메인 모델을 그대로 사용하는 쪽이 유지보수나 가독성 면에서 더 좋다고 생각해요.
특히 UniversityFixtureHelper의 대학_생성() 메서드는 8개의 연속된 문자열 인자 + Map을 넘기게 되어 있더라고요.
- 타입이 모두 String 인자 순서를 실수하기 쉽고 (token과 subject도 헷갈렸던거 기억하시나요 ㅎㅎ..)
- 어떤 값이 들어가야 할지 직관적으로 파악하기 어렵고
- 무엇보다 우리가 회의 때 이야기했던 “도메인별로 분리해서 변경 전파 최소화” 라는 방향성과 어긋난다 생각해요.
각 도메인의 fixture를 나누긴 했지만, 결국 UniversityData에서 이들을 한 번에 호출하면서 다시 의존성이 생기고 있어서, 처음 의도했던 목표가 절반만 달성된 셈이라 생각됩니다🥲
메서드 체이닝 + create() 조합
메서드 체이닝 방식으로 create()에서 해당 객체를 반환하도록 바꿔봤어요!
너무 잘하셨어요! 특히 SiteUser 쪽도 같은 방식으로 리팩토링하면 테스트가 훨씬 읽기 좋아지겠네요.
국가나 지역 같은 경우는 재사용이 되니까, 이미 존재하면 DB에서 찾아오는 방식으로 바꿔야겠네요.
create 메서드 수정에 대해서 제가 생각해본 것이 있는데 더 밑에 한번에 말씀드릴게요~
기타 피드백
- .java 파일 하나의 책임을 최대한 작게 가져가는 게 좋다고 생각해요. xxxFixture와 xxxFixtureBuilder를 분리해보는건 어떤가요?
- university.formatName 필드는 “S3 이미지 저장을 위한 이름”입니다. 이를 테스트에서 동적으로 바꿀 일은 없을테니 고정값으로 두는 방향도 괜찮을 것 같아요.
제가 규혁님 코드 기반으로 리팩토링 아이디어를 살짝 구현해봤는데요,
nayonsoso@46c3e30
- 도메인별로 fixture를 더 명확히 나누고
- create()에서는 연관 도메인의 중복 저장을 막고
- Fixture 클래스와 Builder 클래스를 분리해봤어요.
- (참고로 지금 단계에서는 패키지 구조는 신경 쓰지 않고 일단 다 fixture 하위에 넣었어요)
글로 설명하기보다, 어떤 식으로 바뀌면 좋을지 보여드리는 게 더 좋을 것 같아서 작업해봤습니다.
그래서 코드리뷰가 살짝 늦어진 점 이해해주세요🥹
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixtureBuilder.java (1)
43-52: 하드코딩된 값들에 대한 검토 필요create() 메소드에서 대부분의 필드가 하드코딩되어 있습니다. 현재 PR의 목표에 따라 "비중요 검증 데이터는 고정값으로 설정"한다는 원칙을 따른 것으로 보이나, 몇 가지 개선 사항을 제안합니다:
하드코딩된 값의 관리
- 상수로 분리하여 관리하는 것이 코드 가독성과 유지보수성을 높일 수 있음
- 중요한 검증 데이터와 비중요 데이터를 명확히 구분하는 주석 추가
테스트 시나리오 유연성
- 테스트 시나리오에 따라 변경이 필요한 필드는 빌더 메소드로 제공하는 것이 좋음
빌더 메소드와 상수를 추가하여 코드 가독성과 유연성을 개선할 수 있습니다:
+ // 비중요 검증 데이터를 위한 상수 + private static final String DEFAULT_DETAILS_FOR_LANGUAGE = "detailsForLanguage"; + private static final String DEFAULT_GPA_REQUIREMENT = "gpaRequirement"; + // ... 나머지 상수들 public UniversityInfoForApply create() { UniversityInfoForApply universityInfoForApply = new UniversityInfoForApply( null, term, koreanName, 1, HOME_UNIVERSITY_PAYMENT, ONE_SEMESTER, - "1", "detailsForLanguage", "gpaRequirement", - "gpaRequirementCriteria", "detailsForApply", "detailsForMajor", - "detailsForAccommodation", "detailsForEnglishCourse", "details", + "1", DEFAULT_DETAILS_FOR_LANGUAGE, DEFAULT_GPA_REQUIREMENT, + DEFAULT_GPA_REQUIREMENT_CRITERIA, DEFAULT_DETAILS_FOR_APPLY, DEFAULT_DETAILS_FOR_MAJOR, + DEFAULT_DETAILS_FOR_ACCOMMODATION, DEFAULT_DETAILS_FOR_ENGLISH_COURSE, DEFAULT_DETAILS, new HashSet<>(), university ); return universityInfoForApplyRepository.save(universityInfoForApply); }src/test/java/com/example/solidconnection/support/fixture/UniversityFixtureBuilder.java (1)
45-57: 하드코딩된 URL 값에 대한 개선 제안create() 메소드에서 대부분의 URL이 하드코딩되어 있습니다. 이러한 접근 방식은 PR에서 언급한 "비중요 검증 데이터는 고정값으로 설정"하는 원칙에 따른 것으로 보이나, 다음과 같은 개선을 고려해 보세요:
URL 문자열 관리
- 상수로 분리하여 코드 가독성과 유지보수성 향상
- 의미 있는 이름을 가진 상수로 관리하면 테스트 코드 이해도 증가
일관성 있는 패턴
- CountryFixtureBuilder와 RegionFixtureBuilder는 findOrCreate() 패턴을 사용하지만,
이 클래스는 create()만 제공합니다. 대학 엔티티의 특성상 재사용이 필요하지 않은지 검토 필요URL 상수화 및 코드 구조 개선:
+ // 기본 URL 상수 + private static final String DEFAULT_HOMEPAGE_URL = "https://homepage-url"; + private static final String DEFAULT_ENGLISH_COURSE_URL = "https://english-course-url"; + private static final String DEFAULT_ACCOMMODATION_URL = "https://accommodation-url"; + private static final String DEFAULT_LOGO_IMAGE_URL = "https://logo-image-url"; + private static final String DEFAULT_BACKGROUND_IMAGE_URL = "https://background-image-url"; public University create() { University university = new University( null, koreanName, englishName, "formatName", - "https://homepage-url", - "https://english-course-url", - "https://accommodation-url", - "https://logo-image-url", - "https://background-image-url", + DEFAULT_HOMEPAGE_URL, + DEFAULT_ENGLISH_COURSE_URL, + DEFAULT_ACCOMMODATION_URL, + DEFAULT_LOGO_IMAGE_URL, + DEFAULT_BACKGROUND_IMAGE_URL, null, country, region ); return universityRepository.save(university); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/test/java/com/example/solidconnection/support/TestContainerSpringBootTest.java(2 hunks)src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/CountryFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/CountryRepositoryForTest.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/LanguageRequirementFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/RegionFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/RegionRepositoryForTest.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixture.java(1 hunks)src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java(7 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/test/java/com/example/solidconnection/support/fixture/CountryRepositoryForTest.java
- src/test/java/com/example/solidconnection/support/fixture/RegionRepositoryForTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java
- src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java
- src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java
- src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java
- src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixture.java
- src/test/java/com/example/solidconnection/support/fixture/LanguageRequirementFixture.java
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/test/java/com/example/solidconnection/support/fixture/CountryFixtureBuilder.java (3)
src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
TestComponent(7-19)src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
TestComponent(7-21)src/test/java/com/example/solidconnection/support/fixture/RegionFixtureBuilder.java (1)
TestComponent(7-34)
src/test/java/com/example/solidconnection/support/fixture/UniversityFixtureBuilder.java (5)
src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
TestComponent(7-19)src/test/java/com/example/solidconnection/support/fixture/UniversityFixture.java (1)
TestComponent(7-23)src/test/java/com/example/solidconnection/support/fixture/CountryFixture.java (1)
TestComponent(7-21)src/test/java/com/example/solidconnection/support/fixture/CountryFixtureBuilder.java (1)
TestComponent(8-41)src/test/java/com/example/solidconnection/support/fixture/RegionFixtureBuilder.java (1)
TestComponent(7-34)
src/test/java/com/example/solidconnection/support/fixture/RegionFixtureBuilder.java (2)
src/test/java/com/example/solidconnection/support/fixture/RegionFixture.java (1)
TestComponent(7-19)src/test/java/com/example/solidconnection/support/fixture/CountryFixtureBuilder.java (1)
TestComponent(8-41)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
src/test/java/com/example/solidconnection/support/fixture/CountryFixtureBuilder.java (1)
1-41: 훌륭한 빌더 패턴 구현코드가 매우 깔끔하고 빌더 패턴을 잘 구현했습니다. 메소드 체이닝 접근 방식을 통해 테스트 데이터 생성을 유연하게 만들어 주었네요. 몇 가지 좋은 점을 강조하고 싶습니다:
findOrCreate() 메소드
- 동일한 코드로 국가를 재사용하여 테스트 데이터 중복을 방지함
- 데이터베이스 부하를 줄이는 데 도움이 됨
메소드 체이닝 접근 방식
- 가독성이 좋고 직관적인 API 제공
- 테스트 코드에서 쉽게 사용할 수 있음
의존성 주입
- Lombok의 @requiredargsconstructor를 사용하여 깔끔하게 처리됨
src/test/java/com/example/solidconnection/support/fixture/RegionFixtureBuilder.java (1)
1-34: 잘 구현된 지역 픽스처 빌더CountryFixtureBuilder와 일관된 접근 방식을 유지하여 전체적인 코드 일관성을 잘 유지했습니다. 좋은 점들을 다음과 같이 정리할 수 있습니다:
일관된 API 설계
- CountryFixtureBuilder와 동일한 패턴을 따라 일관성 있는 개발자 경험 제공
- findOrCreate() 메소드를 통한 효율적인 데이터 관리
간결한 구현
- 필요한 필드만 포함하여 간결함 유지
- 테스트에 필요한 기능만 정확히 구현
메소드 체이닝 접근 방식이 PR 목표에 잘 부합되는 것으로 보입니다. 코드가 깔끔하고 목적에 맞게 잘 설계되었습니다.
src/test/java/com/example/solidconnection/support/fixture/UniversityInfoForApplyFixtureBuilder.java (1)
1-42: 잘 구현된 대학 지원 정보 픽스처 빌더빌더 패턴을 일관되게 구현하여 테스트 코드의 가독성과 유지보수성을 높였습니다. 좋은 점들은 다음과 같습니다:
일관된 구조
- 다른 빌더 클래스와 동일한 패턴을 유지하여 일관성 있는 개발자 경험 제공
- 직관적인 메소드 이름과 체이닝 패턴
적절한 의존성 주입
- UniversityInfoForApplyRepository를 생성자를 통해 주입받음
- Spring 테스트 컨텍스트와 잘 통합됨
다만, 다른 빌더들과는 달리
findOrCreate()대신create()를 사용하고 있습니다. 이는 의도적인 선택인지, 아니면 일관성을 위해 통일해야 할 부분인지 확인해 보세요.src/test/java/com/example/solidconnection/support/fixture/UniversityFixtureBuilder.java (1)
1-44: 잘 구현된 대학 픽스처 빌더빌더 패턴을 통해 대학 엔티티 생성을 위한 유연한 인터페이스를 제공하고 있습니다. 다음과 같은 좋은 점이 있습니다:
명확한 구조
- 필수 속성들을 위한 빌더 메소드 제공
- 다른 빌더 클래스와 일관된 접근 방식 유지
의존성 관리
- Country와 Region 객체를 명시적으로 주입받도록 설계
- 다른 픽스처와의 조합을 통해 테스트 데이터 구성 가능
PR에서 언급된 "메소드 체이닝 접근법으로 fixture 데이터를 구성"한다는 목표에 잘 부합되는 구현입니다. 대학 생성을 개별적으로 처리하는 접근 방식도 PR 목표에 맞게 잘 구현되었습니다.
src/test/java/com/example/solidconnection/support/TestContainerSpringBootTest.java (2)
6-6: 새로운 ComponentScan import 추가에 대한 검토
변경 내용
- 테스트 픽스처 컴포넌트를 스캔하기 위한 ComponentScan 어노테이션 import 구문이 추가되었습니다.
- 이는 PR의 주요 목표인 "대학 관련 통합 테스트 데이터 정의 구조 개선"을 위한 중요한 첫 단계입니다.
평가
- 적절한 import 위치에 깔끔하게 추가되었습니다. 👍
- 스프링 프레임워크 표준을 잘 따르고 있습니다.
15-15: ComponentScan 어노테이션 추가에 대한 검토
변경 내용
- TestContainerSpringBootTest 커스텀 어노테이션에 ComponentScan 어노테이션이 추가되었습니다.
- 이제 "com.example.solidconnection.support.fixture" 패키지의 모든 컴포넌트가 테스트 컨텍스트에 자동으로 등록됩니다.
기대 효과
- 새로 추가된 픽스처 클래스(CountryFixture, RegionFixture, UniversityFixture 등)를 테스트에서 쉽게 사용할 수 있습니다.
- 테스트 데이터 설정 코드가 간결해지고, 재사용성이 향상됩니다.
- 메서드 체이닝 방식의 픽스처 패턴을 지원하는 기반이 마련됩니다.
평가
- 어노테이션 스택의 최상단에 배치된 것은 적절합니다. 👍
- 테스트 픽스처 컴포넌트를 명확하게 분리하여 관리하는 좋은 설계 패턴입니다.
- PR 목표에 매우 적합한 변경입니다.
|
너무 잘이해되는 코드네요.. 고생하셨습니다!
추가로
이 의견들 모두 좋은 거 같습니다! |
There was a problem hiding this comment.
1
그럼 대학 관련 테스트 시 매번 @beforeeach에서 모든 데이터를 생성해줘야하나인데 이건 지난번 얘기한 것처럼 어쩔 수 없는거겠죠..?
“모든 데이터를 생성해줘야하나”가
- 대학 지원 정보 테스트를 위해, 국가, 지역, 대학교를 다 세팅해야한다는 뜻인가요?
- 모든 대학교를 생성해야한다는 뜻인가요?
전자는 어쩔 수 없습니다😓
FK 로 되어있기 때문에, DB에 저장되지 않은 국가나 지역을 참조하면 외래키 제약조건으로 예외가 발생할테니깐요.
후자는 필요한 데이터만 given 절에서 세팅하면 된다 생각합니다.
이전에 BaseIntegrationTest에서도, 테스트의 목적과 관계 없는 데이터까지
모든 데이터를 한번에 세팅하려다보니 문제가 생겼었잖아요?
그런 상황을 방지하고자 이번 래팩터링을 한 것도 있으니까
한번에 전체를 다 설정하기보다, 필요한 데이터만 세팅하는게 테스트의 목적을 더 잘 보여준다고 생각해요!
🔽 이렇게요
@Test
void 지역으로_대학을_필터링한다() { // 모든 대학이 아니라, 지역이 '영미권인 대학'과 '영미권이 아닌 대학' 두개만 세팅하여, 영미권인 대학만 필터링되는지 확인
// given
UniversityForApply 괌대학_A_지원_정보 = 괌대학_A_지원_정보();
UniversityForApply 인하대_지원_정보 = 인하대_지원_정보();
// when
UniversityInfoForApplyPreviewResponses response = universityQueryService.searchUniversity("AMERICAS", List.of(), null, null);
// then
assertThat(response.universityInfoForApplyPreviewResponses()).contains(UniversityInfoForApplyPreviewResponse.from(괌대학_A_지원_정보));
assertThat(response.universityInfoForApplyPreviewResponses()).doesNotContains(UniversityInfoForApplyPreviewResponse.from(인하대_지원_정보));
There was a problem hiding this comment.
2
그냥 도메인별로 나눠서 region 안에 RegionFixture, RegionFixtureBuilder, RegionRepositoryForTest를 넣는 게 좋을까요?
네 그게 좋겠습니다 👍
그리고 fixture.region, fixture.country 같은 식으로 구성하기보다는,
region.fixture, country.fixture처럼 도메인 패키지 하위에 fixture를 두는 방식은 어떨까요?
각 도메인의 하위에 테스트용 구성 요소를 위치시키는 편이 도메인 중심 구조와 더 잘 어울린다 생각해요.
또한 이렇게 하면 테스트 코드에, 프로덕션 코드에는 없는 region과 country라는 패키지가 생기게 되는데요,
이에 맞춰 프로덕션 코드에도 동일한 도메인 단위(region, country) 패키지를 만들면 좋겠습니다 ㅎㅎ
지금은 프로덕션 코드의 entity 패키지에 Country, Region 클래스가 있고,
repository 패키지에 CountryRepository, InterestedCountryRepository, InterestedRegionRepository가 흩어져 있습니다 🤧
다른 도메인 중심 패키지와 비교했을 때 일관성이 부족한 상태예요.
제가 제안드린 방법이 괜찮으시면, 우선 이 PR에서 region.fixture, country.fixture 패키지를 구성하고,
이후에 프로덕션 코드 패키지를 별도 PR로 정리해보면 어떨까요?
3
사실 이번에 생긴 클래스 모두 테스트를 위한 컴포넌트들이란 생각이 들어서 그냥 TestComponent로 통일했는데 괜찮나요?
네 좋습니다 😊
4
JpaRepository를 상속받는 인터페이스는 이미 @repository의 기능이 있는 거로 아는데 달아주셨더라구요. 그래서 기존 main 쪽 JpaRepository도 확인해보니 모두 @repository가 붙어있더라구요. 이건 가독성을 위해 붙여주신건가요?
크흠.. 제가 @Repository 안붙여도 된다는걸 까먹어서 그랬던 것 같습니다;;
없애도록 리팩터링해도 괜찮습니다^^
1
동의합니다! 2
이해하기는 이게 제일 좋을 거 같긴 한데
#263 저희가 여기 논의에서 전체 테스트 코드(=fixture)는 하나의 패키지에서 관리하기로 했던 걸 그럼 포기하고 도메인 패키지 하위에 fixture 방식으로 가는 거로 변경하자는 말씀이신건가요? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/java/com/example/solidconnection/region/fixture/RegionFixtureBuilder.java (1)
1-35: RegionFixtureBuilder 클래스가 적절히 구현되었습니다.
빌더 패턴 구현:
- 메소드 체이닝을 통한 유연한 Region 객체 생성이 가능합니다.
code와koreanName속성을 설정하는 메소드가 잘 구현되었습니다.
findOrCreate()메소드 구현:
- 기존 Region이 있으면 재사용하고, 없으면 새로 생성하는 로직이 잘 구현되었습니다.
- 람다 표현식을 활용해 간결하게 코드가 작성되었습니다.
region()메소드 구현 개선 가능:
region()메소드에서 새 인스턴스를 생성하는 방식은 불필요한 객체 생성을 유발할 수 있습니다.- public RegionFixtureBuilder region() { - return new RegionFixtureBuilder(regionRepositoryForTest); - } + public RegionFixtureBuilder region() { + this.code = null; + this.koreanName = null; + return this; + }위 수정은 새 인스턴스를 생성하는 대신 현재 인스턴스를 재사용하고 상태를 초기화하는 방식으로 변경합니다. 이렇게 하면 불필요한 객체 생성을 줄일 수 있습니다.
src/test/java/com/example/solidconnection/country/fixture/CountryFixtureBuilder.java (1)
1-42: CountryFixtureBuilder 구현이 잘 되었지만 개선할 여지가 있습니다.빌더 패턴을 사용한 국가 엔티티 생성 방식이 전반적으로 잘 구현되었습니다. 다음과 같은 특징과 개선점이 있습니다:
장점
- 유연한 메서드 체이닝 구현으로 가독성 높은 코드 작성이 가능합니다.
findOrCreate()메서드를 통해 중복 데이터 생성을 방지합니다.- 필드 설정을 위한 메서드가 명확하게 구분되어 있습니다.
개선 가능한 부분
country()메서드가 매번 새 인스턴스를 생성하므로 메모리 효율성이 떨어질 수 있습니다.- 필수 필드에 대한 유효성 검사가 없어 불완전한 객체가 생성될 가능성이 있습니다.
아래와 같이 개선할 수 있습니다:
public CountryFixtureBuilder country() { - return new CountryFixtureBuilder(countryRepositoryForTest); + this.code = null; + this.koreanName = null; + this.region = null; + return this; } public Country findOrCreate() { + if (code == null || koreanName == null || region == null) { + throw new IllegalStateException("Country requires code, koreanName, and region to be set"); + } return countryRepositoryForTest.findByCode(code) .orElseGet(() -> countryRepositoryForTest.save(new Country(code, koreanName, region))); }src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureBuilder.java (1)
45-57: 기존 엔티티 재사용 로직을 추가할 수 있습니다.
create()메소드는 항상 새로운 University 객체를 생성하고 있습니다. 다른 관련 빌더(RegionFixtureBuilder, CountryFixtureBuilder)에서는findOrCreate()패턴을 사용하여 기존 엔티티를 재사용하고 있습니다.다음과 같이 findOrCreate() 메소드를 추가하여 대학 엔티티도 재사용 가능하게 할 수 있습니다:
- public University create() { + public University findOrCreate() { + return universityRepository.findByKoreanName(koreanName) + .orElseGet(this::create); + } + + private University create() { University university = new University( null, koreanName, englishName, "formatName", "https://homepage-url", "https://english-course-url", "https://accommodation-url", "https://logo-image-url", "https://background-image-url", null, country, region ); return universityRepository.save(university); }이 방식은 테스트 실행 시간을 단축하고 데이터베이스 부하를 줄일 수 있습니다.
src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixture.java (1)
14-26: languageRequirementFixtureBuilder 인스턴스를 재활용할 수 있습니다.현재 코드에서는 각 Builder 호출마다 새 인스턴스를 사용하는 것처럼 보입니다.
다음과 같이 Builder를 재활용하여 약간 더 간결하게 작성할 수 있습니다:
public UniversityInfoForApply 괌대학_A_언어요구사항(UniversityInfoForApply universityInfo) { - languageRequirementFixtureBuilder - .languageTestType(LanguageTestType.TOEFL_IBT) - .minScore("80") - .universityInfoForApply(universityInfo) - .create(); - languageRequirementFixtureBuilder - .languageTestType(LanguageTestType.TOEIC) - .minScore("800") - .universityInfoForApply(universityInfo) - .create(); + LanguageRequirementFixtureBuilder builder = languageRequirementFixtureBuilder.languageRequirement(); + + builder.languageTestType(LanguageTestType.TOEFL_IBT) + .minScore("80") + .universityInfoForApply(universityInfo) + .create(); + + builder.languageTestType(LanguageTestType.TOEIC) + .minScore("800") + .universityInfoForApply(universityInfo) + .create(); return universityInfo; }다만 이 변경은 LanguageRequirementFixtureBuilder 클래스에 languageRequirement() 메소드가 구현되어 있다고 가정합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/test/java/com/example/solidconnection/country/fixture/CountryFixture.java(1 hunks)src/test/java/com/example/solidconnection/country/fixture/CountryFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/country/repository/CountryRepositoryForTest.java(1 hunks)src/test/java/com/example/solidconnection/region/fixture/RegionFixture.java(1 hunks)src/test/java/com/example/solidconnection/region/fixture/RegionFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/region/repository/RegionRepositoryForTest.java(1 hunks)src/test/java/com/example/solidconnection/support/TestContainerSpringBootTest.java(2 hunks)src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixture.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityFixture.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityInfoForApplyFixture.java(1 hunks)src/test/java/com/example/solidconnection/university/fixture/UniversityInfoForApplyFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/example/solidconnection/support/TestContainerSpringBootTest.java
- src/test/java/com/example/solidconnection/university/service/UniversityQueryServiceTest.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureBuilder.java (5)
src/test/java/com/example/solidconnection/university/fixture/UniversityFixture.java (1)
TestComponent(9-97)src/test/java/com/example/solidconnection/region/fixture/RegionFixture.java (1)
TestComponent(7-33)src/test/java/com/example/solidconnection/country/fixture/CountryFixture.java (1)
TestComponent(8-54)src/test/java/com/example/solidconnection/country/fixture/CountryFixtureBuilder.java (1)
TestComponent(9-42)src/test/java/com/example/solidconnection/region/fixture/RegionFixtureBuilder.java (1)
TestComponent(8-35)
src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixtureBuilder.java (1)
src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixture.java (1)
TestComponent(8-113)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
src/test/java/com/example/solidconnection/region/repository/RegionRepositoryForTest.java (1)
1-11: RegionRepositoryForTest 인터페이스가 잘 구현되었습니다.
- 테스트를 위한 Repository 인터페이스 구현:
- 테스트에서 Region 엔티티를 코드로 조회할 수 있는 기능이 추가되었습니다.
- 인터페이스가 간결하고 목적에 맞게 설계되었습니다.
이 인터페이스는 테스트 데이터 설정 시 기존 Region을 재사용할 수 있게 해주어 테스트의 일관성을 유지하는 데 도움이 됩니다.
src/test/java/com/example/solidconnection/country/repository/CountryRepositoryForTest.java (1)
1-11: CountryRepositoryForTest 인터페이스가 적절히 구현되었습니다.
- 테스트를 위한 Country 조회 기능:
- 코드를 통한 Country 조회 기능이 잘 정의되었습니다.
- JpaRepository 확장으로 기본 CRUD 연산도 사용 가능합니다.
이 인터페이스는 RegionRepositoryForTest와 동일한 패턴을 따르고 있어 일관성 있는 테스트 코드 작성이 가능합니다.
src/test/java/com/example/solidconnection/region/fixture/RegionFixture.java (1)
1-33: RegionFixture 클래스가 잘 설계되었습니다.
테스트 컴포넌트 구성:
@TestComponent어노테이션을 통해 테스트 환경에서만 사용되는 컴포넌트임을 명확히 했습니다.- Lombok의
@RequiredArgsConstructor를 사용해 의존성 주입이 깔끔하게 처리되었습니다.지역 데이터 제공 메소드:
- 영미권, 유럽, 아시아의 세 가지 지역을 간결한 메소드로 제공합니다.
- 각 메소드는 메소드 체이닝 패턴을 사용해 가독성이 좋습니다.
findOrCreate()메소드를 통해 데이터 중복 생성을 방지합니다.이 클래스는 PR 목표에 맞게 테스트 데이터 픽스처 구조를 개선하고 메소드 체이닝 접근 방식을 잘 구현했습니다.
src/test/java/com/example/solidconnection/country/fixture/CountryFixture.java (1)
1-54: 잘 구현된 CountryFixture 클래스입니다!이 클래스는 테스트용 국가 데이터를 효율적으로 생성하고 관리하는 좋은 예시입니다. 몇 가지 장점을 아래와 같이 정리해 보았습니다:
구조적 장점
@TestComponent와@RequiredArgsConstructor를 적절히 활용하여 테스트 컴포넌트 의존성 주입을 간결하게 처리했습니다.- 각 국가별 메서드가 명확하게 분리되어 있어 가독성이 좋습니다.
기능적 장점
findOrCreate()메서드를 통해 중복 데이터 생성을 방지하여 테스트 실행 효율성을 높였습니다.- 국가와 지역 간의 관계가 명확하게 표현되어 있습니다.
- 빌더 패턴을 사용하여 객체 생성 코드가 가독성이 높습니다.
명명 규칙
- 한글 메서드명(
미국(),캐나다()등)을 사용하여 도메인 컨텍스트를 명확히 표현했습니다.전반적으로 테스트 픽스처 패턴을 잘 구현한 클래스입니다. 👍
src/test/java/com/example/solidconnection/university/fixture/UniversityInfoForApplyFixture.java (3)
1-17: 클래스 구조와 의존성 주입이 잘 설계되었습니다.코드가 다음과 같은 점에서 잘 구현되었습니다:
@TestComponent어노테이션을 사용하여 테스트 컨텍스트에서만 빈으로 등록- Lombok의
@RequiredArgsConstructor를 활용한 간결한 의존성 주입@Value어노테이션으로 외부 설정 값을 주입받아 사용이렇게 구성된 클래스는 테스트 코드의 재사용성과 유지보수성을 높여줍니다.
18-24: 메소드 네이밍과 구현 패턴이 일관성 있게 적용되었습니다.한국어 대학명을 메소드 이름으로 사용하여 직관적인 API를 제공하고 있습니다. 메소드 체이닝 패턴이 잘 적용되어 가독성 높은 코드를 작성하였습니다.
26-32: 반복되는 패턴의 일관성이 유지되고 있습니다.모든 대학 정보 생성 메소드가 동일한 패턴을 따르고 있어 코드의 일관성과 예측 가능성이 뛰어납니다:
- universityInfoForApplyFixtureBuilder 호출
- term 설정 (주입받은 값 사용)
- koreanName 설정 (대학별 특화된 이름)
- university 설정 (UniversityFixture에서 제공하는 대학 객체)
- create() 호출로 생성 및 저장
이런 일관된 구조는 테스트 코드의 이해도와 유지보수성을 크게 향상시킵니다.
Also applies to: 34-40, 42-48, 50-56, 58-64, 66-72, 74-80, 82-88, 90-96
src/test/java/com/example/solidconnection/university/fixture/UniversityFixtureBuilder.java (2)
1-20: 클래스 구조와 필드 선언이 명확합니다.클래스 구조와 필드 선언에 대한 다음 사항들이 잘 구현되었습니다:
@TestComponent어노테이션으로 테스트 환경에서만 사용되는 컴포넌트임을 명시@RequiredArgsConstructor를 통한 간결한 의존성 주입- 필요한 필드들(koreanName, englishName, country, region)을 명확하게 선언
빌더 패턴 구현을 위한 기본 구조가 잘 갖춰져 있습니다.
21-43: 빌더 패턴이 효과적으로 구현되었습니다.메소드 체이닝을 위한 빌더 패턴이 다음과 같이 적절하게 구현되었습니다:
university()메소드를 통해 새 인스턴스 생성- 각 속성에 대한 setter 메소드가 this를 반환하여 체이닝 가능
- 메소드 이름이 명확하고 일관성 있게 지정됨
이 구현을 통해 테스트 코드에서 대학 객체를 직관적으로 생성할 수 있습니다.
src/test/java/com/example/solidconnection/university/fixture/UniversityInfoForApplyFixtureBuilder.java (3)
1-23: 클래스 구조와 import 선언이 명확합니다.다음 사항들이 잘 구현되었습니다:
- 필요한 import 문들이 정리되어 있음
- 정적 import를 통해 열거형 상수를 간결하게 사용
@TestComponent와@RequiredArgsConstructor사용으로 테스트 컴포넌트와 의존성 주입 명확히 함- 필요한 필드들(term, koreanName, university)을 적절히 선언
클래스의 전반적인 구조가 다른 빌더 클래스들과 일관성 있게 유지되고 있습니다.
24-41: 빌더 패턴 메소드가 잘 구현되었습니다.빌더 패턴 구현이 다음과 같이 효과적으로 되어 있습니다:
universityInfoForApply()메소드로 새 빌더 인스턴스 반환- 각 필드에 대한 setter 메소드가 this를 반환하여 메소드 체이닝 가능
- 메소드 네이밍이 명확하고 일관됨
이 패턴은 테스트 코드의 가독성과 유지보수성을 높이는 데 기여합니다.
43-52: default 값 설정이 효과적으로 구현되었습니다.
create()메소드에서 다음 사항들이 잘 처리되었습니다:
- 테스트에 필요한 기본값들이 하드코딩되어 있어 테스트 데이터 생성이 간결함
- 중요한 필드는 파라미터로 받아 설정하고, 덜 중요한 필드는 고정값으로 처리
- HashSet 초기화로 컬렉션 필드도 적절히 처리
이런 방식은 테스트 데이터 셋업 코드를 간소화하는 데 효과적입니다.
src/test/java/com/example/solidconnection/university/fixture/LanguageRequirementFixture.java (2)
1-13: 클래스 구조가 깔끔하게 설계되었습니다.다음 사항들이 잘 구현되었습니다:
- 필요한 import들이 명확하게 선언됨
@TestComponent와@RequiredArgsConstructor어노테이션 사용이 적절함- LanguageRequirementFixtureBuilder를 주입받아 활용
전체적인 클래스 구조가 다른 Fixture 클래스들과 일관성 있게 유지되고 있습니다.
14-26: 언어 요구사항 메소드 패턴이 일관되고 유연합니다.모든 언어 요구사항 설정 메소드가 다음과 같은 일관된 패턴으로 구현되었습니다:
- 대학별로 특화된 메소드 이름 사용
- UniversityInfoForApply 객체를 파라미터로 받아 확장
- 해당 대학에 적합한 LanguageTestType과 minScore 값 설정
- 마지막에 입력받은 universityInfo 객체를 반환하여 메소드 체이닝 지원
이 방식은 다음과 같은 장점이 있습니다:
- 언어 요구사항을 대학별로 명확하게 분리
- 테스트 코드에서 메소드 체이닝으로 간결하게 사용 가능
- 각 대학별 특성에 맞는 언어 요구사항 설정 가능
전체적으로 테스트 코드의 가독성과 유지보수성을 높이는 좋은 패턴입니다.
Also applies to: 28-40, 42-49, 51-58, 60-67, 69-76, 78-85, 87-94, 96-103, 105-112
...t/java/com/example/solidconnection/university/fixture/LanguageRequirementFixtureBuilder.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/university/fixture/UniversityFixture.java
Show resolved
Hide resolved
There was a problem hiding this comment.
제가 코멘트에서 언급한 "괌대학(B형)" 코멘트와 "토플_80" 코멘트에 대해서만 의견 나누고,
대학 픽스쳐는 이만 넘어가도 좋을 것 같습니다 🤗
저와 규혁님이 열심히 고민한 끝에 나온, 처음 목표한 요구사항은 그래도 다 이루었네요 ㅎㅎ
그래도 100% 만족스럽지는 않은 부분들이 있긴 한데요!
지금은 이만 SiteUserFixture로 넘어가고, 이건 나중에 고민할 부분으로 두기 위해 이렇게 적어봐요.
UniversityInfoForApply 괌대학_A_지원_정보 = universityInfoForApplyFixture.괌대학_A_지원_정보();라는 코드가 너무 길다. 여기에서 universityInfoForApplyFixture 부분만 생략되더라도 괜찮을텐데...- 어학 요구사항과 대학을 연관짓는 부분의 코드가 다소 직관적이지 않다.
| @Value("${university.term}") | ||
| public String term; |
| public UniversityInfoForApply 괌대학_B_지원_정보() { | ||
| return universityInfoForApplyFixtureBuilder.universityInfoForApply() | ||
| .term(term) | ||
| .koreanName("괌대학 B 지원 정보") | ||
| .university(universityFixture.괌_대학()) | ||
| .create(); | ||
| } |
There was a problem hiding this comment.
제가 스타트를 잘 못 끊은 것 같긴 한데요.. 😅😅😅
대학 지원 정보의 koreanName은 괌대학 B 지원 정보가 아니라 괌대학(B형) 형식으로 되어있습니다!
이 테스트 픽스쳐들은 아래 가정에서 나오게 되었는데요,
- 우리 서비스 개발자들은 '괌대학' 이라는 도메인 지식을 당연하게도 알고있다.
- 따라서 '괌대학'이 가진 값이 지정된 픽스쳐를 만들면,
- 테스트 데이터 세팅과 테스트 코드 이해 시간이 줄어든다.
이 중에서 "'괌대학' 이라는 도메인 지식을 당연하게도 알고있다"를 더 생각해본다면,
실제 데이터와 다른 값이 들어있다는 것은 예상하지 못할 것 같다는 생각이 듭니다!
이런 픽스쳐의 특성을 고려하여, 실제와 100% 일치하는 데이터를 세팅하는게 어떨까 싶습니다.
| public UniversityInfoForApply 괌대학_B_지원_정보() { | |
| return universityInfoForApplyFixtureBuilder.universityInfoForApply() | |
| .term(term) | |
| .koreanName("괌대학 B 지원 정보") | |
| .university(universityFixture.괌_대학()) | |
| .create(); | |
| } | |
| public UniversityInfoForApply 괌대학_B_지원_정보() { | |
| return universityInfoForApplyFixtureBuilder.universityInfoForApply() | |
| .term(term) | |
| .koreanName("괌대학(B형)") | |
| .university(universityFixture.괌_대학()) | |
| .create(); | |
| } |
| public UniversityInfoForApply 괌대학_B_언어요구사항(UniversityInfoForApply universityInfo) { | ||
| languageRequirementFixtureBuilder | ||
| .languageTestType(LanguageTestType.TOEFL_IBT) | ||
| .minScore("70") | ||
| .universityInfoForApply(universityInfo) | ||
| .create(); | ||
| languageRequirementFixtureBuilder | ||
| .languageTestType(LanguageTestType.TOEIC) | ||
| .minScore("900") | ||
| .universityInfoForApply(universityInfo) | ||
| .create(); | ||
| return universityInfo; | ||
| } |
There was a problem hiding this comment.
언어 성적 요구사항은 "xx대학_언어요구사항" 보다는 "토플_80" 같은 픽스쳐를 만들면 어떨까요?
앞의 '구체적인 도메인을 지정했으니, 실제와 일치하는 값을 넣자'는 코멘트와 모순된다 느끼실 수도 있는데, 제가 생각하는 본질은 동일해요.
바로, '이 픽스쳐를 봤을 때 다른 개발자들도 그 안의 값을 쉽게 예상할 수 있는가?' 입니다.
괌대학_B형 같은 경우는, 우리 서비스 개발자들이라면
미국에 있고, 영미권에, 괌대학이라는 대학의, B형 지원 정보라는 것을 한번에 떠올릴 수 있습니다.
하지만 "괌대학 B형의 언어 시험 요구사항"은 쉽게 떠올리지 못합니다.
이를 토플_80() 과 같이 바꾼다면,
- 개발자들이 한번에 이해할 수 있고
- 중복되는 코드를 없애줍니다
이런식으로 메서드를 만들어야할까요... 우선 말씀해주신 것들은 반영했습니다! |
2 participants
관련 이슈
작업 내용
#263 의 논의 내용들을 구현 중에 있습니다.
특이 사항
내용들을 구현하다보니 조금 헷갈린 부분이나 고민사항들이 생겼습니다 ㅎㅎ..
리뷰 요구사항 (선택)
현재 fixture를 메서드 체이닝 방식으로 구성했는데, 제가 이해한 방향이 맞는지 궁금합니다.
만약 체이닝 방식이 의도한 것과 다르다면 지금 수정하는 것이 맞을 것 같아 확인 부탁드립니다.
대학 관련 정보 데이터를 하나 생성해보니까,
하나만 만들어도 세팅해줘야 하는 데이터가 생각보다 엄청 많더라구요.
그래서 말씀해주셨던
이걸 적용하려고 했는데, 지역/국가/대학 같은 걸 추가하는 API가 없으니 대학 관련 데이터는 수정이 거의 일어나지 않을 것 같고, 추가적으로 다른 도메인에서도 자주 사용하게 될 것 같은데, 이런 경우에도 대학 도메인 패키지 하위에 fixture를 만드는 게 맞나? 하는 생각이 들었어요.
어떤 식으로 대학 관련 데이터를 만들어두는 게 좋을까요?
마지막은 조금 사소한 포인트긴 한데, 메서드 체이닝 방식으로 대학 관련 데이터를 생성할 때
지역 -> 국가 -> 대학 -> 대학정보 -> 언어 관련
이런 순서로 생성하도록 되어있다 보니까, 이전 단계 값이 없으면 예외를 던지게 일단 처리를 해두긴 했어요.
근데 고민됐던 게, 저희 서비스에는 애초에 대학 관련 생성 API가 없다 보니까
관련 사용자정의 예외가 없는 상황이더라구요.
그래서 테스트만을 위해 새로운 예외를 정의하는 게 맞나? 싶은 고민이 들었고,
일단은 기존에 있던 ErrorResponse를 사용해서 처리해둔 상태입니다.
이거 외에도 혹시 의견 주시면 감사하겠습니다!
변경된 사항 (회의 이후 반영)
각 픽처스는 도메인별로 나누기
-> 여기서 제 의견이 조금 추가된 건 제가 기존에 구현했던 방식이 공유변수 관련해서도 계속 늘 문제였던 거 같아서 Provider를 써야하나 고민을 하다가 그냥 메소드 체이닝 방식 도입 시 create에서 그냥 그 객체를 반환해버리면 되는 거였더라구요.. 그래서 그렇게 일단 한 번 적용해봤습니다.
Fixture 생성 시 중요한 검증 데이터가 아니라면 그냥 고정값 넣어두기
UniversityFixtureHelper에서 모든 대학 한 번에 생성하는 것이 아니라 개별적으로 생성하기
-> 지난 회의 때 한 번에 생성하는 거로 시도하다가 실패를 했었던 게 마음이 아프네요 ㅎㅎ..
예외처리 없애기
추가적으로 UniversityData란 걸 추가해서 좀 편리하게 대학 관련 도메인들 확인할 수 있게 했는데 어떤가요? 이제 이런 방식으로 다른 대학들도 쭉 생성하면 문제가 없을 거 같은데 한 번 검토해주시면 감사하겠습니다!
생각해보니 국가나 지역같은 경우는 재사용이 될테니까
여기에 이미 존재하면 디비에서 찾아오는 걸로 수정을 해야겠네요..