Conversation
- coroutineScope와 async를 사용하여 Web/AI 서버 삭제 요청 병렬 처리 - 하드코딩된 Dispatchers.IO를 @iodispatcher 주입 방식으로 변경하여 테스트 용이성 확보 - runCatching을 통한 통합 예외 처리로 서버 실패 시 로컬 DB 무결성 보장
📝 WalkthroughWalkthroughIntroduces Dagger/Hilt integration for coroutine dispatchers via a new IoDispatcher qualifier, refactors PersonRepositoryImpl to inject and use the dispatcher while parallelizing concurrent remote and AI deletions, adds comprehensive unit tests for deletion scenarios, and updates UI strings for deletion dialogs and error messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PersonRepositoryImpl
participant WebService
participant AIService
participant PersonDao
Client->>PersonRepositoryImpl: deleteAnalyzedPerson(personId)
rect rgb(200, 220, 240)
Note over WebService,AIService: Concurrent Operations
par WebService Deletion
PersonRepositoryImpl->>WebService: deleteEntity(personId)
WebService-->>PersonRepositoryImpl: success/exception
and AIService Deletion
PersonRepositoryImpl->>AIService: deletePerson(personId)
AIService-->>PersonRepositoryImpl: success/exception
end
end
alt All Remote Operations Succeed
PersonRepositoryImpl->>PersonDao: deletePersonById(personId)
PersonDao-->>PersonRepositoryImpl: completed
PersonRepositoryImpl-->>Client: Result.success()
else Any Remote Operation Fails
rect rgb(255, 220, 220)
Note over PersonRepositoryImpl: Error Handling
end
PersonRepositoryImpl-->>Client: Result.failure(exception)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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. ✨ 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. 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: 1
🤖 Fix all issues with AI agents
In
@core/data/impl/src/test/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImplTest.kt:
- Around line 41-53: The test opens Mockito annotations in setUp via
MockitoAnnotations.openMocks(this) but never closes the returned AutoCloseable;
store the returned value (e.g., a field named mocks or openMocks) when calling
MockitoAnnotations.openMocks(this) inside PersonRepositoryImplTest.setUp and add
an @AfterEach tearDown method that calls mocks.close() (or safely closes it) to
prevent resource leaks.
🧹 Nitpick comments (2)
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImpl.kt (1)
102-105: Inconsistent dispatcher usage.
getPersonByIdstill usesDispatchers.IOdirectly instead of the injectedioDispatcher. For consistency and testability, consider using the injected dispatcher here as well.♻️ Suggested fix
override fun getPersonById(personId: Long): Flow<PersonModel?> = personDao.getPersonWithFacesFlow(personId) .map { it?.toModel(emptyMap()) } - .flowOn(Dispatchers.IO) + .flowOn(ioDispatcher)core/data/impl/src/test/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImplTest.kt (1)
71-86: Consider adding a test for WebService failure.The current tests cover AI service failure, but adding a similar test for
webService.deleteEntityfailure would provide symmetric coverage for both concurrent operations.📝 Example test case
@Test fun `webService 삭제 실패 시 로컬 DB 삭제를 호출하지 않는다`() = runTest(testDispatcher) { // Given whenever(mockDbRepo.getPersistentDeviceDatabaseName()).thenReturn("db_name") whenever(mockWebService.deleteEntity(any())).thenThrow(RuntimeException("Web Server Error")) // When val result = repository.deleteAnalyzedPerson(fakePerson) advanceUntilIdle() // Then assertTrue(result.isFailure) verify(mockDao, never()).deletePersonById(fakePerson.id) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/di/CoroutineModule.ktcore/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImpl.ktcore/data/impl/src/test/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImplTest.ktfeature/person/src/main/res/values/strings.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci-build
🔇 Additional comments (5)
feature/person/src/main/res/values/strings.xml (1)
5-9: LGTM! Clear and user-friendly messages.The updated dialog title and error message are more descriptive. The emoji in the toast message adds visual emphasis, though ensure it renders correctly across all supported Android versions.
core/data/impl/src/main/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImpl.kt (2)
166-178: Verify the name field usage for each service.The code uses
person.inputNameforwebService.deleteEntity(line 168) butperson.nameforaiService.deletePerson(line 173). Ensure this difference is intentional based on each service's API contract.
203-227: LGTM!Good use of the injected
ioDispatcherfor IO-bound operations, maintaining consistency with the rest of the refactored code.core/data/impl/src/main/java/com/example/metasearch/core/data/impl/di/CoroutineModule.kt (1)
12-24: LGTM! Standard Hilt pattern for dispatcher injection.This enables testability by allowing test dispatchers to be injected. Consider adding
DefaultDispatcherandMainDispatcherqualifiers in the future if other parts of the codebase need injectable dispatchers.core/data/impl/src/test/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImplTest.kt (1)
55-69: LGTM! Well-structured test for success path.Good verification of both remote service calls and local database deletion. The use of
advanceUntilIdle()correctly handles the async operations.
| @BeforeEach | ||
| fun setUp() { | ||
| MockitoAnnotations.openMocks(this) | ||
| // 생성자 주입을 통해 테스트 디스패처 전달 | ||
| repository = PersonRepositoryImpl( | ||
| personDao = mockDao, | ||
| aiService = mockAiService, | ||
| webService = mockWebService, | ||
| databaseNameRepository = mockDbRepo, | ||
| ioDispatcher = testDispatcher, | ||
| context = mockContext, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Close MockitoAnnotations to prevent resource leaks.
MockitoAnnotations.openMocks() returns an AutoCloseable that should be closed after each test to avoid resource leaks.
♻️ Suggested fix
+import org.junit.jupiter.api.AfterEach
+
@OptIn(ExperimentalCoroutinesApi::class)
class PersonRepositoryImplTest {
@Mock private lateinit var mockDao: PersonDao
// ... other mocks ...
private val testDispatcher = StandardTestDispatcher()
private lateinit var repository: PersonRepositoryImpl
+ private lateinit var closeable: AutoCloseable
private val fakePerson = PersonModel(id = 1L, name = "test_uuid", inputName = "홍길동")
@BeforeEach
fun setUp() {
- MockitoAnnotations.openMocks(this)
+ closeable = MockitoAnnotations.openMocks(this)
repository = PersonRepositoryImpl(
// ...
)
}
+
+ @AfterEach
+ fun tearDown() {
+ closeable.close()
+ }📝 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.
| @BeforeEach | |
| fun setUp() { | |
| MockitoAnnotations.openMocks(this) | |
| // 생성자 주입을 통해 테스트 디스패처 전달 | |
| repository = PersonRepositoryImpl( | |
| personDao = mockDao, | |
| aiService = mockAiService, | |
| webService = mockWebService, | |
| databaseNameRepository = mockDbRepo, | |
| ioDispatcher = testDispatcher, | |
| context = mockContext, | |
| ) | |
| } | |
| private lateinit var closeable: AutoCloseable | |
| private val fakePerson = PersonModel(id = 1L, name = "test_uuid", inputName = "홍길동") | |
| @BeforeEach | |
| fun setUp() { | |
| closeable = MockitoAnnotations.openMocks(this) | |
| // 생성자 주입을 통해 테스트 디스패처 전달 | |
| repository = PersonRepositoryImpl( | |
| personDao = mockDao, | |
| aiService = mockAiService, | |
| webService = mockWebService, | |
| databaseNameRepository = mockDbRepo, | |
| ioDispatcher = testDispatcher, | |
| context = mockContext, | |
| ) | |
| } | |
| @AfterEach | |
| fun tearDown() { | |
| closeable.close() | |
| } |
🤖 Prompt for AI Agents
In
@core/data/impl/src/test/java/com/example/metasearch/core/data/impl/repository/PersonRepositoryImplTest.kt
around lines 41 - 53, The test opens Mockito annotations in setUp via
MockitoAnnotations.openMocks(this) but never closes the returned AutoCloseable;
store the returned value (e.g., a field named mocks or openMocks) when calling
MockitoAnnotations.openMocks(this) inside PersonRepositoryImplTest.setUp and add
an @AfterEach tearDown method that calls mocks.close() (or safely closes it) to
prevent resource leaks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
Test
삭제 실패 토스트 메시지 확인