Test/테스트 코드 보완#14
Hidden character warning
Conversation
There was a problem hiding this comment.
Pull request overview
이 PR은 도메인 로직에 대한 테스트 코드를 보완하고, Kotlin 테스트 코드 작성을 위한 인프라를 구축합니다.
Changes:
PushSubscription및UpcomingScheduleNotification도메인 객체에 대한 단위 테스트 추가- 테스트 객체 생성을 위한 유틸리티 함수 작성 (reflection 기반)
- Kotlin 플러그인 및 의존성 추가, Kotlin 컴파일 태스크 의존성 설정
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/me/pinitnotification/domain/push/PushSubscriptionUtils.kt | PushSubscription 테스트 객체 생성을 위한 헬퍼 함수 추가 |
| src/test/java/me/pinitnotification/domain/push/PushSubscriptionTest.kt | PushSubscription.updateToken() 메서드에 대한 단위 테스트 추가 |
| src/test/java/me/pinitnotification/domain/notification/UpcomingScheduleNotificationUtils.kt | UpcomingScheduleNotification 테스트 객체 생성을 위한 헬퍼 함수 추가 |
| src/test/java/me/pinitnotification/domain/notification/UpcomingScheduleNotificationTest.kt | UpcomingScheduleNotification의 updateScheduleStartTime()과 isDue() 메서드에 대한 단위 테스트 추가 |
| build.gradle | Kotlin 플러그인, 의존성, 그리고 Kotlin 컴파일 태스크 설정 추가 |
| ReflectionUtils.findFields( | ||
| PushSubscription::class.java, | ||
| { field -> field.name == "id" }, | ||
| ReflectionUtils.HierarchyTraversalMode.TOP_DOWN | ||
| ).forEach { field -> | ||
| field.isAccessible = true | ||
| field.set(sample, id) | ||
| } |
There was a problem hiding this comment.
문제점: field.isAccessible = true 사용은 Java 9 이상에서 deprecated되었으며, reflection을 통한 private 필드 접근은 캡슐화를 깨뜨립니다.
영향: 향후 Java 버전 업그레이드 시 호환성 문제가 발생할 수 있으며, 테스트가 내부 구현에 지나치게 의존하게 됩니다.
수정 제안: 테스트를 위한 builder 패턴이나 factory 메서드를 도메인 객체에 추가하거나, @PersistenceConstructor 같은 JPA 관련 애노테이션을 활용하여 reflection 사용을 최소화하세요.
| ReflectionUtils.findFields( | ||
| PushSubscription::class.java, | ||
| { field -> field.name == "modifiedAt" }, | ||
| ReflectionUtils.HierarchyTraversalMode.TOP_DOWN | ||
| ).forEach { field -> | ||
| field.isAccessible = true | ||
| field.set(sample, modifiedAt) | ||
| } |
There was a problem hiding this comment.
문제점: field.isAccessible = true 사용은 Java 9 이상에서 deprecated되었으며, reflection을 통한 private 필드 접근은 캡슐화를 깨뜨립니다.
영향: 향후 Java 버전 업그레이드 시 호환성 문제가 발생할 수 있으며, 테스트가 내부 구현에 지나치게 의존하게 됩니다.
수정 제안: 테스트를 위한 builder 패턴이나 factory 메서드를 도메인 객체에 추가하거나, @PersistenceConstructor 같은 JPA 관련 애노테이션을 활용하여 reflection 사용을 최소화하세요.
| ReflectionUtils.findFields( | ||
| UpcomingScheduleNotification::class.java, | ||
| { field -> field.name == "id" }, | ||
| ReflectionUtils.HierarchyTraversalMode.TOP_DOWN | ||
| ).forEach { field -> | ||
| field.isAccessible = true | ||
| field.set(sample, id) | ||
| } |
There was a problem hiding this comment.
문제점: field.isAccessible = true 사용은 Java 9 이상에서 deprecated되었으며, reflection을 통한 private 필드 접근은 캡슐화를 깨뜨립니다.
영향: 향후 Java 버전 업그레이드 시 호환성 문제가 발생할 수 있으며, 테스트가 내부 구현에 지나치게 의존하게 됩니다.
수정 제안: 테스트를 위한 builder 패턴이나 factory 메서드를 도메인 객체에 추가하거나, @PersistenceConstructor 같은 JPA 관련 애노테이션을 활용하여 reflection 사용을 최소화하세요.
| @Test | ||
| fun updateScheduleStartTime() { | ||
| //given | ||
| val notification = getSampleUpcomingScheduleNotification() | ||
|
|
||
| //when | ||
| notification.updateScheduleStartTime("2024-06-01T10:00:00Z", "123-idempotency-key") | ||
|
|
||
| //then | ||
| Assertions.assertThat(notification.scheduleStartTime).isEqualTo("2024-06-01T10:00:00Z") | ||
| Assertions.assertThat(notification.idempotentKey).isEqualTo("123-idempotency-key") | ||
| } |
There was a problem hiding this comment.
문제점: 테스트가 updateScheduleStartTime 메서드를 호출하지만, 실제로는 두 개의 필드(scheduleStartTime과 idempotentKey)를 함께 업데이트하고 있습니다. 테스트 메서드명이 메서드의 전체 동작을 충분히 설명하지 못합니다.
영향: 테스트의 의도가 명확하지 않아 향후 유지보수 시 혼란을 줄 수 있습니다.
수정 제안: 테스트 메서드명을 updateScheduleStartTimeAndIdempotencyKey처럼 더 구체적으로 변경하거나, 각 필드 업데이트를 개별적으로 테스트하는 것을 고려하세요.
| import java.time.Clock | ||
| import java.time.LocalDateTime | ||
| import java.time.ZoneId | ||
| import java.time.ZoneOffset.UTC | ||
|
|
||
| internal class PushSubscriptionTest { | ||
| val clock = Clock.fixed( | ||
| LocalDateTime.of( | ||
| 2026, | ||
| 1, | ||
| 1, | ||
| 0, | ||
| 0, | ||
| 0 | ||
| ).toInstant(UTC), ZoneId.systemDefault() | ||
| ) |
There was a problem hiding this comment.
문제점: clock 변수가 선언되어 있지만 테스트 메서드에서 실제로 사용되지 않습니다.
영향: 불필요한 코드가 테스트 파일에 포함되어 있어 코드 가독성이 저하되고, 미래의 개발자에게 혼란을 줄 수 있습니다.
수정 제안: 사용되지 않는 clock 변수 선언부(11-20라인)를 제거하세요.
| import java.time.Clock | |
| import java.time.LocalDateTime | |
| import java.time.ZoneId | |
| import java.time.ZoneOffset.UTC | |
| internal class PushSubscriptionTest { | |
| val clock = Clock.fixed( | |
| LocalDateTime.of( | |
| 2026, | |
| 1, | |
| 1, | |
| 0, | |
| 0, | |
| 0 | |
| ).toInstant(UTC), ZoneId.systemDefault() | |
| ) | |
| internal class PushSubscriptionTest { |
| @Test | ||
| fun updateToken() { | ||
| //given | ||
| val subscription = getSamplePushSubscription() | ||
|
|
||
| //when | ||
| subscription.updateToken("new-sample-token") | ||
|
|
||
| //then | ||
| Assertions.assertThat(subscription.token).isEqualTo("new-sample-token") | ||
| } |
There was a problem hiding this comment.
문제점: PushSubscription.updateToken() 메서드는 modifiedAt이 현재 시간보다 이후일 경우 토큰 업데이트를 건너뛰는 로직이 있지만, 이 테스트는 해당 조건 분기를 검증하지 않고 있습니다.
영향: 중요한 비즈니스 로직(시간 기반 조건부 업데이트)이 테스트되지 않아, 향후 코드 변경 시 버그가 발생할 수 있습니다.
수정 제안: modifiedAt이 미래 시간으로 설정된 경우 토큰이 업데이트되지 않는지 확인하는 추가 테스트 케이스를 작성하세요.
| @Test | ||
| fun isDue() { | ||
| //given | ||
| val notification = getSampleUpcomingScheduleNotification( | ||
| scheduleStartTime = "2024-06-01T10:00:00Z" | ||
| ) | ||
|
|
||
| //when | ||
| val isDue = notification.isDue(OffsetDateTime.now(clock)) | ||
|
|
||
| //then | ||
| Assertions.assertThat(isDue).isTrue() | ||
| } |
There was a problem hiding this comment.
문제점: 2026년 1월 1일로 고정된 Clock을 사용하고 있으나, 실제 테스트에서는 2024년 6월 1일의 스케줄 시작 시간과 비교하고 있습니다. 이로 인해 테스트의 의도가 명확하지 않습니다.
영향: 테스트가 과거 날짜로 인해 항상 true를 반환하는 경우만 검증하고 있어, isDue 메서드의 경계 조건이나 false 케이스가 테스트되지 않습니다.
수정 제안: 테스트 시나리오를 명확히 하고, 스케줄 시작 시간이 현재 시간보다 이전인 경우(true)와 이후인 경우(false) 모두를 검증하는 테스트 케이스를 추가하세요.
변경된 점