[Release] Dev -> Prod 병합 #68
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: 브랜치 및 띄어쓰기 린트 수정
* chore: Gemini Code 컨벤션 초안 작성 * chore: 가장 상단 제거 및 스타일 일치 * chore: 마지막 줄 개행 추가
* [Refactor] @tag를 이용한 느린 테스트 그룹 격리 (#54) * test: SlowTest 어노테이션 추가 * chore: Slow Tag 테스트 Task 분리 * test: 테스트 코드 분리 및 Slow 테스트 적용 * test: 동시성 테스트 SlowTest 적용 * test: 동시성 테스트 디렉토리 수정 * test: 잘못된 HTTP Method, URI 수정 * chore: CI 스크립트 -PincludeSlowTests 옵션 적용 * test: URI 복수형 적용 및 Static Import * test: @slowtest 어노테이션 제거 * [Refactor] @BeforeAll을 활용한 TestHelper 객체 재사용 (#56) * test: 메서드 분리 및 축제 저장 로직 if 문 추가 * test: 테스트 추가되는 Bean 분리 적용 * feat: password encoder 과정 test 제외 * test: 테스트 password encoder 시간 기본 값 적용으로 최적화 * test: password encoder Mock 처리 * refactor: @primary 어노테이션 적용 * refactor: BDDMockito 스타일 변경 * test: Mockito 방식에서 직접 구현 형태로 변경
* chore: 리뷰 리마인더 스크립트 추가 * chore: Slack 리뷰 리마인더 워크플로 추가 * chore: 이름 변경 Slack review reminder * style: 주석 설명 변경 * style: Slack 리뷰 메시지 텍스트 수정 및 구분선 추가 * style: 지연 PR 리스트에 번호 추가
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough이 PR은 테스트 인프라와 CI/리뷰 관련 설정을 포괄적으로 변경합니다. Runnable 기반의 동시성 테스트 헬퍼를 Callable 기반으로 재설계하고 ConcurrencyTestResult를 도입해 HTTP 응답 상태를 집계하도록 변경했습니다. 느린 테스트를 표기하는 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. 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 |
Summary of ChangesHello @taek2222, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 자동화된 코드 리뷰 설정( Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 dev 브랜치를 prod에 병합하는 릴리스로 보입니다. 동시성 테스트를 위한 새로운 ConcurrencyTestHelper를 도입하고 기존 테스트를 리팩토링하는 등 테스트 프레임워크에 상당한 개선이 포함되어 있습니다. 또한 Slack으로 리뷰 알림을 보내는 GitHub Action 스크립트도 추가되었습니다. Code Rabbit 설정은 Java 코드에 대한 더 상세한 리뷰 지침으로 업데이트되었습니다. 전반적으로 이러한 변경 사항은 프로젝트의 개발 및 CI/CD 프로세스를 향상시킵니다. 몇 가지 개선을 위한 제안 사항이 있습니다.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@build.gradle`:
- Around line 101-106: 현재 test { useJUnitPlatform { if
(!project.hasProperty('includeSlowTests')) { excludeTags 'slow' } } } 코드는 프로퍼티의
존재 여부만 검사해 '-PincludeSlowTests=false'를 넘겨도 느린 테스트가 포함되는 버그가 있습니다; 수정 방법은 두 가지 중
하나를 선택하세요: (A) 권장: Gradle Provider API를 사용해
project.providers.gradleProperty("includeSlowTests").forUseAtConfigurationTime().orNull
또는 hasValue() 등으로 값을 안전하게 읽어 boolean으로 파싱해 false일 때 excludeTags 'slow'를 적용하도록
변경(참조: useJUnitPlatform 블록과 includeSlowTests 프로퍼티); 또는 (B) 단순:
project.findProperty('includeSlowTests')로 값을 읽어 null 체크 후
String.valueOf(...).toBoolean() 같은 방식으로 false를 올바르게 처리하여 excludeTags 'slow' 조건을
결정하도록 변경(참조: hasProperty(), findProperty(), excludeTags).
In `@src/main/resources/application.yml`:
- Around line 20-21: The spring.profiles.include entry currently lists "server"
but there is no corresponding application-server.yml, causing the profile to be
ignored; either create an application-server.yml with the server-related
settings (e.g., port, context-path, graceful shutdown, Tomcat thread pool and
timeouts) and move those server configs from application.yml into it, or remove
"server" from spring.profiles.include in application.yml to keep all server
settings in a single file; update references to spring.profiles.include and
ensure application-server.yml exists if you choose the profile route.
In
`@src/test/java/com/daedan/festabook/announcement/concurrency/AnnouncementConcurrencyTest.java`:
- Line 26: Add the `@SlowTest` annotation to the AnnouncementConcurrencyTest class
declaration so the concurrency-heavy test is excluded from default test runs;
update the class header in AnnouncementConcurrencyTest (class
AnnouncementConcurrencyTest extends AcceptanceTestSupport) to include `@SlowTest`
and add the corresponding import for the SlowTest annotation if it's not already
present.
In `@src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java`:
- Around line 38-48: In ConcurrencyTestHelper, don’t swallow exceptions
silently: in the worker runnable’s catch (Exception ignore) replace the empty
ignore with a log statement using the class logger (from `@Slf4j`) such as
log.error("Exception in concurrent task", e) so task failures are visible; and
in the outer catch (InterruptedException ignore) restore the interrupt status by
calling Thread.currentThread().interrupt() and also log the interruption (e.g.,
log.warn("ConcurrencyTestHelper interrupted", ie)) so upstream code can observe
the interrupt.
🧹 Nitpick comments (14)
infra/buildspec.yml (1)
18-18: SERVER_YML 미설정/손상 시 실패 원인이 불명확해질 수 있어요.현재는
SERVER_YML이 비어있거나 base64가 깨졌을 때base64 -d오류만 남고 실패 이유가 불명확해질 수 있습니다. 릴리즈 파이프라인에서는 원인 메시지가 명확해야 디버깅 비용이 줄어듭니다.
대안을 고민해볼 만합니다:
- 옵션 A: 명시적 유효성 검사 후 실패 처리
- 장점: 실패 원인이 즉시 드러남(“SERVER_YML missing” 등).
- 단점: 스크립트가 길어짐.
- 옵션 B:
set -euo pipefail로 엄격 모드 적용 + 명시적 오류 메시지
- 장점: 누락된 환경변수도 즉시 감지.
- 단점: 기존 단계에서 예상치 못한 중단 가능성.
예시(옵션 A):
🔧 개선 예시
- echo "⚙️ [Pre Build Phase] 환경파일 생성 시작" + - if [ -z "$SERVER_YML" ]; then echo "❌ SERVER_YML is missing"; exit 1; fi - 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이렇게 하면 “왜 실패했는지”가 로그에 명확히 남습니다.
.gemini/styleguide.md (1)
21-28: 마크다운 중첩 리스트 들여쓰기 불일치 (선택적 수정)정적 분석 도구(markdownlint)에서 중첩 리스트의 들여쓰기가 2칸이 아닌 4칸으로 되어 있다고 경고하고 있습니다. 현재 방식도 가독성 면에서 문제는 없지만, 일관된 마크다운 스타일을 위해 2칸 들여쓰기로 통일하는 것을 고려해 볼 수 있습니다.
선택지:
- 옵션 A (현상 유지): 4칸 들여쓰기는 시각적 가독성이 좋고, 대부분의 마크다운 렌더러에서 정상 동작합니다.
- 옵션 B (2칸으로 변경): markdownlint 표준을 따르면 다른 도구들과의 호환성이 높아지고,
.markdownlint.json설정 없이도 CI에서 경고가 사라집니다.현재 프로젝트에 마크다운 린트 규칙이 강제되지 않는다면 현상 유지도 괜찮습니다.
.github/scripts/review-reminder.js (4)
10-14: USER_MAP 하드코딩은 유지보수 부담을 증가시킵니다.현재 GitHub 사용자명과 Slack ID 매핑이 코드에 직접 하드코딩되어 있습니다. 팀원이 추가되거나 변경될 때마다 코드 수정이 필요하고, 이는 민감한 Slack ID 정보가 코드 히스토리에 남게 됩니다.
대안 선택지:
- 환경변수/시크릿 사용: JSON 형태로
SLACK_USER_MAP시크릿을 설정하고 파싱하는 방식. 코드 변경 없이 설정만으로 관리 가능하나, JSON 파싱 로직이 추가됩니다.- 별도 설정 파일:
.github/config/user-map.json같은 설정 파일로 분리. 코드와 설정이 분리되어 가독성이 좋으나, 파일 I/O가 추가됩니다.- 현재 방식 유지: 팀 규모가 작고 변동이 적다면 현재 방식도 괜찮습니다.
현재 팀 규모가 3명으로 작다면 당장은 문제없으나, 향후 확장성을 고려해 개선을 검토해 보시길 권장합니다.
94-107: 마지막 PR 뒤의 불필요한 divider 제거를 권장합니다.현재 로직은 모든 PR 섹션 뒤에 divider를 추가하므로, 마지막 PR 다음에도 불필요한 구분선이 표시됩니다. Slack 메시지의 시각적 완성도를 위해 마지막 항목 이후에는 divider를 생략하는 것이 좋습니다.
♻️ 개선 제안
// 지연 PR 하나씩 메시지 추가 delayedPrs.forEach((pr, index) => { message.blocks.push({ "type": "section", "text": { "type": "mrkdwn", "text": `*${index + 1}. <${pr.url}|${pr.title}>*\n` + `⏳ *${pr.hours}시간* 경과\n` + `👤 작성자: ${pr.author}\n` + `👀 리뷰어: ${pr.reviewers}` } }); - message.blocks.push({"type": "divider"}); + // 마지막 항목이 아닌 경우에만 divider 추가 + if (index < delayedPrs.length - 1) { + message.blocks.push({"type": "divider"}); + } });
67-70: 지연 PR이 없을 때의 로깅 추가를 고려해 보세요.현재는 지연 PR이 없으면 조용히 종료됩니다. 디버깅이나 모니터링 관점에서, 스크립트가 정상 실행되었으나 알림 대상이 없었다는 것을 로그로 남기면 Actions 실행 기록 확인 시 유용합니다.
♻️ 로깅 추가 예시
// 지연 PR 없을 시 종료 if (delayedPrs.length === 0) { + console.log(`✅ ${LIMIT_HOURS}시간 이상 대기 중인 PR이 없습니다.`); return; }
109-117: fetch 호출에 명시적인 타임아웃 설정이 필요합니다.현재 코드는 네트워크 지연이나 Slack 서버 문제로 fetch 요청이 무한정 대기할 가능성이 있습니다. GitHub Actions의 기본 작업 타임아웃(6시간)이 있지만, 개별 네트워크 요청에 명시적인 타임아웃을 설정하면 예측 가능한 에러 처리와 빠른 실패(fail-fast) 원칙을 구현할 수 있습니다. 이를 통해 장시간 hanging으로 인한 리소스 낭비를 방지하고, 문제 상황에 대한 명확한 로그를 남길 수 있습니다.
Node.js 20에서 실행되는 현재 환경에서는
AbortSignal.timeout()을 사용할 수 있습니다. 이는 표준 웹 API이며, 별도 라이브러리 의존성 없이 사용 가능한 장점이 있습니다.⏱️ 타임아웃 적용 예시
const response = await fetch(WEBHOOK_URL, { + signal: AbortSignal.timeout(10000), // 10초 타임아웃 method: 'POST', headers: {'Content-Type': 'application/json'}, body: JSON.stringify(message) });.github/workflows/slack-review-reminder.yml (1)
9-11: job에 timeout-minutes 설정 추가를 권장합니다.네트워크 이슈 등으로 스크립트가 무한 대기할 경우를 대비해, job 레벨에서 명시적인 타임아웃을 설정하면 Actions 분 사용량을 보호할 수 있습니다.
♻️ timeout 추가 예시
jobs: remind: runs-on: ubuntu-latest + timeout-minutes: 5src/test/java/com/daedan/festabook/support/ConcurrencyTestHelper.java (1)
25-43: 스레드 풀 크기와 요청 분배 로직의 의도를 명확히 문서화하면 좋겠습니다.현재 구현에서
tomcatThreadCount개의 스레드가 생성되고, 전달된requests가 라운드-로빈 방식으로 분배됩니다. 이 설계의 의도는 이해되지만, 코드만으로는 다음 사항이 명확하지 않을 수 있습니다:
- 왜 Tomcat 스레드 수를 기준으로 동시 요청 수를 결정하는가?
- 요청이 1개일 때와 여러 개일 때의 동작 차이
예를 들어,
requests가 1개이면 동일 요청이tomcatThreadCount번 실행되고, 2개이면 번갈아 실행됩니다. 이는 의도된 설계일 수 있으나, 간단한 JavaDoc이나 주석으로 의도를 명시하면 유지보수성이 향상됩니다.src/test/java/com/daedan/festabook/announcement/concurrency/AnnouncementConcurrencyTest.java (2)
60-66: 동시성 테스트의 검증 범위 확장을 권장합니다.현재 테스트는 최종 DB 상태(
countByFestivalIdAndIsPinnedTrue)만 검증하고 있습니다. 이는 결과적 정합성은 확인하지만, 동시성 제어가 실제로 동작했는지에 대한 직접적인 증거가 부족합니다.
FestivalNotificationConcurrencyTest(관련 코드 스니펫 참조)에서 보여주는 패턴처럼,ConcurrencyTestResult를 활용하여 다음을 추가로 검증하면 테스트의 신뢰성이 높아집니다:
- 성공 응답 수: 정확히 3개의 요청만
HttpStatus.CREATED를 반환했는지- 실패 응답 수: 나머지 요청이 예상된 에러 코드(
HttpStatus.BAD_REQUEST)를 반환했는지♻️ 검증 강화 예시
// when - concurrencyTestHelper.execute(httpRequest); + ConcurrencyTestResult concurrencyResult = concurrencyTestHelper.execute(httpRequest); // then Long result = announcementJpaRepository.countByFestivalIdAndIsPinnedTrue(festival.getId()); - assertThat(result).isEqualTo(expectedPinnedAnnouncementCount); + int successCount = concurrencyResult.getStatusCodeCount(HttpStatus.CREATED); + + assertSoftly(s -> { + s.assertThat(result).isEqualTo(expectedPinnedAnnouncementCount); + s.assertThat(successCount).isEqualTo(expectedPinnedAnnouncementCount); + s.assertThat(concurrencyResult.getStatusCodeCount(HttpStatus.BAD_REQUEST)) + .isEqualTo(concurrencyResult.getRequestCount() - successCount); + });
105-111:updateAnnouncement테스트에도 동일한 검증 강화가 필요합니다.
createAnnouncement테스트와 마찬가지로, 이 테스트도 최종 DB 상태만 검증하고 있습니다. 생성과 수정이 동시에 발생하는 시나리오에서는 다음 사항을 추가로 검증하면 테스트의 가치가 높아집니다:
- 생성 요청 중 몇 개가 성공했는지 (
HttpStatus.CREATED)- 수정 요청의 결과 (
HttpStatus.OK또는HttpStatus.BAD_REQUEST)- 전체 요청 대비 성공/실패 비율
이렇게 하면 동시성 제어 메커니즘(예: 낙관적 락, 비관적 락)이 의도대로 동작하는지 더 명확하게 확인할 수 있습니다.
src/test/java/com/daedan/festabook/festival/concurrency/FestivalNotificationConcurrencyTest.java (4)
25-25:@DisplayNameGeneration어노테이션 누락팀 컨벤션에 따르면 테스트 클래스에는
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)어노테이션을 명시해야 합니다. 이 어노테이션이 없으면 테스트 실행 결과에서 메서드명의 언더스코어가 그대로 노출되어 가독성이 떨어집니다.♻️ 권장 수정안
+import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) class FestivalNotificationConcurrencyTest extends AcceptanceTestSupport {As per coding guidelines, 테스트 클래스에는 해당 어노테이션을 작성해야 합니다.
50-55: RestAssured 개행 컨벤션 조정 필요현재 람다 표현식 내에서
RestAssured다음에 개행 후.given()이 위치하고 있습니다. 팀 컨벤션에서는given()앞에서 개행하여RestAssured.given()형태로 작성하도록 안내하고 있습니다. 이렇게 하면 RestAssured의 시작점이 명확해지고, 체이닝의 흐름을 한눈에 파악하기 쉬워집니다.♻️ 권장 수정안
- Callable<Response> httpRequest = () -> RestAssured - .given() + Callable<Response> httpRequest = () -> + RestAssured.given() .contentType(ContentType.JSON) .body(request) .when() .post("/festivals/{festivalId}/notifications", festival.getId());As per coding guidelines,
given()앞에서 개행하여RestAssured.given()형태로 작성합니다.
66-71: HTTP 상태 코드 검증 로직의 견고성 검토현재 assertion에서
CONFLICT상태 코드 수가전체 요청 수 - 성공 수와 같은지 검증하고 있습니다. 이 로직은 모든 실패 응답이 CONFLICT(409)라는 가정에 의존합니다.만약 동시성 테스트 중 예기치 않은 오류(500 Internal Server Error, 400 Bad Request 등)가 발생한다면, 이 assertion은 실패하게 되고 실제 문제의 원인을 파악하기 어려워질 수 있습니다.
선택지를 제안드립니다:
옵션 A (현 상태 유지): 테스트 환경이 안정적이고, CREATED/CONFLICT 외 다른 상태 코드가 발생하지 않는다고 확신할 수 있다면 현재 구현도 유효합니다. 단, 테스트 실패 시 디버깅이 어려울 수 있습니다.
옵션 B (견고성 강화): 예상치 못한 상태 코드 발생 시 명확한 실패 메시지를 제공하도록 개선합니다. 이렇게 하면 테스트 실패 원인을 빠르게 파악할 수 있습니다.
♻️ 옵션 B 구현 예시
assertSoftly(s -> { s.assertThat(notificationCount).isEqualTo(1); s.assertThat(successCount).isEqualTo(1); - s.assertThat(result.getStatusCodeCount(HttpStatus.CONFLICT)) - .isEqualTo(result.getRequestCount() - successCount); + + int conflictCount = result.getStatusCodeCount(HttpStatus.CONFLICT); + long expectedConflictCount = result.getRequestCount() - successCount; + + s.assertThat(conflictCount) + .as("CONFLICT 응답 수가 예상과 다릅니다. 예상치 못한 상태 코드가 발생했을 수 있습니다.") + .isEqualTo(expectedConflictCount); });추가로,
ConcurrencyTestResult클래스에 전체 상태 코드 분포를 출력하는 메서드가 있다면 테스트 실패 시 디버깅에 큰 도움이 될 것입니다. 이는 향후 개선 사항으로 고려해 보시기 바랍니다.
39-40: 테스트 메서드 네이밍 컨벤션 확인팀 컨벤션에서 테스트 메서드명은
성공 - 사유/조건또는실패 - 사유형식을 따르도록 안내하고 있습니다. 현재 메서드명동시성_중복_알림_등록_방지는 테스트의 목적을 잘 설명하지만, 컨벤션과는 다소 차이가 있습니다.동시성 테스트의 특성상 '성공/실패'로 단순 구분하기 어려운 측면이 있어, 현재 네이밍이 의도를 더 명확히 전달한다고 판단되신다면 유지하셔도 무방합니다. 다만 일관성 측면에서
성공_동시_요청시_하나만_등록됨과 같은 형태도 고려해 보실 수 있습니다.
src/test/java/com/daedan/festabook/announcement/concurrency/AnnouncementConcurrencyTest.java
Show resolved
Hide resolved
* chore: Opened 알림 메시지 고도화 * chore: Opened Push 테스트 실행 * chore: Opened Push 트리거 제거 * chore: 리리퀘, 완료 메시지도 고도화 * chore: 입력 값 이스케이핑 적용 및 일관성 유지
|
|
Summary by CodeRabbit
새 기능
개선 사항
테스트
문서
✏️ Tip: You can customize this high-level summary in your review settings.