test(backend): 백엔드 테스트 커버리지 대폭 확장 + 진짜 버그 8건 발견·수정#52
Merged
Conversation
- MemberServiceTest 신규 — 회원가입/로그인/로그아웃/탈퇴. happy path뿐 아니라 실패 경로 (중복 이메일, 존재하지 않는 회원, 비밀번호 불일치)와 탈퇴 시 pose_data 비동기 정리 트리거 조건(afterCommit 이전엔 미호출, 세션 없으면 동기화 자체를 등록 안 함)까지 검증 - MemberDeletionCascadeIntegrationTest 신규 — 실제 DB(H2)로 회원 탈퇴 CASCADE 체인 검증 (세션·리포트·피드백로그). 작성 중 Session.member/DailyLog.member에 Hibernate @onDelete가 없어 테스트 스키마에서 탈퇴해도 세션이 안 지워지는 걸 발견 - refresh_token(2일 전), Report/SessionFeedbackLog(오늘 초반)에 이은 같은 버그 클래스. 실 mysql/schema.sql은 이미 ON DELETE CASCADE라 프로덕션 동작엔 영향 없고, JPA 엔티티 매핑과 테스트 스키마만 뒤늦게 맞춘 것 - 라이브 재배포 불필요 - refresh_token/body_records는 엔티티 매핑 구조상 이 테스트로 검증 불가함을 주석으로 명시 (전자는 Member로 가는 @manytoone 자체가 없음, 후자는 JPA 엔티티 자체가 없는 미구현 테이블) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AI가 BT-SET으로 세트 경계마다 재전송할 수 있는 피드백 batch가 uniqueKey(session_id, occurred_at, feedback_type) + INSERT IGNORE로 실제로 안전한지 검증(db-deep-dive.md §C). 실제 JdbcTemplate.batchUpdate 경로를 타야 하는 로직이라 모킹 대신 real H2로 확인. - 전체 신규 삽입, 완전 동일 재전송(전부 흡수), 일부만 겹치는 재전송(신규분만 삽입) - 존재하지 않는 세션(SESSION_NOT_FOUND, 삽입 없음), 빈 이벤트(0 반환) - 배치 중간에 잘못된 feedback_type이 있으면 INVALID_INPUT_VALUE + 같은 배치의 앞선 유효 행도 삽입 안 됨(setValues() 단계에서 예외 발생 시 executeBatch() 자체가 호출 안 됨을 실측 확인) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
테스트 커버리지 전면 확장 — 남아있던 무테스트 영역(보안, CRUD성 서비스, 컨트롤러/HTTP 레벨) 전부 커버: - JwtUtilTest, CustomUserDetailsServiceTest, JwtAuthFilterTest — 인증 메커니즘 자체가 지금까지 무테스트였음 - OnboardingServiceTest, PreferenceServiceTest, FeedbackTemplateServiceTest, SessionFeedbackQueryServiceTest - MemberControllerIntegrationTest, SessionControllerIntegrationTest, AdminAuthorizationIntegrationTest, ProtectedEndpointSmokeTest — 실제 보안 필터체인까지 태우는 MockMvc 통합테스트(컨트롤러 계층이 지금까지 전부 무테스트였음) 발견·수정한 진짜 버그 2건: 1. JwtUtil.isValidToken — io.jsonwebtoken.security.SecurityException을 명시 import 안 해서 catch절의 SecurityException이 java.lang.SecurityException으로 잘못 resolve됨. 서명 변조 토큰이 이 catch에 안 걸리고 그대로 예외를 던지던 버그. 2. @PreAuthorize("hasRole('ADMIN')") 권한 없음이 403이 아니라 500으로 나가던 버그 — GlobalExceptionHandler의 catch-all(@ExceptionHandler(Exception.class))이 MVC 디스패치 도중 발생하는 AccessDeniedException을 SecurityConfig의 CustomAccessDeniedHandler(필터체인 레벨 전용)보다 먼저 가로채서 생김. AdminExerciseController·ExercisesController. extractReference 등 @PreAuthorize 쓰는 모든 엔드포인트에 영향 — 명시적 @ExceptionHandler(AccessDeniedException.class) 추가로 수정. 전체 테스트 스위트 0 실패. 2번 버그는 실제 동작에 영향을 주는 수정이라 Docker 이미지 재빌드 + 재기동으로 라이브 반영 완료. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
무테스트로 남아있던 서비스 전부 커버 완료: - AdminExerciseServiceTest, PoseDataCleanupServiceTest, PoseDataServiceTest, ExerciseGrpcServiceTest, ExerciseAnalysisServiceTest, SessionServiceTest (createSession/getWeeklyActivity/getCalendarMain/getDailyActivity/endSession) 발견·수정한 진짜 버그(8번째) — ExerciseAnalysisService.startAnalysis가 this.sendAnalysisRequestToFastApi(...)로 자기호출(self-invocation)해서 @async가 Spring 프록시를 못 타고 조용히 동기 실행되던 문제. 같은 클래스의 completeSession→ applyCompleteFromApp는 이미 self 필드로 이 문제를 피해갔는데(주석에도 명시), 이 호출만 빠져있었음. 즉 POST /exercises/sessions가 "즉시 세션ID 반환"하는 설계 의도와 달리 FastAPI gRPC 왕복(최대 5초 데드라인)을 전부 기다린 뒤에야 응답했을 가능성 - self.로 수정해 실제 비동기 실행되도록 함(같은 클래스의 self 패턴과 통일). 전체 테스트 스위트 0 실패. 라이브 컨테이너 재빌드+재기동 완료. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough예외 처리와 JWT 검증을 보완하고, 분석 요청을 트랜잭션 커밋 이후 실행하도록 변경했습니다. 회원 삭제 시 세션·일일 로그 삭제 전파를 설정했으며, 보안·회원·운동·세션·피드백 서비스의 단위 및 통합 테스트를 추가했습니다. Changes보안 및 인증 흐름
운동 및 세션 흐름
회원 및 데이터 삭제
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (1)
backend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.java (1)
134-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winself-invocation 수정은 맞지만, 트랜잭션 커밋 전 비동기 실행 레이스가 새로 노출됩니다.
this.→self.로 바꿔@Async가 실제로 별도 스레드에서 동작하게 된 것은 올바른 수정입니다. 하지만 이제sendAnalysisRequestToFastApi가 정말로 별도 스레드/트랜잭션에서 실행되므로,startAnalysis의@Transactional커밋 이전에 비동기 작업이 시작될 수 있습니다. 서킷브레이커 OPEN이거나 gRPC 에러가 발생하면sessionService.markAsFailedIfStillInProgress(sessionId, ...)가 호출되는데, 이때 세션이 아직 커밋되지 않아 조회에 실패하면 조용히 no-op되어 실패 마킹이 누락될 수 있습니다(스케줄러의 30분+ 타임아웃까지 방치).동일 서비스의
SessionService.endSession→analysisService.stopAnalysis는 이미TransactionSynchronizationManager.registerSynchronization(afterCommit)패턴으로 커밋 후 호출을 보장합니다.startAnalysis에도 동일한 안전장치를 적용하는 것이 일관성 있고 안전합니다.As per path instructions, backend Java 파일은 트랜잭션 경계와 동시성 관점에서 중점적으로 검토해야 합니다 ("트랜잭션 경계, N+1, 인덱스 활용, 동시성(낙관적 락 등), 예외 처리 일관성, 입력 검증을 중점적으로 본다").
🔧 제안: 커밋 후 비동기 호출로 변경
Session savedSession = sessionService.createSession(appDto, currentMemberId, finalUrl); Long sessionId = savedSession.getId(); - // 비동기로 FastAPI에 분석 요청 — self를 거쳐야 `@Async가` Spring 프록시를 타고 실제로 - // 비동기 실행됨. this.로 호출하면 자기호출(self-invocation)이라 AOP 프록시를 우회해서 - // `@Async가` 조용히 무시되고 동기 실행되는 문제가 있었음(2026-07-24, 테스트로 발견) — - // completeSession→applyCompleteFromApp에 이미 쓰던 self 패턴을 여기에도 동일 적용. - self.sendAnalysisRequestToFastApi(sessionId, appDto, finalUrl, member.getSelectedPersona().name()); + // 비동기 요청은 세션 INSERT가 실제로 커밋된 이후에만 발사돼야 함 — 그래야 실패 시 + // markAsFailedIfStillInProgress가 세션을 조회할 수 있음(endSession→stopAnalysis와 동일 패턴). + String persona = member.getSelectedPersona().name(); + org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization( + new org.springframework.transaction.support.TransactionSynchronization() { + `@Override` + public void afterCommit() { + self.sendAnalysisRequestToFastApi(sessionId, appDto, finalUrl, persona); + } + } + );🤖 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 `@backend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.java` around lines 134 - 155, Update startAnalysis so self.sendAnalysisRequestToFastApi is scheduled through TransactionSynchronizationManager after the surrounding transaction commits, rather than invoked immediately. Preserve the existing async self-proxy call and pass sessionId, appDto, finalUrl, and the selected persona into the afterCommit callback.Source: Path instructions
🧹 Nitpick comments (2)
backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.java (1)
21-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value세 핸들러 간 응답 빌드 로직 중복.
handleBusinessException과handleAccessDeniedException이ErrorResponseDto빌드 로직을 거의 동일하게 반복하고 있습니다(handleUnexpectedException도 마찬가지). 공통 헬퍼로 추출하면 유지보수성이 개선됩니다.♻️ 제안 리팩터링
+ private ResponseEntity<ErrorResponseDto> buildResponse(ErrorCode code) { + return ResponseEntity + .status(code.getStatus()) + .body(ErrorResponseDto.builder() + .status(code.getStatus()) + .message(code.getMessage()) + .timestamp(LocalDateTime.now()) + .build()); + } + `@ExceptionHandler`(AccessDeniedException.class) public ResponseEntity<ErrorResponseDto> handleAccessDeniedException(AccessDeniedException e) { - ErrorCode code = ErrorCode.ACCESS_DENIED; log.warn("AccessDeniedException: {}", e.getMessage()); - return ResponseEntity - .status(code.getStatus()) - .body(ErrorResponseDto.builder() - .status(code.getStatus()) - .message(code.getMessage()) - .timestamp(LocalDateTime.now()) - .build()); + return buildResponse(ErrorCode.ACCESS_DENIED); }🤖 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 `@backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.java` around lines 21 - 53, Extract the duplicated ErrorResponseDto construction used by handleBusinessException, handleAccessDeniedException, and handleUnexpectedException into a shared private helper that accepts the relevant ErrorCode. Update all three handlers to build responses through that helper while preserving their existing statuses, messages, timestamps, and logging behavior.backend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.java (1)
162-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"AI 재통보 없음" 주장이 실제로 검증되지 않습니다.
테스트 이름은 "AI 재통보 없음"을 명시하지만, 이 테스트는
afterCommit시뮬레이션을 전혀 수행하지 않으므로analysisService.stopAnalysis는 애초에 호출될 일이 없어 어떤 값을 검증하더라도 통과합니다. 두 번째endSession호출이 새로운TransactionSynchronization을 중복 등록하지 않는지(멱등성의 핵심)는 실제로 검증되지 않은 상태입니다. 174-187행의endSession_triggersStopAnalysisOnlyAfterCommit처럼TransactionSynchronizationManager.getSynchronizations()를 커밋 시뮬레이션해stopAnalysis가 정확히 1회만 호출되는지 확인하는 것을 제안합니다.🧪 제안: 실제 재통보 여부까지 검증
`@Test` `@DisplayName`("이미 종료된 세션 재호출은 멱등 — endTime 안 바뀜, AI 재통보 없음") void endSession_alreadyEnded_isIdempotent() { Session session = inProgressSession(); sessionService.endSession(session.getId(), member.getId()); LocalDateTime firstEndTime = sessionRepository.findById(session.getId()).orElseThrow().getEndTime(); sessionService.endSession(session.getId(), member.getId()); assertThat(sessionRepository.findById(session.getId()).orElseThrow().getEndTime()).isEqualTo(firstEndTime); + + List<TransactionSynchronization> syncs = TransactionSynchronizationManager.getSynchronizations(); + syncs.forEach(TransactionSynchronization::afterCommit); + verify(analysisService, times(1)).stopAnalysis(session.getId()); // 두 번째 호출로 중복 등록되지 않았는지 확인 }🤖 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 `@backend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.java` around lines 162 - 172, Update endSession_alreadyEnded_isIdempotent to simulate transaction commit using TransactionSynchronizationManager, and verify analysisService.stopAnalysis is called exactly once across both endSession invocations. Confirm the second call does not register another synchronization, while preserving the existing endTime equality assertion.
🤖 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.
Inline comments:
In
`@backend/src/test/java/com/shadowfit/integration/MemberDeletionCascadeIntegrationTest.java`:
- Around line 47-123: Extend MemberDeletionCascadeIntegrationTest to cover
DailyLog deletion: inject the DailyLog repository, create and flush a DailyLog
associated with member in setUp, capture its ID in
deleteAccount_cascadesToAllOwnedData, and assert it cannot be found after member
deletion and EntityManager flush/clear. Keep the existing cascade assertions
unchanged.
In
`@backend/src/test/java/com/shadowfit/service/Exercise/ExerciseAnalysisServiceTest.java`:
- Around line 126-142: Update startAnalysis_success_createsSessionSynchronously
to mock or stub the asynchronous self.sendAnalysisRequestToFastApi(...) call
before invoking analysisService.startAnalysis, preventing the background gRPC
path and its connection-failure side effects while preserving the test’s
verification of synchronous IN_PROGRESS session creation.
In
`@backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java`:
- Around line 32-40: Remove `@Transactional` from PoseDataCleanupServiceTest and
ensure fixtures created by setUp() or equivalent setup logic are committed
before invoking cleanupBySessionIds(). Use a separate transaction boundary for
fixture insertion if needed, while keeping the asynchronous cleanup assertions
unchanged.
---
Outside diff comments:
In
`@backend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.java`:
- Around line 134-155: Update startAnalysis so self.sendAnalysisRequestToFastApi
is scheduled through TransactionSynchronizationManager after the surrounding
transaction commits, rather than invoked immediately. Preserve the existing
async self-proxy call and pass sessionId, appDto, finalUrl, and the selected
persona into the afterCommit callback.
---
Nitpick comments:
In
`@backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.java`:
- Around line 21-53: Extract the duplicated ErrorResponseDto construction used
by handleBusinessException, handleAccessDeniedException, and
handleUnexpectedException into a shared private helper that accepts the relevant
ErrorCode. Update all three handlers to build responses through that helper
while preserving their existing statuses, messages, timestamps, and logging
behavior.
In
`@backend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.java`:
- Around line 162-172: Update endSession_alreadyEnded_isIdempotent to simulate
transaction commit using TransactionSynchronizationManager, and verify
analysisService.stopAnalysis is called exactly once across both endSession
invocations. Confirm the second call does not register another synchronization,
while preserving the existing endTime equality assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88849aee-e3c7-430d-b049-a20e1bdc4779
📒 Files selected for processing (25)
backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.javabackend/src/main/java/com/shadowfit/global/security/jwt/JwtUtil.javabackend/src/main/java/com/shadowfit/model/exercise/Session.javabackend/src/main/java/com/shadowfit/model/report/DailyLog.javabackend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.javabackend/src/test/java/com/shadowfit/global/security/jwt/JwtAuthFilterTest.javabackend/src/test/java/com/shadowfit/global/security/jwt/JwtUtilTest.javabackend/src/test/java/com/shadowfit/integration/AdminAuthorizationIntegrationTest.javabackend/src/test/java/com/shadowfit/integration/MemberControllerIntegrationTest.javabackend/src/test/java/com/shadowfit/integration/MemberDeletionCascadeIntegrationTest.javabackend/src/test/java/com/shadowfit/integration/ProtectedEndpointSmokeTest.javabackend/src/test/java/com/shadowfit/integration/SessionControllerIntegrationTest.javabackend/src/test/java/com/shadowfit/service/Exercise/AdminExerciseServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/ExerciseAnalysisServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/ExerciseGrpcServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/FeedbackLogServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/FeedbackTemplateServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/PoseDataServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/SessionFeedbackQueryServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.javabackend/src/test/java/com/shadowfit/service/Member/CustomUserDetailsServiceTest.javabackend/src/test/java/com/shadowfit/service/Member/MemberServiceTest.javabackend/src/test/java/com/shadowfit/service/Member/OnboardingServiceTest.javabackend/src/test/java/com/shadowfit/service/Member/PreferenceServiceTest.java
Comment on lines
+126
to
+142
| @Test | ||
| @DisplayName("세션 시작 — 정상 케이스면 세션이 즉시 IN_PROGRESS로 동기 생성·반환됨 " + | ||
| "(self.sendAnalysisRequestToFastApi가 진짜 비동기로 나가 이 트랜잭션 안에서 영향 없어야 함)") | ||
| void startAnalysis_success_createsSessionSynchronously() { | ||
| VideoRequestDto dto = VideoRequestDto.builder().exerciseId(exercise.getId()).build(); | ||
|
|
||
| Long sessionId = analysisService.startAnalysis(dto, member.getId()); | ||
|
|
||
| // self.를 거쳐 실제로 @Async 프록시를 타면, 비동기 스레드는 이 테스트 트랜잭션이 커밋되기 | ||
| // 전이라 세션을 아예 못 봐서(findById 실패) markAsFailedIfStillInProgress가 조용히 | ||
| // no-op됨 — 그래서 동기 반환 직후 이 트랜잭션 안에서는 항상 IN_PROGRESS로 보여야 함. | ||
| // (self. 대신 this.로 self-invocation하면 @Async가 무시돼 동기 실행되면서 이 값이 | ||
| // 깨질 수 있음 — 2026-07-24 발견·수정한 버그의 회귀 방지 성격도 겸함) | ||
| Session created = sessionRepository.findById(sessionId).orElseThrow(); | ||
| assertThat(created.getStatus()).isEqualTo(Status.IN_PROGRESS); | ||
| assertThat(created.getMember().getId()).isEqualTo(member.getId()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# fastapi-client gRPC 채널의 테스트 설정 확인
fd -e yml -e yaml -e properties | xargs rg -n -i "fastapi-client|grpc" 2>/dev/nullRepository: Shadowfit/init
Length of output: 1186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test resources application.yml =="
cat -n backend/src/test/resources/application.yml | sed -n '1,220p'
echo
echo "== target test class =="
cat -n backend/src/test/java/com/shadowfit/service/Exercise/ExerciseAnalysisServiceTest.java | sed -n '1,260p'
echo
echo "== search for mock/replacement of fastapi-client or exerciseAsyncStub =="
rg -n "exerciseAsyncStub|fastapi-client|`@MockitoBean`|`@MockBean`|`@Mock`|`@SpyBean`|GrpcClient|CircuitBreaker|OPEN|CLOSED|sendAnalysisRequestToFastApi|getAuthenticatedStub|startAnalysis\\(" backend/src/test/java backend/src/test/resources backend/src/main/javaRepository: Shadowfit/init
Length of output: 22163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ExerciseAnalysisService relevant section =="
cat -n backend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.java | sed -n '135,250p'
echo
echo "== async / executor / grpc test configuration hints =="
rg -n "`@EnableAsync`|AsyncConfigurer|ThreadPoolTaskExecutor|`@Async`|grpc\.client\.fastapi-client|static://localhost:0|aiServer|CircuitBreakerRegistry|transitionToOpenState|transitionToClosedState" backend/src/main/java backend/src/test/java backend/src/test/resources backend/src/main/resourcesRepository: Shadowfit/init
Length of output: 9063
startAnalysis_success_createsSessionSynchronously에서 비동기 gRPC 호출을 끊어주세요.
이 테스트는 self.sendAnalysisRequestToFastApi(...)가 백그라운드에서 실제 스텁을 두드릴 수 있습니다. 프로파일 주소가 static://localhost:0이라 외부 AI 서버까지는 안 가더라도, 연결 실패 로그 같은 부수효과가 남을 수 있으니 이 케이스에서는 async 경로를 목 처리해서 세션 생성만 검증하는 편이 낫습니다.
🤖 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
`@backend/src/test/java/com/shadowfit/service/Exercise/ExerciseAnalysisServiceTest.java`
around lines 126 - 142, Update startAnalysis_success_createsSessionSynchronously
to mock or stub the asynchronous self.sendAnalysisRequestToFastApi(...) call
before invoking analysisService.startAnalysis, preventing the background gRPC
path and its connection-failure side effects while preserving the test’s
verification of synchronous IN_PROGRESS session creation.
Member
Author
There was a problem hiding this comment.
검토했습니다 — 이 케이스는 목 처리가 기술적으로 불가능하고, 사실 필요도 없다는 것까지 확인했습니다.
-
sendAnalysisRequestToFastApi는@Async라 Mockito spy로 목 처리(doNothing().when(spy)...)를 시도하면 스텁 등록 자체가 비동기 프록시를 타면서InvalidUseOfMatchersException으로 깨집니다. Spring AOP(@async) 프록시 위에 Mockito spy를 얹는 조합이 근본적으로 지원되지 않습니다. -
더 중요한 건, 애초에 목 처리할 대상이 없다는 점입니다.
self.sendAnalysisRequestToFastApi()는registerSynchronization(...).afterCommit()으로만 트리거되는데, 이 테스트 클래스는@Transactional기본 rollback 정책이라 테스트 메서드 실행 중 실제 커밋이 한 번도 일어나지 않습니다. 원본 테스트를 그대로 돌려 로그를 확인해보니"비동기 분석 요청 시작"로그가 전혀 찍히지 않아, 이 테스트에서는 gRPC 호출도 그로 인한 부수효과도 실제로 발생하지 않음을 실측으로 확인했습니다.
목 처리 대신 이 근거(afterCommit 미발동 실측)를 테스트 주석으로 남겼습니다 — 810f073.
…컨텍스트 - ExerciseAnalysisService: self.sendAnalysisRequestToFastApi 호출을 세션 INSERT 커밋 이후로 지연시켜, 비동기 스레드가 아직 커밋 안 된 세션을 못 찾는 레이스 제거 - SessionServiceTest: endSession 멱등 테스트가 실제로 stopAnalysis 호출 여부를 검증하지 않던 공백 수정 (afterCommit 시뮬레이션 후 호출 횟수 단언 추가) - MemberDeletionCascadeIntegrationTest: DailyLog cascade 커버리지 누락 보완 - GlobalExceptionHandler: 4개 핸들러의 응답 생성 중복을 buildResponse 헬퍼로 정리 - PoseDataCleanupServiceTest: tearDown()의 @transactional이 조용히 무시되던 문제 수정 — Spring의 TransactionalTestExecutionListener는 @test 메서드만 트랜잭션으로 감싸고 JUnit이 직접 호출하는 @AfterEach 콜백은 감싸지 않아 @Modifying 벌크 삭제가 TransactionRequiredException을 던졌음. TransactionTemplate으로 명시적 트랜잭션 사용 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java (1)
117-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win고정
Thread.sleep으로 비동기 테스트를 동기화하지 마세요.현재 null/빈 리스트는 repository를 호출하지 않는 no-op 경로이므로 sleep 없이 상태를 바로 검증해도 됩니다. 비동기 디스패치 완료 자체를 검증하려는 목적이라면 고정 지연 대신 latch나 제어 가능한 executor 같은 명시적 동기화 수단을 사용해야 합니다.
수정 예시
-void cleanupBySessionIds_emptyOrNull_noop() throws InterruptedException { +void cleanupBySessionIds_emptyOrNull_noop() { cleanupService.cleanupBySessionIds(List.of()); cleanupService.cleanupBySessionIds(null); - Thread.sleep(200); assertThat(poseDataRepository.findFramesBySessionId(targetSession.getId())).hasSize(1);As per path instructions,
backend/**/*.java는 동시성 동작을 중점적으로 검토해야 하므로 고정 시간 대기보다 결정적인 동기화 방식을 사용해야 합니다.🤖 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 `@backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java` around lines 117 - 120, Remove the fixed Thread.sleep from cleanupBySessionIds_emptyOrNull_noop and verify the no-op behavior immediately without waiting, since null and empty inputs must not call the repository. If asynchronous dispatch completion is explicitly tested elsewhere, replace time-based waiting there with a deterministic latch or controllable executor.Source: Path instructions
🤖 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.
Inline comments:
In
`@backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java`:
- Around line 96-100: Update the teardown around the TransactionTemplate
callback in PoseDataCleanupServiceTest to tolerate partial setup failure:
collect non-null IDs from targetSession and otherSession before deleting pose
data and sessions, and conditionally delete exerciseId and memberId only when
initialized. Preserve the existing cleanup order and transaction boundary while
preventing cleanup exceptions from masking the original setup failure.
---
Nitpick comments:
In
`@backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java`:
- Around line 117-120: Remove the fixed Thread.sleep from
cleanupBySessionIds_emptyOrNull_noop and verify the no-op behavior immediately
without waiting, since null and empty inputs must not call the repository. If
asynchronous dispatch completion is explicitly tested elsewhere, replace
time-based waiting there with a deterministic latch or controllable executor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fca8d8ec-8eba-4fbb-a43c-451855dd9584
📒 Files selected for processing (5)
backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.javabackend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.javabackend/src/test/java/com/shadowfit/integration/MemberDeletionCascadeIntegrationTest.javabackend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.javabackend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/main/java/com/shadowfit/service/Exercise/ExerciseAnalysisService.java
- backend/src/test/java/com/shadowfit/integration/MemberDeletionCascadeIntegrationTest.java
- backend/src/main/java/com/shadowfit/global/error/GlobalExceptionHandler.java
- backend/src/test/java/com/shadowfit/service/Exercise/SessionServiceTest.java
Comment on lines
+96
to
+100
| new TransactionTemplate(transactionManager).executeWithoutResult(status -> { | ||
| poseDataRepository.deleteBySessionIdIn(List.of(targetSession.getId(), otherSession.getId())); | ||
| sessionRepository.deleteAllById(List.of(targetSession.getId(), otherSession.getId())); | ||
| exercisesRepository.deleteById(exerciseId); | ||
| memberRepository.deleteById(memberId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
teardown을 부분 setup 실패에도 안전하게 만드세요.
@BeforeEach가 중간에 실패해도 @AfterEach는 실행될 수 있습니다. 이때 targetSession 또는 otherSession이 null이면 현재 cleanup이 NPE를 발생시켜 원래 실패 원인을 가립니다. 세션 ID를 null-safe하게 수집하고, exerciseId와 memberId도 조건부로 삭제하세요.
수정 예시
new TransactionTemplate(transactionManager).executeWithoutResult(status -> {
- poseDataRepository.deleteBySessionIdIn(List.of(targetSession.getId(), otherSession.getId()));
- sessionRepository.deleteAllById(List.of(targetSession.getId(), otherSession.getId()));
- exercisesRepository.deleteById(exerciseId);
- memberRepository.deleteById(memberId);
+ List<Long> sessionIds = new java.util.ArrayList<>();
+ if (targetSession != null && targetSession.getId() != null) {
+ sessionIds.add(targetSession.getId());
+ }
+ if (otherSession != null && otherSession.getId() != null) {
+ sessionIds.add(otherSession.getId());
+ }
+ if (!sessionIds.isEmpty()) {
+ poseDataRepository.deleteBySessionIdIn(sessionIds);
+ sessionRepository.deleteAllById(sessionIds);
+ }
+ if (exerciseId != null) {
+ exercisesRepository.deleteById(exerciseId);
+ }
+ if (memberId != null) {
+ memberRepository.deleteById(memberId);
+ }
});As per path instructions, backend/**/*.java는 트랜잭션 경계와 예외 처리를 중점적으로 검토해야 하므로 teardown도 부분 실패 경로를 안전하게 처리해야 합니다.
📝 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
| new TransactionTemplate(transactionManager).executeWithoutResult(status -> { | |
| poseDataRepository.deleteBySessionIdIn(List.of(targetSession.getId(), otherSession.getId())); | |
| sessionRepository.deleteAllById(List.of(targetSession.getId(), otherSession.getId())); | |
| exercisesRepository.deleteById(exerciseId); | |
| memberRepository.deleteById(memberId); | |
| new TransactionTemplate(transactionManager).executeWithoutResult(status -> { | |
| List<Long> sessionIds = new java.util.ArrayList<>(); | |
| if (targetSession != null && targetSession.getId() != null) { | |
| sessionIds.add(targetSession.getId()); | |
| } | |
| if (otherSession != null && otherSession.getId() != null) { | |
| sessionIds.add(otherSession.getId()); | |
| } | |
| if (!sessionIds.isEmpty()) { | |
| poseDataRepository.deleteBySessionIdIn(sessionIds); | |
| sessionRepository.deleteAllById(sessionIds); | |
| } | |
| if (exerciseId != null) { | |
| exercisesRepository.deleteById(exerciseId); | |
| } | |
| if (memberId != null) { | |
| memberRepository.deleteById(memberId); | |
| } |
🤖 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
`@backend/src/test/java/com/shadowfit/service/Exercise/PoseDataCleanupServiceTest.java`
around lines 96 - 100, Update the teardown around the TransactionTemplate
callback in PoseDataCleanupServiceTest to tolerate partial setup failure:
collect non-null IDs from targetSession and otherSession before deleting pose
data and sessions, and conditionally delete exerciseId and memberId only when
initialized. Preserve the existing cleanup order and transaction boundary while
preventing cleanup exceptions from masking the original setup failure.
Source: Path instructions
- tearDown(): setUp() 중간 실패 시 targetSession/otherSession/exerciseId/memberId가 null일 수 있는데 그대로 삭제 호출하면 NPE로 원래 setup 실패 원인이 가려짐 — null 아닌 것만 골라 정리하도록 방어 - cleanupBySessionIds_emptyOrNull_noop: null/빈 리스트는 repository 호출 자체가 없는 no-op 경로라 Thread.sleep(200) 불필요 — 제거 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
startAnalysis_success_createsSessionSynchronously에서 self.sendAnalysisRequestToFastApi 목 처리를 시도했으나 @async 메서드에 Mockito spy를 씌우는 조합이 깨져 불가능했음. 대신 이 테스트가 @transactional rollback 정책상 실제 커밋이 없어 afterCommit 콜백(비동기 gRPC 호출의 트리거)이 애초에 발동하지 않는다는 사실을 로그로 실측 확인 — 목 처리 대상 자체가 없어 주석으로 근거만 남김. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
테스트 파일 8개 → 25개로 확장. 커버 영역: 보안(JwtUtil/JwtAuthFilter/CustomUserDetailsService), 회원(MemberService/OnboardingService/PreferenceService), 세션/피드백/포즈데이터 관련 서비스 전부, 컨트롤러 계층(MockMvc + 실제 보안 필터체인), 관리자 권한(@PreAuthorize) 경계.
이 과정에서 발견·수정한 진짜 버그 8건
Report/SessionFeedbackLog의@OnDelete누락 — 테스트 스키마(H2)에서 회원/세션 삭제 cascade가 실제 schema.sql과 다르게 동작Session.member/DailyLog.member의@OnDelete누락 — 같은 클래스의 버그, 회원탈퇴 CASCADE 테스트로 발견ReportService의 "이전 세션과 비교"가 자기 자신과 비교되던 버그 (findFirst...OrderByStartTimeDesc가 현재 세션을 제외 안 함)WorstSectionCalculator가 모든 3프레임 구간에 null syncRate가 껴있으면 터무니없는 worst 구간을 반환하던 버그PoseDataPartitionScheduler.parsePartition이 잘못된 월(p2026_13등) 파티션명에 크래시하던 버그JwtUtil.isValidToken—io.jsonwebtoken.security.SecurityExceptionimport 누락으로 서명 변조 토큰이 예외를 그대로 던지던 버그@PreAuthorize권한 없음이 403이 아니라 500으로 나가던 버그 —GlobalExceptionHandler의 catch-all이 Spring Security의AccessDeniedException을 가로채서 발생. 관리자 API 전체(AdminExerciseController,ExercisesController.extractReference)에 영향ExerciseAnalysisService.startAnalysis의@Asyncself-invocation 버그 —this.sendAnalysisRequestToFastApi(...)로 자기호출해서@Async가 Spring 프록시를 못 타고 조용히 동기 실행됨.POST /exercises/sessions가 "즉시 세션ID 반환" 설계 의도와 달리 FastAPI gRPC 왕복(최대 5초 데드라인)을 전부 기다렸을 가능성 —self.로 수정(같은 클래스의 기존 self 패턴과 통일)Test plan
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit