Dev #57
Dev #57
Conversation
* chore: 서버 튜닝 Yml 적용 * chore: SERVER 오타 변경
* test: 동시성 Helper 스레드 수 기반 동작 변환 * test: 동시성 테스트 결과 객체 추가 및 Runnable→Callable로 전환 * test: Exception 예외 상태 코드 증가 추가 * test: 동시성 테스트 잘못된 부분 수정 * test: 동시성 Helper 패키지 위치 변경 * test: Error Count 및 Cause 추가 * test: assertAll 형태로 변경 * test: Softly 변경 * test: 현재 사용하지 않는 Error 추적 객체 삭제
* chore: CodeRabbit 1차 수정 * chore: 브랜치 및 띄어쓰기 린트 수정
📝 WalkthroughWalkthrough이 PR은 ConcurrencyTestHelper를 static 유틸리티 클래스에서 Spring 관리형 컴포넌트로 마이그레이션합니다. Runnable 기반 API를 Callable\<Response\> 기반으로 변경하고, 스레드 풀 크기를 서버 설정(server.tomcat.threads.max)과 동기화하며, 새로운 ConcurrencyTestResult 클래스로 결과를 수집합니다. .coderabbit.yaml은 경로 기반 지침(path_instructions)으로 통합되고, application.yml에 "server" 프로파일이 추가되며, buildspec.yml에 SERVER_YML 환경 변수 복호화 단계가 추가됩니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
📋 상세 분석이 변경사항은 아키텍처 레벨의 리팩토링으로, 여러 차원의 개선을 포함하고 있습니다. 각 영역별 검토 포인트를 정리하겠습니다. 1️⃣ ConcurrencyTestHelper 마이그레이션 관점변경의 배경 및 효과: 기존 static 유틸리티 방식에서 Spring 컴포넌트로의 전환은 의존성 주입(DI) 관점에서의 개선입니다. 다음과 같은 이점이 있습니다:
다만, 검토 시 고려할 점:
학습적 관점의 제언: 동시성 테스트 헬퍼가 Spring 컴포넌트가 되면서 설정과 테스트 로직의 강결합이 증가합니다. 향후 다음을 고려하면 좋습니다:
2️⃣ ConcurrencyTestResult 설계 평가강점:
주의 영역: 3️⃣ 테스트 파일 업데이트 검토AnnouncementControllerTest와 FestivalNotificationConcurrencyTest의 변경을 보면: 일관성 측면:
잠재적 우려: 개선 방향: // 현재 방식 (암묵적):
assertThat(result.getStatusCodeCount(HttpStatus.CONFLICT))
.isEqualTo(requestCount - 1);
// 개선 방식 (명시적):
softly.assertThat(result.getStatusCodeCount(HttpStatus.CREATED))
.as("성공한 요청 수")
.isEqualTo(1);
softly.assertThat(result.getStatusCodeCount(HttpStatus.CONFLICT))
.as("충돌한 요청 수")
.isEqualTo(requestCount - 1);
softly.assertThat(result.getRequestCount())
.as("전체 요청 수")
.isEqualTo(requestCount);4️⃣ 설정 변경사항 (application.yml)변경 내용:
검토 포인트: 5️⃣ .coderabbit.yaml 리팩토링이 변경은 리뷰 프로세스 자동화 설정입니다. 기존의 여러 토글(high_level_summary, collapse_walkthrough 등)을 경로 기반(path_instructions) 지침으로 통합했습니다. 효과:
고려할 점:
최종 권고사항
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
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 |
|
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java (2)
457-473: 동시성 테스트 결과 검증 누락
concurrencyTestHelper.execute()가 반환하는ConcurrencyTestResult를 검증하지 않고 있습니다.문제점:
- 동시 요청 중 일부가 실패해도 테스트가 성공으로 판정됩니다
ConcurrencyTestHelper가 예외를 무시하기 때문에, 실제 요청 실패 여부를 알 수 없습니다- 테스트가 실제로는 실패해야 하는 상황을 놓칠 수 있습니다 (false positive)
예상 시나리오:
- 100개의 동시 요청 중 97개가 실패하고 3개만 성공해도, 현재 테스트는 DB에 3개가 저장된 것을 확인하고 통과합니다
- 하지만 실제로는 97개의 요청이 예외를 발생시켰고, 이는 심각한 문제를 나타낼 수 있습니다
🔎 결과 검증 추가 제안
// when - concurrencyTestHelper.execute(httpRequest); + ConcurrencyTestResult result = concurrencyTestHelper.execute(httpRequest); // then + // 모든 요청이 성공하거나 실패(고정 공지 최대치 초과)했는지 검증 + assertSoftly(s -> { + s.assertThat(result.getTotalRequests()) + .as("총 요청 수") + .isEqualTo(tomcatThreadCount); + s.assertThat(result.getStatusCount(HttpStatus.CREATED) + result.getStatusCount(HttpStatus.BAD_REQUEST)) + .as("성공(201) 또는 예상된 실패(400) 응답 수") + .isEqualTo(tomcatThreadCount); + s.assertThat(result.getExceptionCount()) + .as("예외 발생 수") + .isZero(); + }); + Long pinnedCount = announcementJpaRepository.countByFestivalIdAndIsPinnedTrue(festival.getId()); assertThat(pinnedCount).isEqualTo(expectedPinnedAnnouncementCount);참고: 이 검증을 추가하려면 먼저
ConcurrencyTestHelper의 예외 처리 문제를 해결해야 합니다.
489-513: 동시성 테스트 결과 검증 누락 (두 번째 테스트)첫 번째 테스트와 동일한 문제가 있습니다.
ConcurrencyTestResult를 검증하지 않아 동시 요청의 성공/실패 여부를 확인할 수 없습니다.추가 고려사항:
이 테스트는 **두 가지 다른 API(생성/수정)**를 동시에 호출하므로, 각 API별 성공/실패 비율을 세밀하게 검증해야 더 의미 있는 테스트가 됩니다.🔎 결과 검증 추가 제안
// when - concurrencyTestHelper.execute(createAnnouncementRequest, updateAnnouncementRequest); + ConcurrencyTestResult result = concurrencyTestHelper.execute(createAnnouncementRequest, updateAnnouncementRequest); // then + assertSoftly(s -> { + s.assertThat(result.getTotalRequests()) + .as("총 요청 수") + .isEqualTo(tomcatThreadCount); + s.assertThat(result.getSuccessCount()) + .as("성공 응답 수 (2xx)") + .isGreaterThan(0) + .as("최소한 일부 요청은 성공해야 함"); + s.assertThat(result.getExceptionCount()) + .as("예외 발생 수") + .isZero(); + }); + Long pinnedCount = announcementJpaRepository.countByFestivalIdAndIsPinnedTrue(festival.getId()); assertThat(pinnedCount).isEqualTo(expectedPinnedAnnouncementCount);선택지:
- 옵션 A: 엄격한 검증 - 모든 요청이 예상된 상태 코드(201/200/400)로 응답했는지 확인
- 옵션 B: 느슨한 검증 - 예외가 발생하지 않았고, 최종 DB 상태만 확인 (현재 방식 + 예외 체크)
동시성 테스트의 목적이 "경쟁 조건에서도 비즈니스 규칙(고정 공지 3개 제한)이 지켜지는가"라면, 옵션 B도 충분할 수 있습니다. 하지만 예외가 발생하지 않았다는 최소한의 검증은 필수입니다.
🤖 Fix all issues with AI agents
In @infra/buildspec.yml:
- Line 18: The current pipeline echoes "$SERVER_YML" and decodes it to
src/main/resources/application-server.yml without validation; change this to
"fail fast" by first checking the SERVER_YML env var is set and non-empty,
emitting a clear error and exiting non-zero if missing, then perform the base64
decode and verify the decode succeeded and that
src/main/resources/application-server.yml is non-empty (otherwise echo an error
and exit non-zero); ensure the checks surround the echo "$SERVER_YML" | base64
-d > src/main/resources/application-server.yml step so CI fails loudly on
missing/invalid SERVER_YML or failed decoding.
In @src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java:
- Around line 20-50: The execute(...) method currently ignores exceptions and
calls ConcurrencyTestResult.recordStatusCode(...) from multiple threads causing
race conditions and lost failures; make ConcurrencyTestResult thread-safe (e.g.,
use AtomicInteger/ConcurrentHashMap) and add a thread-safe
recordException(Throwable t) so worker threads can report failures; in the
lambda catch block call result.recordException(e) and log the error instead of
swallowing it; for InterruptedException in the outer try/catch restore the
interrupt status with Thread.currentThread().interrupt() and propagate or fail
the test so cancellation/timeouts are respected.
- Around line 17-18: 필드 tomcatThreadCount(@Value("${server.tomcat.threads.max}")
private int tomcatThreadCount)에 기본값이 없어서 설정 누락 시 애플리케이션이 실패합니다;
ConcurrencyTestHelper 클래스의 해당 @Value 어노테이션을 기본값을 포함하도록 변경(예:
"${server.tomcat.threads.max:100}")하여 설정이 없을 때 안전한 기본 스레드 수를 사용하도록 하세요.
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
58-59:base_branches: [".*"]패턴의 영향 검토
".*"정규식은 모든 브랜치를 매칭합니다. 이는 범용적이지만, 아래 상황을 고려해 보세요:
- 장점: 모든 브랜치에서 일관된 리뷰 적용, 설정 단순화
- 단점: WIP/feature 브랜치에서도 리뷰가 트리거되어 불필요한 노이즈 발생 가능, API 호출 증가
특정 브랜치만 리뷰하고 싶다면
["main", "dev", "release/.*"]처럼 명시적으로 지정하는 것도 방법입니다. 현재 설정이 의도된 것이라면 무시하셔도 됩니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.coderabbit.yamlinfra/buildspec.ymlsrc/main/resources/application.ymlsrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/support/AcceptanceTestSupport.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestResult.javasrc/test/resources/application.yml
💤 Files with no reviewable changes (1)
- src/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.java
🧰 Additional context used
📓 Path-based instructions (5)
**/application*.yml
📄 CodeRabbit inference engine (code-style.md)
설정: application.yml을 사용하고 properties는 사용하지 않는다.
Files:
src/main/resources/application.ymlsrc/test/resources/application.yml
src/**
⚙️ CodeRabbit configuration file
src/**: 당신은 팀의 기술적 의사결정을 돕는 '프린시펄 엔지니어(Principal Engineer)'입니다.
단순한 코드 교정이 아닌, '시스템의 안정성'과 '설계의 방향성'을 제시하여 개발자가 스스로 최적의 판단(Trade-off)을 내릴 수 있도록 돕는 것이 목표입니다.
- [Critical: 시스템 안정성 및 성능 심화] (OOM, 동시성, 리소스)
- 메모리 및 리소스 누수:
Stream.collect()로 대량의 데이터를 힙에 로드하거나,static컬렉션에 데이터가 무한히 쌓이는 패턴(OOM 위험)을 찾아내세요.try-with-resources미사용,ThreadLocal미정리 등 리소스 누수 가능성을 차단하세요.- 동시성(Concurrency) 및 락:
- 공유 자원에 대한 'Check-Then-Act' 레이스 컨디션을 찾아내고,
Atomic변수나ConcurrentHashMap활용을 제안하세요.- Java 21 Virtual Thread 환경에서 장시간 블로킹되는 I/O 작업이
synchronized블록 내에 있을 경우 스레드 피닝(Pinning)이 발생할 수 있으니, 이러한 경우에는ReentrantLock사용을 고려하도록 안내하세요.- 실패 격리 및 트랜잭션:
- 외부 API 호출(Network I/O)이
@Transactional범위 내에 있어 DB 커넥션을 오래 점유하는지 확인하고, 분리를 제안하세요.- 무분별한 재시도(Retry)로 인한 트래픽 폭주(Retry Storm) 가능성을 경고하세요.
- [Architecture: 최신 설계 트렌드 및 방향성 고민]
- Modern Java & 가독성:
- 단순히 최신 문법(Record, Pattern Matching, Switch Expression)을 쓰는 것을 넘어, 그것이 '불변성(Immutability)'과 '명확한 의도 전달'에 기여하는지 확인하세요.
- 복잡한 상속보다는 조합(Composition)을, 명령형 로직보다는 선언형(Stream/Functional) 스타일을 권장하되, 과도한 체이닝으로 디버깅이 어려워질 경우엔 가독성을 우선하세요.
- 설계적 사고(Design Thinking):
- 도메인 로직이 서비스 계층에만 비대하게 몰리는 'Transaction Script' 패턴을 경계하고, 도메인 객체 자체가 로직을 수행하는 '풍부한 도메인 모델(Rich Domain Model)' 방향을 제시해 주세요.
- "이 로직이 과연 이 클래스의 책임인가?"를 질문하여 SRP(단일 책임 원칙)와 응집도에 대해 고민하게 만드세요.
- [Test Strategy: 견고함 검증]
- Controller:
RestAssured+RANDOM_PORT를 사용한 인수 테스트(E2E)여야 하며,MockMvc사용은 지양합니다.- Service/Domain: 외부 의존성을 배제한 순수 단위 테스트를 지향하세요.
- 단순 커버리지보다는 동시성 테스트, 엣지 케이스(Null, 경계값), 예외 발생 시나리오가 포함되었는지 확인하세요.
- [Feedback Style: 트레이드 오프와 의사결정 유도]
지적보다는 제안: "이건 틀렸습니다" 대신 "이 방식은 A라는 장점이 있지만 B라는 리스크가 있습니다"라고 설명해 주세요.
선택지 제공:
- 옵션 A: 성능이 최우선일 때의 접근법 (예: 캐싱 도입, 복잡도 증가)
- 옵션 B: 유지보수성과 가독성이 최우선일 때의 접근법 (예: 구조 단순화)
- 위와 같이 선택지를 주고 개발자가 상황에 맞춰 결정하도록 유도하세요.
리뷰 피로도를 낮추기 위해 핵심적인 문제(Critical/Major)를 선별해서 코멘트 달아주세요.
모든 피드백은 정중한 한국어로 작성해 주세요.
Files:
src/main/resources/application.ymlsrc/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/support/AcceptanceTestSupport.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestResult.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.javasrc/test/resources/application.yml
**/*.java
📄 CodeRabbit inference engine (code-style.md)
**/*.java: Enum 네이밍: 관행을 의식하지 않고 해당 도메인에 어울리는 이름으로 이해하기 쉽게 작성하며, 필요한 경우 용어 사전에도 등록한다.
메서드 접두사: 데이터를 확실하게 가져올 수 없고 호출자가 체크해야 한다면 'find' 접두사로 작성한다 (예: Optional 반환)
메서드 접두사: 데이터를 확실하게 가져올 수 있을 때 'get' 접두사로 작성한다.
final 키워드 사용: 클래스 필드 변수를 제외하고는 final 키워드를 사용하지 않는다.
어노테이션 순서: 어노테이션의 순서는 상단에서 하단으로 어노테이션 길이가 짧은 순서대로 배치한다 (피라미드 구조)
파라미터 순서: 실제 사용 순서대로 파라미터 순서를 정렬한다.
Bean 주입 방식: 생성자 주입은 lombok의 @requiredargsconstructor를 사용한다.
개행: 빈 Record의 첫 줄은 개행한다.
개행: 클래스의 첫 줄은 개행한다.
개행: 한 줄에는 한개의 점만 올 수 있도록 2번째 점부터 개행한다. Stream() 은 예외적으로 이 규칙을 회피할 수 있고 Stream() 다음에는 첫 줄을 개행한다.
개행: 마지막 닫는 괄호와 세미콜론을 개행하여 작성한다 (리턴문에서 여러 체이닝이 있을 때).
개행: 인터페이스에 내용이 있다면 첫 줄을 개행한다.
래퍼 타입 vs 원시 타입: 기본적으로 래퍼 타입을 사용하고 null이 들어오지 않는다는 확신이 생길 때만 primitive type을 사용한다.
Files:
src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/support/AcceptanceTestSupport.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestResult.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
**/*.{java,ts,tsx,js}
📄 CodeRabbit inference engine (code-style.md)
**/*.{java,ts,tsx,js}: 변수 네이밍: 줄임말을 사용하지 않는다.
메서드 네이밍: 줄임말을 사용하지 않는다.
Files:
src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/support/AcceptanceTestSupport.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestResult.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
**/*Test.java
📄 CodeRabbit inference engine (code-style.md)
**/*Test.java: 테스트 코드 대상: RestAssured를 사용하여 API를 테스트한다.
테스트 코드 컨벤션: @DisplayName은 사용하지 않는다.
테스트 메서드: 한글로 작성하고 숫자로 시작한다면 _로 시작한다.
테스트 클래스: @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)를 작성한다.
테스트 메서드 구조: 테스트하려는 메서드마다 @nested를 작성하고 클래스 명은 테스트하려는 메서드 명과 동일하게 작성한다.
테스트 메서드 네이밍: 메서드 명은 '성공 - 사유/조건', '실패 - 사유'로 작성한다.
테스트 변수 네이밍: 테스트 결과 값과 기댓값 변수의 이름은 'expected', 'result'로 작성한다.
테스트 모킹: 모킹과 모킹된 객체 주입은 @mock, @Injectmocks를 사용한다.
테스트 필드 순서: @LocalServerPort는 필드 순서의 가장 마지막에 작성한다.
테스트 상태 검증: HttpStatus를 사용하여 상태 코드 검증을 작성한다.
테스트 DTO 검증: dto 필드를 검증할 때 필드의 수 검증도 작성한다.
테스트 컬렉션 검증: 컬렉션의 사이즈 검증은 '$' + 'hasSize()'로 검증을 작성한다.
테스트 개행: Mockito.given() 뒤 첫 점부터 개행한다.
테스트 개행: assertThat()은 체이닝이 한 줄을 넘어가거나 여러 체이닝인 경우 개행한다.
테스트 개행: given() 앞에서 개행하여 RestAssured.given() 형태로 작성한다.
Files:
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
🧠 Learnings (4)
📚 Learning: 2026-01-06T04:11:44.293Z
Learnt from: CR
Repo: festabook/backend PR: 0
File: code-style.md:0-0
Timestamp: 2026-01-06T04:11:44.293Z
Learning: Applies to **/*Test.java : 테스트 코드 대상: RestAssured를 사용하여 API를 테스트한다.
Applied to files:
src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
📚 Learning: 2026-01-06T04:11:44.293Z
Learnt from: CR
Repo: festabook/backend PR: 0
File: code-style.md:0-0
Timestamp: 2026-01-06T04:11:44.293Z
Learning: Applies to **/*Test.java : 테스트 상태 검증: HttpStatus를 사용하여 상태 코드 검증을 작성한다.
Applied to files:
src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java
📚 Learning: 2026-01-05T03:09:15.636Z
Learnt from: taek2222
Repo: festabook/backend PR: 0
File: :0-0
Timestamp: 2026-01-05T03:09:15.636Z
Learning: CodeRabbit의 `finishing_touches` 설정(docstrings, unit_tests)은 기본값이 `true`이므로, 설정 파일에서 명시하지 않아도 자동으로 활성화됩니다.
Applied to files:
.coderabbit.yaml
📚 Learning: 2026-01-06T04:11:44.293Z
Learnt from: CR
Repo: festabook/backend PR: 0
File: code-style.md:0-0
Timestamp: 2026-01-06T04:11:44.293Z
Learning: Applies to **/*Fixture.java : Test Fixture 다른 Fixture 사용: 다른 Fixture를 사용할 때도 정적 필드로 선언한다.
Applied to files:
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java
🧬 Code graph analysis (1)
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java (1)
src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
ConcurrencyTestResult(8-25)
⏰ 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: Run-PR-Test
🔇 Additional comments (8)
.coderabbit.yaml (3)
17-56: path_instructions 구성 승인
Principal Engineer관점의 리뷰 가이드라인이 잘 구성되어 있습니다. 특히:
- Java 21 Virtual Thread의
synchronized블록 내 I/O 블로킹 시 스레드 피닝(Pinning) 경고는 실질적으로 유용한 지침입니다.- 트랜잭션 내 외부 API 호출 분리 권고와 Retry Storm 경고는 실무에서 자주 발생하는 문제를 선제적으로 방지합니다.
- "지적보다는 제안" 방식의 피드백 스타일 가이드는 코드 리뷰 문화에 긍정적으로 기여할 것입니다.
73-102: code_generation 설정 승인Docstring과 Unit Test 생성 지침이 일관성 있게 구성되어 있습니다:
- Docstrings (lines 78-85): Getter/Setter/Override 제외 규칙과 서술형 문장 스타일은 불필요한 문서화를 줄이고 가독성을 높입니다.
- Unit Tests (lines 89-102):
@DisplayName대신 한글 메서드명 + 접두어(성공_,예외_) 방식은 테스트 결과 리포트에서도 직관적으로 의도가 드러나는 장점이 있습니다.- Controller 테스트에서
RestAssured강제는reviews.path_instructions의 E2E 테스트 전략(line 44)과 일치하여 설정 간 정합성이 유지됩니다.
3-3:tone_instructions는 이미 스키마 제약을 충족합니다현재 설정된
tone_instructions의 실제 길이는 239자로, 스키마 상한선인 250자 이내에 있습니다. 추가 조치가 필요하지 않습니다.src/main/resources/application.yml (1)
21-21: 프로파일 분리 전략이 명확합니다.서버 관련 설정을 별도 프로파일로 분리하는 접근은 환경별 설정 관리 측면에서 합리적입니다. 테스트 환경에서 server.tomcat.threads 설정과 함께 사용되는 구조가 일관성 있게 구성되었습니다.
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java (1)
40-72: 동시성 테스트 구조가 명확하고 검증 로직이 견고합니다.이번 리팩토링으로 테스트 코드의 의도가 훨씬 명확해졌습니다:
개선된 점:
- Callable 패턴 전환: 기존 AtomicInteger 기반 에러 카운팅보다 ConcurrencyTestResult를 통한 결과 집계가 더 직관적입니다.
- SoftAssertions 활용: 여러 검증을 그룹화하여 실패 시 모든 assertion 결과를 한 번에 확인할 수 있습니다.
- 검증 로직의 완결성:
- DB 레코드 1개 생성 확인 (line 67)
- HTTP 201 응답 1개 확인 (line 68)
- 나머지 요청은 모두 HTTP 409 확인 (lines 69-70)
이 세 가지 assertion을 통해 "정확히 하나만 성공하고 나머지는 중복으로 거부되었다"는 것을 명확히 검증하고 있습니다.
테스트 코드 컨벤션 준수:
- 한글 메서드명 ✓
- @nested 구조 ✓
- SoftAssertions 사용 ✓
- 변수명 규칙 준수 (expected/result 패턴은 아니지만, notificationCount/successCount로 의도가 명확함) ✓
src/test/java/com/daedan/festabook/support/AcceptanceTestSupport.java (1)
18-24: 테스트 지원 클래스에 ConcurrencyTestHelper가 깔끔하게 통합되었습니다.
@Import와@Autowired를 통해 ConcurrencyTestHelper를 모든 acceptance 테스트에서 사용할 수 있도록 구성한 점이 좋습니다. 상속을 통해 중복 없이 동시성 테스트 인프라를 제공할 수 있게 되었습니다.의존성 주입 방식에 대한 참고:
- 현재 필드 주입(
@Autowired)을 사용하고 있습니다.- 프로덕션 코드에서는 생성자 주입(
@RequiredArgsConstructor)이 권장되지만, 테스트 지원 클래스에서는 필드 주입도 일반적으로 허용되는 관행입니다.- Trade-off:
- 필드 주입은 테스트 코드의 간결성을 높이고, 베이스 클래스 확장 시 유연합니다.
- 생성자 주입은 불변성과 명시적 의존성을 보장하지만, 테스트 베이스 클래스에서는 과도한 보일러플레이트를 야기할 수 있습니다.
현재 맥락에서는 적절한 선택으로 보입니다.
src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
8-25: 동시성 결과 집계 클래스가 목적에 맞게 잘 설계되었습니다.
ConcurrentHashMap과AtomicLong을 활용하여 스레드 안전한 결과 수집 구조를 구현했습니다. 테스트 유틸리티로서 명확한 책임을 가지고 있고, API도 직관적입니다.동시성 안전성 분석:
- ✅
statusCodeCounts.merge(status, 1, Integer::sum): 개별 키에 대해 원자적 연산- ✅
requestCount.incrementAndGet(): 원자적 증가 연산- 🔍 참고사항 (이론적 고려):
- 13-15번 라인의 두 연산은 전체적으로는 원자적이지 않습니다.
- 즉,
statusCodeCounts가 업데이트된 후requestCount가 증가되기 전 찰나의 시점에, 다른 스레드가 읽기를 수행하면 일시적으로 불일치를 볼 수 있습니다.- 실무적 판단: 이 클래스는 테스트에서 "모든 요청 완료 후 결과를 읽는" 패턴으로 사용되므로, 이런 일시적 불일치는 발생하지 않습니다. 락을 추가해 복잡도를 높이는 것보다, 현재의 단순한 구조가 더 적합합니다.
코드 컨벤션 준수:
- 클래스 첫 줄 개행 ✓
- 메서드명 축약 없음 ✓
- 래퍼 타입 사용 (Integer) ✓
src/test/resources/application.yml (1)
35-38: 설정 검증 완료: Tomcat 스레드 개수와 동시성 테스트가 이미 올바르게 정렬되어 있습니다.ConcurrencyTestHelper의 구현을 확인한 결과, 동시 요청 개수와 Tomcat 스레드 제한 간에 의도된 정렬이 이루어져 있습니다:
- 동적 설정 참조 (line 17-18):
@Value("${server.tomcat.threads.max}")로 application.yml의 값을 직접 읽음- 요청 개수 자동 동기화 (line 25, 29): ExecutorService와 제출 태스크 개수를 모두
tomcatThreadCount로 설정- 의도적 설계: 서버의 스레드 풀 크기와 정확히 일치하도록 테스트 동시성을 구성
따라서 15개의 스레드 제한은 요청 큐잉 없이 순수한 동시성 테스트를 가능하게 하는 잘 설계된 패턴입니다. 추가 조치가 필요하지 않습니다.
| - echo "⚙️ [Pre Build Phase] 환경파일 생성 시작" | ||
| - mkdir -p src/main/resources/firebase | ||
| - echo "$SECRET_YML" | base64 -d > src/main/resources/application-secret.yml | ||
| - echo "$SERVER_YML" | base64 -d > src/main/resources/application-server.yml |
There was a problem hiding this comment.
환경 변수 검증 및 에러 처리 추가를 권장합니다.
현재 구현은 SERVER_YML 환경 변수가 존재하지 않거나, 비어있거나, 잘못된 base64 인코딩일 경우에도 빌드가 계속 진행됩니다. 이는 다음과 같은 문제를 야기할 수 있습니다:
- 빌드 단계에서는 성공으로 보이지만, 실제 배포 후 런타임에 설정 파일 누락으로 인한 장애 발생
- 문제 원인 파악이 어려워짐 - 빌드 로그에서는 정상으로 보이기 때문에 디버깅 시간이 증가
- 잘못된 설정이 프로덕션까지 배포될 위험
CI/CD 파이프라인에서는 "조용히 실패"하는 것보다 "명시적으로 실패"하는 것이 훨씬 안전합니다. 문제를 조기에 발견할수록 수정 비용이 낮아지기 때문입니다.
🔎 권장 개선안: 환경 변수 검증 및 에러 처리 추가
옵션 1 (권장): 환경 변수 존재 여부 사전 검증
pre_build:
commands:
- echo "⚙️ [Pre Build Phase] 환경파일 생성 시작"
- mkdir -p src/main/resources/firebase
+ - |
+ if [ -z "$SECRET_YML" ] || [ -z "$SERVER_YML" ]; then
+ echo "❌ 필수 환경 변수가 설정되지 않았습니다."
+ [ -z "$SECRET_YML" ] && echo " - SECRET_YML is not set"
+ [ -z "$SERVER_YML" ] && echo " - SERVER_YML is not set"
+ exit 1
+ fi
- echo "$SECRET_YML" | base64 -d > src/main/resources/application-secret.yml
- echo "$SERVER_YML" | base64 -d > src/main/resources/application-server.yml
- echo "$FIREBASE_ADMINSDK_ACCOUNT_KEY" > src/main/resources/firebase/firebase-adminsdk-account.json
- echo "✅ [Pre Build Phase] 완료"장점:
- 가장 명확하고 이해하기 쉬움
- 어떤 환경 변수가 누락되었는지 명시적으로 표시
- 파일 생성 전에 실패하므로 안전
단점:
- 코드가 약간 길어짐
옵션 2: 각 명령어에 개별 검증 추가
- - echo "$SERVER_YML" | base64 -d > src/main/resources/application-server.yml
+ - echo "$SERVER_YML" | base64 -d > src/main/resources/application-server.yml || { echo "❌ SERVER_YML 디코딩 실패"; exit 1; }장점:
- 최소한의 변경
- 각 명령어마다 세밀한 제어 가능
단점:
- 디코딩 시도 후에야 실패를 감지 (늦은 검증)
- 여러 줄에 반복적인 패턴 추가 필요
옵션 3: 파일 생성 후 검증
- echo "$SECRET_YML" | base64 -d > src/main/resources/application-secret.yml
- echo "$SERVER_YML" | base64 -d > src/main/resources/application-server.yml
- echo "$FIREBASE_ADMINSDK_ACCOUNT_KEY" > src/main/resources/firebase/firebase-adminsdk-account.json
+ - |
+ for file in application-secret.yml application-server.yml; do
+ if [ ! -s "src/main/resources/$file" ]; then
+ echo "❌ $file 파일이 비어있거나 생성되지 않았습니다."
+ exit 1
+ fi
+ done
- echo "✅ [Pre Build Phase] 완료"장점:
- 파일이 실제로 올바르게 생성되었는지 확인
- 빈 파일 감지 가능
단점:
- 파일 내용이 유효한 YAML인지는 검증하지 못함
- 실패 시 어떤 환경 변수가 문제인지 파악하기 어려움
개인적으로는 옵션 1을 권장합니다. 사전 검증은 "fail fast" 원칙에 부합하며, 디버깅이 가장 쉽기 때문입니다.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @infra/buildspec.yml at line 18, The current pipeline echoes "$SERVER_YML"
and decodes it to src/main/resources/application-server.yml without validation;
change this to "fail fast" by first checking the SERVER_YML env var is set and
non-empty, emitting a clear error and exiting non-zero if missing, then perform
the base64 decode and verify the decode succeeded and that
src/main/resources/application-server.yml is non-empty (otherwise echo an error
and exit non-zero); ensure the checks surround the echo "$SERVER_YML" | base64
-d > src/main/resources/application-server.yml step so CI fails loudly on
missing/invalid SERVER_YML or failed decoding.
| @Value("${server.tomcat.threads.max}") | ||
| private int tomcatThreadCount; |
There was a problem hiding this comment.
설정 값 누락 시 시작 실패 위험
@Value 어노테이션에 기본값(default value)이 지정되어 있지 않아, server.tomcat.threads.max 속성이 누락되면 애플리케이션 시작이 실패합니다.
테스트 환경 설정이 변경되거나 다른 개발자가 로컬에서 실행할 때 불필요한 실패를 방지하려면 기본값을 제공하는 것이 좋습니다.
🔎 기본값 제공 제안
- @Value("${server.tomcat.threads.max}")
+ @Value("${server.tomcat.threads.max:100}")
private int tomcatThreadCount;참고: Tomcat 기본 스레드 수는 200이지만, 테스트 환경에서는 100 정도가 적절할 수 있습니다. 프로젝트 요구사항에 맞게 조정하세요.
📝 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.
| @Value("${server.tomcat.threads.max}") | |
| private int tomcatThreadCount; | |
| @Value("${server.tomcat.threads.max:100}") | |
| private int tomcatThreadCount; |
🤖 Prompt for AI Agents
In @src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java around
lines 17 - 18, 필드 tomcatThreadCount(@Value("${server.tomcat.threads.max}")
private int tomcatThreadCount)에 기본값이 없어서 설정 누락 시 애플리케이션이 실패합니다;
ConcurrencyTestHelper 클래스의 해당 @Value 어노테이션을 기본값을 포함하도록 변경(예:
"${server.tomcat.threads.max:100}")하여 설정이 없을 때 안전한 기본 스레드 수를 사용하도록 하세요.
| @SafeVarargs | ||
| public final ConcurrencyTestResult execute(Callable<Response>... requests) { | ||
| validateRequests(requests); | ||
| ConcurrencyTestResult result = new ConcurrencyTestResult(); | ||
|
|
||
| try (ExecutorService executorService = Executors.newFixedThreadPool(tomcatThreadCount)) { | ||
| CountDownLatch startLatch = new CountDownLatch(1); | ||
| CountDownLatch endLatch = new CountDownLatch(tomcatThreadCount); | ||
|
|
||
| for (int i = 0; i < tomcatThreadCount; i++) { | ||
| int currentCount = i % requests.length; | ||
| executorService.submit(() -> { | ||
| try { | ||
| startLatch.await(); | ||
| Response response = requests[currentCount].call(); | ||
|
|
||
| HttpStatus httpStatus = HttpStatus.valueOf(response.getStatusCode()); | ||
| result.recordStatusCode(httpStatus); | ||
| } catch (Exception ignore) { | ||
| } finally { | ||
| endLatch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| startLatch.countDown(); | ||
| endLatch.await(); | ||
| } catch (InterruptedException ignore) { | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
[Critical] 동시성 안정성 및 예외 처리 문제
이 메서드는 세 가지 중요한 문제를 포함하고 있습니다:
1. 레이스 컨디션 (Line 37)
result.recordStatusCode(httpStatus);여러 스레드가 동시에 result 객체의 recordStatusCode()를 호출하고 있습니다. ConcurrencyTestResult 클래스가 내부적으로 thread-safe하지 않다면, 상태 코드 집계가 부정확해질 수 있습니다.
영향도: 테스트 결과가 비결정적(non-deterministic)이 되어 간헐적으로 잘못된 검증 결과를 내거나, 동시성 버그를 놓칠 수 있습니다.
2. 예외 무시 (Line 38)
} catch (Exception ignore) {모든 예외를 무조건 무시하고 있어, 다음 문제가 발생합니다:
- 테스트 실패가 숨겨짐:
Callable<Response>내부에서 발생한 assertion 실패, 네트워크 오류 등이 완전히 무시됩니다 - 디버깅 불가: 어떤 요청이 실패했는지, 왜 실패했는지 전혀 알 수 없습니다
- false positive 테스트: 실제로는 실패한 동시성 테스트가 성공으로 보고됩니다
3. InterruptedException 무시 (Line 47)
} catch (InterruptedException ignore) {스레드 인터럽션을 무시하면 테스트 타임아웃이나 취소 요청이 제대로 처리되지 않을 수 있습니다.
🔎 개선 방안 제안
옵션 A: ConcurrencyTestResult를 thread-safe하게 구현 + 예외 추적
@SafeVarargs
public final ConcurrencyTestResult execute(Callable<Response>... requests) {
validateRequests(requests);
ConcurrencyTestResult result = new ConcurrencyTestResult();
try (ExecutorService executorService = Executors.newFixedThreadPool(tomcatThreadCount)) {
CountDownLatch startLatch = new CountDownLatch(1);
CountDownLatch endLatch = new CountDownLatch(tomcatThreadCount);
for (int i = 0; i < tomcatThreadCount; i++) {
int currentCount = i % requests.length;
executorService.submit(() -> {
try {
startLatch.await();
Response response = requests[currentCount].call();
HttpStatus httpStatus = HttpStatus.valueOf(response.getStatusCode());
result.recordStatusCode(httpStatus);
- } catch (Exception ignore) {
+ } catch (Exception e) {
+ log.error("동시성 테스트 중 예외 발생", e);
+ result.recordException(e);
} finally {
endLatch.countDown();
}
});
}
startLatch.countDown();
endLatch.await();
- } catch (InterruptedException ignore) {
+ } catch (InterruptedException e) {
+ log.error("동시성 테스트가 인터럽트되었습니다", e);
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("동시성 테스트 실행 중 인터럽트 발생", e);
}
return result;
}이 경우 ConcurrencyTestResult에 다음 메서드를 추가해야 합니다:
recordException(Exception e): thread-safe하게 예외를 기록- 내부적으로
ConcurrentHashMap또는AtomicInteger사용
옵션 B: ExecutorService의 Future를 활용해 예외 전파
List<Future<HttpStatus>> futures = new ArrayList<>();
for (int i = 0; i < tomcatThreadCount; i++) {
int currentCount = i % requests.length;
Future<HttpStatus> future = executorService.submit(() -> {
startLatch.await();
Response response = requests[currentCount].call();
return HttpStatus.valueOf(response.getStatusCode());
});
futures.add(future);
}
startLatch.countDown();
for (Future<HttpStatus> future : futures) {
try {
HttpStatus status = future.get();
result.recordStatusCode(status);
} catch (ExecutionException e) {
log.error("요청 실행 실패", e.getCause());
result.recordException(e.getCause());
}
}권장 사항:
- 옵션 A를 추천합니다. 더 간결하며 현재 구조를 크게 변경하지 않아도 됩니다.
ConcurrencyTestResult.recordStatusCode()와recordException()을AtomicInteger기반의 카운터나ConcurrentHashMap을 사용해 thread-safe하게 구현하세요.- 예외를 로깅하고 결과 객체에 기록하여, 테스트 코드에서 검증할 수 있도록 하세요.
Based on learnings: 동시성 이슈는 시스템 안정성에 직결되는 Critical 문제입니다.
🤖 Prompt for AI Agents
In @src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java around
lines 20 - 50, The execute(...) method currently ignores exceptions and calls
ConcurrencyTestResult.recordStatusCode(...) from multiple threads causing race
conditions and lost failures; make ConcurrencyTestResult thread-safe (e.g., use
AtomicInteger/ConcurrentHashMap) and add a thread-safe recordException(Throwable
t) so worker threads can report failures; in the lambda catch block call
result.recordException(e) and log the error instead of swallowing it; for
InterruptedException in the outer try/catch restore the interrupt status with
Thread.currentThread().interrupt() and propagate or fail the test so
cancellation/timeouts are respected.
Coverage Test
Summary by CodeRabbit
릴리스 노트
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.