Issue: (#147) 댓글 엔티티 리팩토링 및 테스트 코드 작성 #152
Conversation
|
""" Walkthrough이 변경 사항은 테스트 코드에서 Mockito를 MockK로 전환하고, Kotest를 테스트 프레임워크로 도입하며, CommentService의 메서드 시그니처를 ID 기반으로 리팩토링합니다. 또한 SpringContextHolder의 context 프로퍼티를 외부에서 접근 가능하도록 변경하였고, 테스트 코드의 구조와 커버리지를 확장했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Facade as ReadLetterFacade
participant Service as CommentService
participant Repo as CommentRepository
Facade->>Service: findAllByLetterId(letter.id)
Service->>Repo: findAllByLetterId(letter.id)
Repo-->>Service: List<Comment>
Service-->>Facade: List<Comment>
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/test/kotlin/gomushin/backend/schedule/domain/service/ScheduleServiceTest.kt (1)
66-66:⚠️ Potential issue잘못된 verification 호출을 수정하세요.
현재
scheduleService.save(any())를 verify하고 있는데, 이는 테스트 대상인 서비스 자체의 메서드를 검증하는 것입니다. 단위 테스트에서는 의존성 호출을 검증해야 합니다.다음과 같이 수정하세요:
- verify { scheduleService.save(any()) } + verify { scheduleRepository.save(any()) }
🧹 Nitpick comments (2)
src/main/kotlin/gomushin/backend/core/common/support/SpringContextHolder.kt (1)
9-9: 캡슐화를 고려한 대안 접근 방법 검토 필요
context프로퍼티의 가시성을 public으로 변경하는 것은 테스트 목적으로는 유용하지만, 캡슐화 원칙을 위반합니다.다음과 같은 대안을 고려해보세요:
- 테스트용 별도 설정 메서드 제공
- 테스트 전용 인터페이스 분리
@TestOnly어노테이션 추가현재 변경사항이 테스트 요구사항에 맞다면 최소한 문서화를 통해 의도를 명확히 하세요:
+/** + * ApplicationContext instance. + * Note: Public visibility is required for test mocking purposes. + */ lateinit var context: ApplicationContextsrc/test/kotlin/gomushin/backend/schedule/domain/service/ScheduleServiceTest.kt (1)
45-49: SpringContextHolder 설정 패턴의 중복을 개선할 수 있습니다.동일한 SpringContextHolder 설정 코드가 여러 테스트 클래스에서 반복되고 있습니다. 공통 base 클래스나 utility 함수로 추출하는 것을 고려해보세요.
예시:
abstract class BaseServiceTest { @MockK(relaxed = true) protected lateinit var mockAppEnv: AppEnv @MockK protected lateinit var mockApplicationContext: ApplicationContext @BeforeEach fun baseSetup() { SpringContextHolder.context = mockApplicationContext every { mockApplicationContext.getBean(AppEnv::class.java) } returns mockAppEnv every { mockAppEnv.getId() } returns "test-env" } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle.kts(2 hunks)src/main/kotlin/gomushin/backend/core/common/support/SpringContextHolder.kt(1 hunks)src/main/kotlin/gomushin/backend/schedule/domain/service/CommentService.kt(1 hunks)src/main/kotlin/gomushin/backend/schedule/facade/ReadLetterFacade.kt(1 hunks)src/test/kotlin/gomushin/backend/schedule/domain/facade/UpsertAndDeleteCommentFacadeTest.kt(2 hunks)src/test/kotlin/gomushin/backend/schedule/domain/service/CommentServiceTest.kt(1 hunks)src/test/kotlin/gomushin/backend/schedule/domain/service/ScheduleServiceTest.kt(2 hunks)
🔇 Additional comments (13)
src/main/kotlin/gomushin/backend/schedule/domain/service/CommentService.kt (2)
42-44: 훌륭한 리팩토링입니다!메서드 시그니처를
Letter객체에서LongID로 변경한 것은 다음과 같은 이점을 제공합니다:
- 결합도 감소: 전체 Letter 객체 대신 필요한 ID만 전달
- 성능 향상: 불필요한 객체 데이터 전달 제거
- 의도 명확화: 실제로 필요한 데이터가 무엇인지 명시
42-44: 메서드 시그니처 리팩토링이 잘 되었습니다.
findAllByLetter에서findAllByLetterId로 메서드명을 변경하고 파라미터를 Letter 객체에서 Long ID로 단순화한 것은 좋은 리팩토링입니다. 이를 통해:
- Comment와 Letter 엔티티 간의 결합도가 감소
- API가 더 명확하고 간단해짐
- 불필요한 객체 전달을 피해 성능 개선
ReadLetterFacade에서도 호출 부분이 적절히 업데이트된 것을 확인했습니다.
build.gradle.kts (1)
15-15: Kotest 의존성 추가가 적절합니다테스트 프레임워크 현대화를 위한 Kotest 의존성 추가가 잘 되었습니다:
- 버전 5.5.4는 안정적인 버전입니다
- 필요한 핵심 의존성(runner, assertions)이 모두 포함되었습니다
- 기존
mockkVersion패턴과 일관성 있게 버전 변수를 사용했습니다Also applies to: 94-95
src/main/kotlin/gomushin/backend/schedule/facade/ReadLetterFacade.kt (2)
63-63: CommentService 리팩토링과 일치하는 올바른 업데이트
commentService.findAllByLetterId(letter.id)호출로 변경한 것이 서비스 레이어의 메서드 시그니처 변경과 완벽하게 일치합니다. 기능적으로 동일하면서 더 효율적인 구현입니다.
63-63: 서비스 메서드 변경에 맞춘 호출 부분 업데이트가 정확합니다.CommentService의 메서드 시그니처 변경에 맞춰
findAllByLetter(letter)에서findAllByLetterId(letter.id)로 호출을 변경한 것이 적절합니다. 기능적으로는 동일하면서도 리팩토링된 서비스 API를 올바르게 사용하고 있습니다.src/test/kotlin/gomushin/backend/schedule/domain/service/ScheduleServiceTest.kt (1)
9-26: MockK와 Kotest 마이그레이션이 잘 적용되었습니다.Mockito에서 MockK로, JUnit assertion에서 Kotest assertion으로의 전환이 일관성 있게 잘 적용되었습니다.
src/test/kotlin/gomushin/backend/schedule/domain/service/CommentServiceTest.kt (2)
47-92: 테스트 구조화가 잘 되어있습니다.UpsertTest 중첩 클래스로 생성과 수정 관련 테스트들이 잘 그룹화되어 있고, 각 테스트의 목적이 명확합니다.
94-170: 포괄적인 읽기 테스트 커버리지가 좋습니다.getById, findById, findAllByLetterId 메서드들에 대한 성공/실패 케이스가 모두 잘 커버되어 있습니다.
src/test/kotlin/gomushin/backend/schedule/domain/facade/UpsertAndDeleteCommentFacadeTest.kt (4)
30-56: MockK 마이그레이션과 설정이 잘 적용되었습니다.Mockito에서 MockK로의 전환이 깔끔하게 이루어졌고, SpringContextHolder 설정도 적절합니다.
87-175: 삭제 테스트 커버리지가 우수합니다.DeleteTest 중첩 클래스로 삭제 관련 테스트들이 잘 그룹화되어 있고, 성공 케이스와 함께 권한 검증, 데이터 일관성 검증 등의 에러 케이스들도 모두 잘 커버되어 있습니다.
118-145: 예외 메시지 검증이 정확합니다.권한 오류 상황에서의 예외 메시지 검증이 정확하게 이루어지고 있습니다. Kotest의
shouldThrow와shouldBe사용도 적절합니다.
147-174: 데이터 일관성 검증 테스트가 잘 작성되었습니다.댓글의 letterId와 입력받은 letterId 불일치에 대한 검증 로직이 잘 테스트되어 있습니다. 이런 엣지 케이스들을 테스트하는 것은 매우 중요합니다.
src/main/kotlin/gomushin/backend/core/common/support/SpringContextHolder.kt (1)
9-9: 테스트를 위한 가시성 변경이 적절합니다.
context프로퍼티의 가시성을 변경하여 테스트 클래스에서 ApplicationContext를 모킹할 수 있도록 한 것은 Kotest 프레임워크 도입과 테스트 코드 작성 목표에 부합합니다. 다만 캡슐화 원칙을 일부 위반하므로, 프로덕션 코드에서 이 프로퍼티에 직접 접근하지 않도록 주의해야 합니다.
src/test/kotlin/gomushin/backend/schedule/domain/service/CommentServiceTest.kt
Outdated
Show resolved
Hide resolved
| // then | ||
| result shouldBe comment | ||
| verify { commentRepository.findByIdOrNull(id) } | ||
| verify { commentService.findById(id) } |
There was a problem hiding this comment.
불필요한 verification 호출을 제거하세요.
commentService.findById(id)를 verify하는 것은 테스트 대상인 getById 메서드에서 내부적으로 호출하는 메서드를 검증하는 것입니다. 이는 구현 세부사항을 테스트하는 것으로 부적절합니다.
다음과 같이 수정하세요:
result shouldBe comment
verify { commentRepository.findByIdOrNull(id) }
- verify { commentService.findById(id) }📝 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.
| verify { commentService.findById(id) } | |
| result shouldBe comment | |
| verify { commentRepository.findByIdOrNull(id) } |
🤖 Prompt for AI Agents
In
src/test/kotlin/gomushin/backend/schedule/domain/service/CommentServiceTest.kt
at line 121, remove the verification call for commentService.findById(id)
because verifying internal method calls of the method under test exposes
implementation details. Simply delete the verify { commentService.findById(id) }
line to fix this.
kimyeoungrok
left a comment
kimyeoungrok
left a comment
There was a problem hiding this comment.
굿 수고했어 시험기간이라 미리 못봐서 미안 ㅎㅎ
| fun setup() { | ||
| SpringContextHolder.context = mockApplicationContext | ||
| every { mockApplicationContext.getBean(AppEnv::class.java) } returns mockAppEnv | ||
| every { mockAppEnv.getId() } returns "test-env" |
There was a problem hiding this comment.
이거 원래 코드는 yml의 spring.application.name에 있는 값을 가져오는 걸로 알고있는데 지금 test yml보니깐 거기는 "backend"로 되어있던데 "test-env"로 한 이유가 궁금해
There was a problem hiding this comment.
아 test yml 에 있는거 까먹고 그냥 하드코딩했어
사실 이건 중요한거 아니고, lazyInit 예외를 막으려고 넣은거라..
| commentService.upsert(null, letterId, authorId, nickname, upsertCommentRequest) | ||
|
|
||
| // then | ||
| verify { commentService.save(any()) } |
There was a problem hiding this comment.
알아보니깐 verify는 스터빙한 객체의 메서드가 제대로 작동하는지 보는거라네(commentService는 실제 객체) 밑에 coderabbit이 지적한 것처럼 commentService가 아니라 commentRepository가 되어야할듯
✅ PR 유형
어떤 변경 사항이 있었나요?
📝 작업 내용
✏️ 관련 이슈
🎸 기타 사항 or 추가 코멘트
Summary by CodeRabbit
버그 수정
테스트
기타