[FIX] 코드 예시 noteNumbers 불변 리스트로 반환 - #202
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changes코드 예시 음 번호 반환
홈 서비스 테스트 시간대
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/mr/domain/home/service/HomeServiceTest.java (1)
148-150: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift현재 시각을 고정해 자정 경계의 테스트 실패를 방지하세요.
테스트 픽스처와
HomeService가LocalDate.now(ZoneId.of("Asia/Seoul"))를 각각 호출합니다. 테스트가 한국 시간 자정 전후에 실행되면 두 호출이 서로 다른 날짜를 반환할 수 있습니다. 이 경우 어제 출석 테스트와 주간·월간 연습 시간 테스트가 간헐적으로 실패할 수 있습니다.
HomeService에 고정 가능한 시간 제공자를 주입하고, 테스트에서 동일한 고정 시각을 사용하세요. Javajava.time.Clock의 fixed time 패턴을 참고하면 됩니다.Also applies to: 163-166, 180-182, 194-196, 215-215, 230-234, 249-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/domain/home/service/HomeServiceTest.java` around lines 148 - 150, HomeService and HomeServiceTest independently call LocalDate.now, causing midnight-boundary flakes. Inject a java.time.Clock into HomeService, derive the Seoul date from that clock, and configure the tests—including the referenced fixtures and assertions—to use the same fixed clock instant and Asia/Seoul zone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/java/com/mr/domain/home/service/HomeServiceTest.java`:
- Around line 148-150: HomeService and HomeServiceTest independently call
LocalDate.now, causing midnight-boundary flakes. Inject a java.time.Clock into
HomeService, derive the Seoul date from that clock, and configure the
tests—including the referenced fixtures and assertions—to use the same fixed
clock instant and Asia/Seoul zone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 558f1e9e-6792-4921-85c0-3506124eb4a9
📒 Files selected for processing (1)
src/test/java/com/mr/domain/home/service/HomeServiceTest.java
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
ChordExampleItem.from()에서noteNumbers를List.copyOf()로 감싸 불변 스냅샷으로 반환하도록 수정PersistentBag)을 태우는 통합 테스트 추가🔥 리뷰 요청 사항
PM님이 리뷰에서 지적해주신 크래시 건입니다. 상황 공유드립니다.
noteNumbers는@ElementCollection이라 운영 환경에서는 Hibernate가 관리하는 컬렉션 타입으로 로드되는데, 이걸 DTO에 그대로 담아 반환하다가 에러가 났던 것으로 보입니다.LearningServiceTest)에 assertion만 추가했는데,mock(ChordExample.class)가List.of(...)를 반환하도록 스텁되어 있어서 수정 전/후 구분 없이 항상 통과하는 걸 확인했습니다(즉 이 크래시를 재현 못 하는 테스트).ChordExampleNoteNumbersIntegrationTest를 새로 추가해서, 실제 로컬 DB에 저장 후entityManager.flush()+clear()로 영속성 컨텍스트를 비우고 다시 조회해 진짜 Hibernate 컬렉션을 태워봤습니다. 수정 전 코드로는 이 테스트가 실제로 실패(noteNumbers.add(1)이 예외 없이 성공)하고, 수정 후엔 통과하는 것까지 확인했습니다.noteNumbers()반환 타입 자체(List<Integer>)는 안 바뀌어서 API 응답 스펙에는 영향 없습니다.✅ 체크리스트
📎 참고 사항
@ElementCollection/@OneToMany반환하는 다른 DTO도 한 번씩 점검해볼 만합니다.🔎 크래시 원인 추가 확인 (디스코드 논의 정리)
open-in-view가false로 변경됐습니다 (DB 커넥션 과다 점유로 서버 다운되던 문제 대응 차 원정님이 의도적으로 설정)ChordExample.noteNumbers(@ElementCollection(fetch = LAZY))는 기존 코드에서 실제 값을 안 읽고 Hibernate 프록시 참조만 DTO에 담아 넘기고 있었습니다List.copyOf(chordExample.getNoteNumbers())수정은 세션이 아직 열려있는 트랜잭션 내부에서 값을 즉시 읽어 복사해두기 때문에, 세션이 닫힌 뒤에도 안전하게 직렬화됩니다open-in-view=false가 전역 설정이라, 다른 도메인에도@OneToMany/@ElementCollection을 DTO에 그대로 노출하는 곳이 있으면 같은 패턴으로 터질 수 있어서 한 번씩 점검해봐도 좋을 것 같습니다.🙋 쉽게 설명하면
noteNumbers는 원래 "DB 가면 있음"이라는 꼬리표만 붙어있던 값이었어요 (필요할 때만 진짜로 가져오는 방식)List.copyOf)로 바꾼 거고, 그게 크래시 원인이랑 정확히 맞아떨어졌습니다!🐛 CI 간헐적 실패 조사 결과 (develop 최신화 과정에서 별도로 발견)
이 PR을 develop에 맞춰 merge하는 과정에서 CI가 간헐적으로 실패해서 원인을 팠습니다. 이번 PR 내용(ChordExample)이랑은 무관한 별개 버그입니다.
HomeService의 "오늘 날짜" 계산을LocalDate.now()→LocalDate.now(ZoneId.of("Asia/Seoul"))로 명시적으로 고쳤는데,HomeServiceTest.java의 테스트 3곳(getHome_noPracticeToday_countsFromYesterday등)이 이 수정에서 빠져서 여전히 시간대 지정 없는LocalDate.now()를 쓰고 있었습니다.expected: 1, but was: 0으로 실패했습니다.ChordExample필드 초기화 커밋과 실패 타이밍이 겹쳐서 그게 원인인 줄 의심했는데, 코드를 대조해보니 무관했습니다(HomeServiceTest는learning도메인 코드를 전혀 참조하지 않음). 오히려 그 커밋을 새벽 3시에 푸시해서 CI가 하필 위험한 시간대(UTC/KST 날짜가 어긋나는 구간)에 돈 게 우연히 겹친 것이었습니다.HomeServiceTest.java의 누락된 3곳을 같은 파일의 다른 테스트들과 동일하게Asia/Seoul명시로 맞춰서 고쳤습니다 (수정 전 코드로는 로컬에서도 재현 확인, 수정 후 통과 확인).팀 전체 참고: 같은 이유로 한국 새벽 0~~9시 사이에 푸시하면, 시간대 처리가 빠진 다른 코드도 지금 작업 내용과 무관하게 CI가 실패할 수 있습니다.ㄴ #193 머지된 상태라 같은 이유로 ci 문제는 안 생길 겁니다!
Summary by CodeRabbit
개선 사항
테스트