Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough서비스 레이어 통합 테스트들을 컨트롤러(MockMvc) 레벨로 전환하고 테스트 인프라(TestGeminiConfig, TestPropertyConfig, ControllerTestSupport 등)를 도입·확장했습니다. 또한 GeminiClient.init()의 초기화 로직을 try-catch로 감싸 초기화 실패 시 예외 전파 대신 경고 로깅 처리하도록 변경했습니다. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
테스트 코드를 Service 레이어 중심의 통합 테스트에서 Controller(MockMvc) 기반 통합 테스트로 재구성하고, 테스트 환경에서 Gemini 연동 의존성을 분리/대체할 수 있도록 테스트 전용 설정을 추가한 PR입니다.
Changes:
TestGeminiConfig를 추가해 테스트에서GeminiClient를 Mock으로 대체- 다수의 서비스 통합 테스트를 삭제하고, 동일 시나리오를 Controller 통합 테스트(MockMvc)로 이전
- 테스트 프로퍼티/테스트 부트스트랩 구성을 보강하고,
GeminiClient초기화 로직을 예외 허용 형태로 조정
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/application-test.yml | 테스트 프로필에 Slack signing secret 및 Gemini 설정 추가 |
| src/test/java/gg/agit/konect/support/TestGeminiConfig.java | 테스트에서 사용할 GeminiClient Mock 빈 추가 |
| src/test/java/gg/agit/konect/support/IntegrationTestSupport.java | 테스트 컨텍스트 구성에 TestGeminiConfig 포함 |
| src/test/java/gg/agit/konect/support/ControllerTestSupport.java | Controller 통합 테스트 베이스 클래스 확장(MockMvc/DB 유틸/테스트 프로퍼티 등) |
| src/test/java/gg/agit/konect/domain/user/service/UserServiceIntegrationTest.java | Service 통합 테스트 삭제(Controller 테스트로 이전) |
| src/test/java/gg/agit/konect/domain/user/controller/UserControllerIntegrationTest.java | User 관련 Controller 통합 테스트 신규 추가 |
| src/test/java/gg/agit/konect/domain/studytime/service/StudyTimerServiceIntegrationTest.java | Service 통합 테스트 삭제(Controller 테스트로 이전) |
| src/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.java | StudyTime 관련 Controller 통합 테스트 신규 추가 |
| src/test/java/gg/agit/konect/domain/schedule/controller/ScheduleControllerIntegrationTest.java | Schedule 테스트를 Controller 통합 테스트로 전환 및 패키지/검증 방식 수정 |
| src/test/java/gg/agit/konect/domain/club/controller/ClubRecruitmentControllerIntegrationTest.java | ClubRecruitment 테스트를 Controller 통합 테스트로 전환 |
| src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java | ClubMember 테스트를 Controller 통합 테스트로 전환 및 시나리오 재구성 |
| src/test/java/gg/agit/konect/domain/club/controller/ClubBasicControllerIntegrationTest.java | Club 기본 API 테스트를 Controller 통합 테스트로 전환 및 케이스 보강 |
| src/test/java/gg/agit/konect/domain/club/controller/ClubApplicationControllerIntegrationTest.java | ClubApplication 테스트를 Controller 통합 테스트로 전환 |
| src/test/java/gg/agit/konect/KonectApplicationTests.java | 컨텍스트 로딩 테스트에 테스트 프로필/Import/TestPropertySource 추가 |
| src/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.java | Gemini 초기화 시 예외를 swallow하도록 변경(테스트 환경 고려) |
Comments suppressed due to low confidence (1)
src/test/java/gg/agit/konect/support/IntegrationTestSupport.java:22
- IntegrationTestSupport에서도 @TestPropertySource로 classpath:.env.test.properties를 지정하고 있는데, 리포지토리 내에 해당 파일이 없어 테스트 컨텍스트 로딩이 실패할 수 있습니다. 파일을 src/test/resources에 추가하거나 locations 지정을 제거하고 application-test.yml로만 테스트 설정을 유지해주세요.
@Import({TestSecurityConfig.class, TestJpaConfig.class, TestGeminiConfig.class})
@EnableAutoConfiguration(exclude = {
OAuth2ClientAutoConfiguration.class
})
@TestPropertySource(locations = "classpath:.env.test.properties", properties = {
"spring.config.import=",
"spring.datasource.driver-class-name=org.h2.Driver",
"spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE",
Comment on lines
101
to
103
| protected void mockLoginUser(Integer userId) throws Exception { | ||
| given(loginUserArgumentResolver.resolveArgument(any(), any(), any(), any())).willReturn(userId); | ||
| } |
There was a problem hiding this comment.
mockLoginUser()로 resolveArgument만 스텁하는 현재 구성은, setUpCommonMocks()에서 supportsParameter를 항상 true로 스텁하는 것과 결합되면 @userid가 없는 파라미터(예: @RequestBody DTO, @PathVariable 등)까지 이 ArgumentResolver가 처리하려고 시도해 컨트롤러 바인딩이 실패할 수 있습니다. supportsParameter는 실제 구현처럼 @UserId 애노테이션 + int/Integer 타입일 때만 true가 되도록 Answer로 스텁하거나, supportsParameter 스텁을 제거하는 방식으로 수정해 주세요.
Comment on lines
+48
to
+62
| @TestPropertySource(locations = "classpath:.env.test.properties", properties = { | ||
| "spring.config.import=", | ||
| "spring.datasource.driver-class-name=org.h2.Driver", | ||
| "spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DB_CLOSE_DELAY=-1;DB_CLOSE_ON_EXIT=FALSE", | ||
| "spring.datasource.username=sa", | ||
| "spring.datasource.password=", | ||
| "spring.jpa.hibernate.ddl-auto=create-drop", | ||
| "spring.jpa.properties.hibernate.dialect=org.hibernate.dialect.H2Dialect", | ||
| "spring.jpa.properties.javax.persistence.validation.mode=none", | ||
| "spring.flyway.enabled=false", | ||
| "spring.data.redis.host=localhost", | ||
| "spring.data.redis.port=6379", | ||
| "spring.security.enabled=false", | ||
| "logging.ignored-url-patterns=" | ||
| }) |
There was a problem hiding this comment.
@TestPropertySource에서 classpath:.env.test.properties를 지정하고 있지만, 리포지토리 내에 해당 파일이 존재하지 않아 테스트 컨텍스트 로딩이 실패할 수 있습니다. 파일을 src/test/resources에 추가하거나, locations 지정 자체를 제거하고 application-test.yml만으로 테스트 설정을 구성해주세요.
| @SpringBootTest | ||
| @ActiveProfiles("test") | ||
| @Import({TestGeminiConfig.class, TestSecurityConfig.class}) | ||
| @TestPropertySource(locations = "classpath:.env.test.properties") |
There was a problem hiding this comment.
@TestPropertySource(locations = "classpath:.env.test.properties")로 지정한 파일이 리포지토리에 없어서 테스트 실행 시 ApplicationContext 로딩이 실패할 수 있습니다. 실제로 필요한 테스트 전용 프로퍼티가 없다면 locations를 제거하고, 필요하다면 src/test/resources에 .env.test.properties를 추가해 주세요.
Suggested change
| @TestPropertySource(locations = "classpath:.env.test.properties") |
Comment on lines
+68
to
+81
| try { | ||
| this.vertexAI = new VertexAI( | ||
| geminiProperties.projectId(), | ||
| geminiProperties.location() | ||
| ); | ||
| this.generativeModel = new GenerativeModel(geminiProperties.model(), vertexAI); | ||
| log.info("GeminiClient 초기화 완료: project={}, location={}, model={}", | ||
| geminiProperties.projectId(), | ||
| geminiProperties.location(), | ||
| geminiProperties.model() | ||
| ); | ||
| } catch (Exception e) { | ||
| log.warn("GeminiClient 초기화 실패 (테스트 환경이거나 인증 정보 없음): {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
GeminiClient.init()에서 예외를 catch 후 경고 로그만 남기고 애플리케이션을 계속 기동하면, 운영 환경에서 인증/설정 문제를 조기에 감지하지 못하고 기능이 조용히 비활성화될 수 있습니다(이후 호출은 null 반환/기본 응답으로 degradate). 테스트 환경에서만 무시하려는 목적이라면 프로필/프로퍼티 기반으로 조건부로 swallow 하거나, 테스트에서는 @MockBean/@TestConfiguration로 빈 자체를 대체하고 운영에서는 초기화 실패 시 예외를 다시 던져 fail-fast 하도록 분리해 주세요.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/gg/agit/konect/support/ControllerTestSupport.java (1)
95-95:⚠️ Potential issue | 🟠 Major[LEVEL: medium]
supportsParameter를 항상 true로 두면 컨트롤러 바인딩 검증이 왜곡됩니다.
Line 95 설정은LoginUserArgumentResolver가 대상이 아닌 파라미터에도 적용 가능한 상태를 만들어 실제 MVC 해석 경로와 달라집니다.
이 경우 인증 사용자 파라미터 선언/설정 오류가 있어도 테스트가 통과하거나, 반대로 무관한 인자 해석에서 오탐 실패가 날 수 있습니다.
supportsParameter는 로그인 사용자 어노테이션이 있는 파라미터에서만 true가 되도록 제한해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/gg/agit/konect/support/ControllerTestSupport.java` at line 95, The test currently stubs given(loginUserArgumentResolver.supportsParameter(any())).willReturn(true) which makes LoginUserArgumentResolver claim support for every parameter; change the stub so supportsParameter returns true only for MethodParameter instances that have the LoginUser annotation (e.g., using an argument matcher/argThat that checks p.hasParameterAnnotation(LoginUser.class)) and return false otherwise, keeping the stub scoped to the LoginUser-annotated parameter so controller binding behavior in tests matches real MVC resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.java`:
- Around line 79-81: The GeminiClient constructor/initialization currently
swallows all Exceptions in the catch block (log.warn(...) in GeminiClient),
hiding real startup failures; change the catch to rethrow the exception (or wrap
in a RuntimeException) unless the application is running under a test profile
(e.g., check active profile or Environment.isTestProfile()), so only in test
contexts you log-and-continue but in non-test profiles you fail-fast by
rethrowing the caught exception; update the catch around GeminiClient
initialization to branch on the profile and rethrow when not test.
In
`@src/test/java/gg/agit/konect/domain/club/controller/ClubBasicControllerIntegrationTest.java`:
- Around line 130-131: The test in ClubBasicControllerIntegrationTest hardcodes
a non-existent club id as 99999 which can collide over time; update the test
that calls performGet("/clubs/99999") to compute a guaranteed-missing id at
runtime (e.g., query current max club id and add a large offset or create a club
and use id+1 then delete it) or choose a much larger constant, ensuring the
computed id is used in performGet; change references in the test method(s) that
call performGet to use this dynamically computed missingId variable.
In
`@src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java`:
- Around line 360-364: The test in ClubMemberControllerIntegrationTest uses
performGet("/clubs/" + club.getId() + "/pre-members") and only asserts
jsonPath("$.preMembers").isArray(), which allows non-empty arrays; change the
assertion to verify the array is empty by adding a size check such as
jsonPath("$.preMembers", hasSize(0)) so the expectation matches the "no
pre-members" scenario—ensure any required static import (e.g.,
org.hamcrest.Matchers.hasSize) is added and replace or append the isArray()
assertion accordingly.
In
`@src/test/java/gg/agit/konect/domain/schedule/controller/ScheduleControllerIntegrationTest.java`:
- Around line 54-55: Replace the use of LocalDateTime.now() (the local variable
now in ScheduleControllerIntegrationTest) with a fixed, deterministic instant
for the test (e.g., a hard-coded LocalDateTime or a Clock injected/fixed for the
test) so the test is not time-sensitive; update the occurrences where now is set
(the current uses at the shown spot and the other instances indicated around
lines 84-85 and 111-112) to use that fixed time or Clock to ensure reproducible
test behavior.
In
`@src/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.java`:
- Around line 74-84: The test method startTimerWhenAlreadyRunningFails should
assert the success of the prerequisite timer-start call so the conflict check is
meaningful; update the first performPost("/studytimes/timers") in
StudyTimeControllerIntegrationTest.startTimerWhenAlreadyRunningFails to include
an expectation (e.g., andExpect(status().isOk())) to ensure the initial timer
was started before calling performPost("/studytimes/timers") again for the 409
assertion.
In `@src/test/java/gg/agit/konect/support/ControllerTestSupport.java`:
- Around line 48-62: The inline `@TestPropertySource` block duplicated in
ControllerTestSupport (the block starting at the annotation) should be extracted
into a single shared location used by both ControllerTestSupport and
IntegrationTestSupport; create a common meta-annotation or an abstract base test
class (e.g., SharedTestProperties or `@CommonTestProperties`) that declares the
existing properties and replace the inline `@TestPropertySource` on
ControllerTestSupport and IntegrationTestSupport with a reference to that shared
meta-annotation or have them extend the new abstract base so there is a single
source of truth for the test properties.
---
Outside diff comments:
In `@src/test/java/gg/agit/konect/support/ControllerTestSupport.java`:
- Line 95: The test currently stubs
given(loginUserArgumentResolver.supportsParameter(any())).willReturn(true) which
makes LoginUserArgumentResolver claim support for every parameter; change the
stub so supportsParameter returns true only for MethodParameter instances that
have the LoginUser annotation (e.g., using an argument matcher/argThat that
checks p.hasParameterAnnotation(LoginUser.class)) and return false otherwise,
keeping the stub scoped to the LoginUser-annotated parameter so controller
binding behavior in tests matches real MVC resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 960debab-1f21-4858-88f9-a584f59b28eb
📒 Files selected for processing (15)
src/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.javasrc/test/java/gg/agit/konect/KonectApplicationTests.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubApplicationControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubBasicControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubRecruitmentControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/schedule/controller/ScheduleControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/studytime/service/StudyTimerServiceIntegrationTest.javasrc/test/java/gg/agit/konect/domain/user/controller/UserControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/user/service/UserServiceIntegrationTest.javasrc/test/java/gg/agit/konect/support/ControllerTestSupport.javasrc/test/java/gg/agit/konect/support/IntegrationTestSupport.javasrc/test/java/gg/agit/konect/support/TestGeminiConfig.javasrc/test/resources/application-test.yml
💤 Files with no reviewable changes (2)
- src/test/java/gg/agit/konect/domain/user/service/UserServiceIntegrationTest.java
- src/test/java/gg/agit/konect/domain/studytime/service/StudyTimerServiceIntegrationTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/test/java/gg/agit/konect/support/TestGeminiConfig.javasrc/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/user/controller/UserControllerIntegrationTest.javasrc/test/java/gg/agit/konect/support/IntegrationTestSupport.javasrc/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.javasrc/test/resources/application-test.ymlsrc/test/java/gg/agit/konect/support/ControllerTestSupport.javasrc/test/java/gg/agit/konect/KonectApplicationTests.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubBasicControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubRecruitmentControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/schedule/controller/ScheduleControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubApplicationControllerIntegrationTest.java
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.java
🪛 Checkov (3.2.334)
src/test/resources/application-test.yml
[low] 123-124: Base64 High Entropy String
(CKV_SECRET_6)
🔇 Additional comments (2)
src/test/java/gg/agit/konect/domain/user/controller/UserControllerIntegrationTest.java (1)
32-60: [LEVEL: INFO] 탈퇴 정책의 핵심 분기(회장 거절/무소속 성공)를 HTTP 상태로 명확히 검증한 점이 좋습니다.컨트롤러 레벨로 옮긴 목적과 맞게 400/204 결과를 직접 확인하고 있어 회귀 탐지력이 충분합니다.
src/test/java/gg/agit/konect/domain/club/controller/ClubApplicationControllerIntegrationTest.java (1)
74-81: [LEVEL: INFO] 응답 상태 검증과 DB 상태 검증을 함께 수행한 테스트 구성이 좋습니다.단순 HTTP 상태 확인을 넘어 실제 저장/멤버십 반영 여부까지 확인해서 회귀를 잘 잡을 수 있습니다.
Also applies to: 183-186, 223-225
Comment on lines
+79
to
+81
| } catch (Exception e) { | ||
| log.warn("GeminiClient 초기화 실패 (테스트 환경이거나 인증 정보 없음): {}", e.getMessage()); | ||
| } |
There was a problem hiding this comment.
[LEVEL: high] 초기화 예외를 전부 무시하면 운영 장애가 숨겨집니다.
Line 79-81에서 Exception을 모두 잡아 경고만 남기고 반환해 초기화 실패가 부트 시점에 드러나지 않습니다.
GCP 인증/권한 오류가 발생해도 서비스는 정상 기동처럼 보이고 이후 AI 응답이 지속 실패해 장애 탐지와 원인 파악이 늦어질 수 있습니다.
테스트 프로필에서만 예외를 허용하고, 그 외 프로필에서는 예외를 재던져 fail-fast 하도록 분기해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/gg/agit/konect/infrastructure/gemini/client/GeminiClient.java`
around lines 79 - 81, The GeminiClient constructor/initialization currently
swallows all Exceptions in the catch block (log.warn(...) in GeminiClient),
hiding real startup failures; change the catch to rethrow the exception (or wrap
in a RuntimeException) unless the application is running under a test profile
(e.g., check active profile or Environment.isTestProfile()), so only in test
contexts you log-and-continue but in non-test profiles you fail-fast by
rethrowing the caught exception; update the catch around GeminiClient
initialization to branch on the profile and rethrow when not test.
Comment on lines
+130
to
+131
| performGet("/clubs/99999") | ||
| .andExpect(status().isNotFound()); |
There was a problem hiding this comment.
[LEVEL: MINOR] 존재하지 않는 동아리 ID를 99999로 고정하면 장기적으로 충돌 가능성이 있습니다.
비존재 보장을 위해 충분히 큰 값을 사용하거나 테스트 내에서 동적으로 계산하는 편이 안정적입니다.
최소 수정 예시
- performGet("/clubs/99999")
+ performGet("/clubs/" + Integer.MAX_VALUE)
.andExpect(status().isNotFound());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| performGet("/clubs/99999") | |
| .andExpect(status().isNotFound()); | |
| performGet("/clubs/" + Integer.MAX_VALUE) | |
| .andExpect(status().isNotFound()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/gg/agit/konect/domain/club/controller/ClubBasicControllerIntegrationTest.java`
around lines 130 - 131, The test in ClubBasicControllerIntegrationTest hardcodes
a non-existent club id as 99999 which can collide over time; update the test
that calls performGet("/clubs/99999") to compute a guaranteed-missing id at
runtime (e.g., query current max club id and add a large offset or create a club
and use id+1 then delete it) or choose a much larger constant, ensuring the
computed id is used in performGet; change references in the test method(s) that
call performGet to use this dynamically computed missingId variable.
src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java
Show resolved
Hide resolved
Comment on lines
54
to
55
| LocalDateTime now = LocalDateTime.now(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
[LEVEL: MINOR] LocalDateTime.now() 기반 데이터는 시간 경계에서 테스트를 간헐적으로 불안정하게 만들 수 있습니다.
기준 시간을 고정하면 테스트 재현성과 디버깅 난이도가 좋아집니다.
최소 수정 예시
+ private static final LocalDateTime BASE_TIME = LocalDateTime.of(2026, 3, 1, 12, 0);
...
- LocalDateTime now = LocalDateTime.now();
+ LocalDateTime now = BASE_TIME;As per coding guidelines "가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다."
Also applies to: 84-85, 111-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/gg/agit/konect/domain/schedule/controller/ScheduleControllerIntegrationTest.java`
around lines 54 - 55, Replace the use of LocalDateTime.now() (the local variable
now in ScheduleControllerIntegrationTest) with a fixed, deterministic instant
for the test (e.g., a hard-coded LocalDateTime or a Clock injected/fixed for the
test) so the test is not time-sensitive; update the occurrences where now is set
(the current uses at the shown spot and the other instances indicated around
lines 84-85 and 111-112) to use that fixed time or Clock to ensure reproducible
test behavior.
...test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.java
Show resolved
Hide resolved
src/test/java/gg/agit/konect/support/ControllerTestSupport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java`:
- Around line 342-346: The test currently constructs ClubPreMemberAddRequest
with a null position which can trigger validation failures before the permission
check; change the request to use a valid ClubPosition enum value (e.g.,
ClubPosition.MEMBER) when creating the ClubPreMemberAddRequest in
ClubMemberControllerIntegrationTest so that performPost("/clubs/" + club.getId()
+ "/pre-members", request) reaches the authorization path and reliably asserts
status().isForbidden().
In `@src/test/java/gg/agit/konect/support/ControllerTestSupport.java`:
- Around line 98-107: Extract the duplicated persistence helper methods into a
single abstract test base: create an abstract class (e.g.,
PersistenceContextTestSupport) that declares the `@Autowired` EntityManager and
the protected methods persist(T entity) and clearPersistenceContext(), then have
ControllerTestSupport and IntegrationTestSupport extend that new base and remove
their local copies; ensure you reference and use the existing method names
persist and clearPersistenceContext and the shared entityManager so behavior and
signatures remain identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e8f854b-04f5-488f-8dc5-2a825a6b2c65
📒 Files selected for processing (5)
src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.javasrc/test/java/gg/agit/konect/support/ControllerTestSupport.javasrc/test/java/gg/agit/konect/support/IntegrationTestSupport.javasrc/test/java/gg/agit/konect/support/TestPropertyConfig.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.javasrc/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.javasrc/test/java/gg/agit/konect/support/IntegrationTestSupport.javasrc/test/java/gg/agit/konect/support/TestPropertyConfig.javasrc/test/java/gg/agit/konect/support/ControllerTestSupport.java
🔇 Additional comments (6)
src/test/java/gg/agit/konect/support/IntegrationTestSupport.java (1)
14-18: [LEVEL: low] 테스트 설정 통합 방향이 일관되고 적절합니다.Line 14와 Line 18에서
TestGeminiConfig와@TestPropertyConfig를 사용해 테스트 컨텍스트 구성이 명시적으로 정리되었습니다.src/test/java/gg/agit/konect/domain/studytime/controller/StudyTimeControllerIntegrationTest.java (2)
74-85: [LEVEL: low] 전제 조건 검증이 포함되어 충돌 시나리오의 원인 분리가 명확합니다.Line 79-80에서 선행 타이머 시작 성공을 먼저 고정한 뒤 Line 83-84에서 409를 검증해 테스트 의도가 분명합니다.
123-131: [LEVEL: low] 응답 검증과 영속 상태 검증을 함께 수행한 점이 좋습니다.Line 130에서 타이머 삭제 여부까지 확인해 API 결과와 DB 상태를 함께 보장하고 있습니다.
src/test/java/gg/agit/konect/support/TestPropertyConfig.java (1)
12-26: [LEVEL: low] 테스트 프로퍼티를 메타 어노테이션으로 공통화한 변경이 적절합니다.동일 설정을 한 곳에서 관리할 수 있어 테스트 베이스 클래스 간 일관성 유지에 유리합니다.
src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java (1)
360-364: [LEVEL: low] 빈 목록 시나리오에서 크기까지 검증한 점이 좋습니다.Line 364의
hasSize(0)로 시나리오 기대값이 명확히 고정되었습니다.src/test/java/gg/agit/konect/support/ControllerTestSupport.java (1)
82-88: [LEVEL: low]supportsParameter()목 구현이 프로덕션 조건과 정합적입니다.Line 82-88의 조건(
@UserId+Integer/int)이 실제LoginUserArgumentResolver구현과 동일해 테스트 신뢰성이 높습니다.
Comment on lines
+342
to
+346
| ClubPreMemberAddRequest request = new ClubPreMemberAddRequest("2022000001", "신입생", null); | ||
|
|
||
| // then | ||
| assertThat(response.preMembers()).hasSize(2); | ||
| // when & then | ||
| performPost("/clubs/" + club.getId() + "/pre-members", request) | ||
| .andExpect(status().isForbidden()); |
There was a problem hiding this comment.
[LEVEL: low] 권한 실패 테스트는 유효한 요청 본문으로 고정해 원인 분리를 강화해 주세요.
Line 342에서 ClubPosition을 null로 보내면, 검증 순서에 따라 403 대신 요청 유효성 실패가 먼저 발생할 여지가 있습니다. 권한 테스트 의도를 고정하려면 유효한 직책 값을 사용하는 편이 안전합니다.
최소 수정 예시
- ClubPreMemberAddRequest request = new ClubPreMemberAddRequest("2022000001", "신입생", null);
+ ClubPreMemberAddRequest request = new ClubPreMemberAddRequest("2022000001", "신입생", ClubPosition.MEMBER);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ClubPreMemberAddRequest request = new ClubPreMemberAddRequest("2022000001", "신입생", null); | |
| // then | |
| assertThat(response.preMembers()).hasSize(2); | |
| // when & then | |
| performPost("/clubs/" + club.getId() + "/pre-members", request) | |
| .andExpect(status().isForbidden()); | |
| ClubPreMemberAddRequest request = new ClubPreMemberAddRequest("2022000001", "신입생", ClubPosition.MEMBER); | |
| // when & then | |
| performPost("/clubs/" + club.getId() + "/pre-members", request) | |
| .andExpect(status().isForbidden()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/gg/agit/konect/domain/club/controller/ClubMemberControllerIntegrationTest.java`
around lines 342 - 346, The test currently constructs ClubPreMemberAddRequest
with a null position which can trigger validation failures before the permission
check; change the request to use a valid ClubPosition enum value (e.g.,
ClubPosition.MEMBER) when creating the ClubPreMemberAddRequest in
ClubMemberControllerIntegrationTest so that performPost("/clubs/" + club.getId()
+ "/pre-members", request) reaches the authorization path and reliably asserts
status().isForbidden().
Comment on lines
+98
to
+107
| protected <T> T persist(T entity) { | ||
| entityManager.persist(entity); | ||
| entityManager.flush(); | ||
| return entity; | ||
| } | ||
|
|
||
| protected void clearPersistenceContext() { | ||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
[LEVEL: medium] 영속성 헬퍼 중복을 공통 베이스로 통합해 설정 드리프트를 줄여 주세요.
Line 98-107의 persist, clearPersistenceContext는 src/test/java/gg/agit/konect/support/IntegrationTestSupport.java의 동일 메서드와 중복됩니다. 공통 추상 베이스로 추출해 변경 지점을 단일화하는 것이 안전합니다.
최소 수정 예시
-public abstract class ControllerTestSupport {
+public abstract class ControllerTestSupport extends PersistenceContextTestSupport {
@@
- `@Autowired`
- protected EntityManager entityManager;
@@
- protected <T> T persist(T entity) {
- entityManager.persist(entity);
- entityManager.flush();
- return entity;
- }
-
- protected void clearPersistenceContext() {
- entityManager.flush();
- entityManager.clear();
- }// src/test/java/gg/agit/konect/support/PersistenceContextTestSupport.java
public abstract class PersistenceContextTestSupport {
`@Autowired`
protected EntityManager entityManager;
protected <T> T persist(T entity) {
entityManager.persist(entity);
entityManager.flush();
return entity;
}
protected void clearPersistenceContext() {
entityManager.flush();
entityManager.clear();
}
}As per coding guidelines **/*: "가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/gg/agit/konect/support/ControllerTestSupport.java` around lines
98 - 107, Extract the duplicated persistence helper methods into a single
abstract test base: create an abstract class (e.g.,
PersistenceContextTestSupport) that declares the `@Autowired` EntityManager and
the protected methods persist(T entity) and clearPersistenceContext(), then have
ControllerTestSupport and IntegrationTestSupport extend that new base and remove
their local copies; ensure you reference and use the existing method names
persist and clearPersistenceContext and the shared entityManager so behavior and
signatures remain identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 개요
🚀 주요 변경 내용
TestGeminiConfig추가했습니다.Service->Controller로 변경했습니다.💬 참고 사항
✅ Checklist (완료 조건)