Refactor/test and coroutines#300
Conversation
Walkthroughデータ層リポジトリと複数のドメインユースケースにCoroutineDispatcherの注入を導入し、テスト用のコルーチンライブラリとINTERNETパーミッションを追加、@localonlyテストアノテーションを新規作成し、ユースケース指向のandroidTestスイートを拡張しました。IDE検査プロファイルにも1エントリ追加。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UseCase
participant Repository
participant Database
participant Dispatcher
Client->>UseCase: ユースケース呼び出し
UseCase->>Dispatcher: withContext(coroutineDispatcher) { ... }
Dispatcher->>Repository: 実IO操作要求
Repository->>Dispatcher: withContext(coroutineDispatcher) { ... }
Repository->>Database: CRUD操作
Database-->>Repository: 結果
Repository-->>UseCase: 結果返却
UseCase-->>Client: 最終結果返却
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AndroidApp/domain/build.gradle.kts (1)
23-28: テスト依存関係の設定スコープを修正してください。テストライブラリに
implementationを使用すると、本番コードに不要な依存関係が含まれてしまいます。androidTestImplementationまたはtestImplementationを使用してください。🔧 修正案
// test - implementation(libs.junit) - implementation(libs.com.google.truth) - implementation(libs.androidx.test.ext.junit) - implementation(libs.androidx.test.espresso.core) - implementation(libs.kotlinx.corountines.test) + androidTestImplementation(libs.junit) + androidTestImplementation(libs.com.google.truth) + androidTestImplementation(libs.androidx.test.ext.junit) + androidTestImplementation(libs.androidx.test.espresso.core) + androidTestImplementation(libs.kotlinx.corountines.test)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ImportDataUseCaseImpl.kt (1)
24-31:readLine()は1行しか読み込みません。複数行のJSONデータが失われる可能性があります。
reader.readLine()は最初の1行のみを読み込むため、JSONが複数行にわたる場合、データが欠落しサイレントにインポートが失敗します。🐛 修正案: 全行を読み込むように変更
withContext(coroutineDispatcher) { context.contentResolver.openInputStream(uri).use { input -> BufferedReader(InputStreamReader(input)).use { reader -> - sb.append(reader.readLine()) + reader.forEachLine { line -> + sb.append(line) + } } } }または、より簡潔に:
- val sb = StringBuilder() - withContext(coroutineDispatcher) { - context.contentResolver.openInputStream(uri).use { input -> - BufferedReader(InputStreamReader(input)).use { reader -> - sb.append(reader.readLine()) - } - } - } - val json = sb.toString() + val json = withContext(coroutineDispatcher) { + context.contentResolver.openInputStream(uri)?.bufferedReader()?.use { it.readText() } + ?: throw RuntimeException("Failed to open input stream") + }
🤖 Fix all issues with AI agents
In
@AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt:
- Around line 42-44: The outer withContext(coroutineDispatcher) in
NotifyTargetAppNotificationUseCaseImpl is redundant because NotifyUseCaseImpl
already wraps its I/O in withContext(coroutineDispatcher); remove the
surrounding withContext and call notifyUseCase("${title}\n${message}") directly
to avoid a no-op nested dispatcher switch, keeping the existing notifyUseCase
invocation and signature unchanged.
In @AndroidApp/gradle/libs.versions.toml:
- Line 67: The library alias in libs.versions.toml is misspelled: change the
alias from "kotlinx-corountines-test" to "kotlinx-coroutines-test" and keep the
module and version.ref as-is; then update any build script references that
import that alias (e.g., in the module's build.gradle.kts where the old alias is
used) to the corrected alias so the dependency resolution succeeds.
🧹 Nitpick comments (2)
AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/Annotations.kt (1)
1-5: アノテーションの目的を説明するKDocを追加することを検討してください。
@LocalOnlyアノテーションの使用目的が明確ではありません。将来のメンテナンス性のため、このアノテーションをいつ・なぜ使用するかを説明するドキュメントの追加を推奨します。📝 ドキュメント追加の提案
package me.nya_n.notificationnotifier +/** + * ローカル環境でのみ実行すべきテストに付与するアノテーション。 + * CI環境では実行をスキップするために使用します。 + */ @Target(AnnotationTarget.FUNCTION) @Retention(AnnotationRetention.RUNTIME) annotation class LocalOnlyAndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (1)
106-108:LoadAppUseCaseImplへのtestDispatcher注入が不足他のユースケース（
NotifyUseCaseImpl、ExportDataUseCaseImpl、ImportDataUseCaseImpl）にはtestDispatcherを注入していますが、LoadAppUseCaseImplにはデフォルトのDispatchers.IOが使用されています。テストの一貫性のため、testDispatcherを注入することを検討してください。♻️ 修正案
- loadAppUseCase = LoadAppUseCaseImpl(userSettingsRepository, appRepository) + loadAppUseCase = LoadAppUseCaseImpl(userSettingsRepository, appRepository, testDispatcher)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
AndroidApp/.idea/inspectionProfiles/Project_Default.xmlAndroidApp/data/repository/src/main/kotlin/me/nya_n/notificationnotifier/data/repository/impl/AppRepositoryImpl.ktAndroidApp/domain/build.gradle.ktsAndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/Annotations.ktAndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.ktAndroidApp/domain/src/main/AndroidManifest.xmlAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ExportDataUseCaseImpl.ktAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ImportDataUseCaseImpl.ktAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/LoadAppUseCaseImpl.ktAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.ktAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyUseCaseImpl.ktAndroidApp/gradle/libs.versions.toml
🧰 Additional context used
🧬 Code graph analysis (8)
AndroidApp/data/repository/src/main/kotlin/me/nya_n/notificationnotifier/data/repository/impl/AppRepositoryImpl.kt (1)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (1)
AppRepositoryImpl(82-82)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ExportDataUseCaseImpl.kt (2)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (3)
ImportDataUseCaseImpl(95-95)ExportDataUseCaseImpl(94-94)NotifyUseCaseImpl(102-102)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/ExportDataUseCase.kt (1)
invoke(7-9)
AndroidApp/domain/build.gradle.kts (3)
AndroidApp/ui/build.gradle.kts (2)
implementation(17-35)id(1-5)AndroidApp/app/build.gradle.kts (1)
implementation(22-32)AndroidApp/build-logic/build.gradle.kts (1)
compileOnly(5-7)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ImportDataUseCaseImpl.kt (3)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (2)
ImportDataUseCaseImpl(95-95)ExportDataUseCaseImpl(94-94)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/ImportDataUseCase.kt (2)
invoke(7-9)invoke(8-8)AndroidApp/ui/src/main/kotlin/me/nya_n/notificationnotifier/ui/screen/settings/SettingsViewModel.kt (1)
importData(80-93)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyUseCaseImpl.kt (2)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (2)
NotifyUseCaseImpl(102-102)NotifyTargetAppNotificationUseCaseImpl(99-101)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/NotifyUseCase.kt (1)
invoke(4-4)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt (2)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (2)
NotifyUseCaseImpl(102-102)NotifyTargetAppNotificationUseCaseImpl(99-101)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/NotifyUseCase.kt (1)
invoke(4-4)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/LoadAppUseCaseImpl.kt (2)
AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (1)
LoadAppUseCaseImpl(97-97)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/LoadAppUseCase.kt (1)
invoke(6-13)
AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (1)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/AddTargetAppUseCaseImpl.kt (1)
appRepository(7-13)
⏰ 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: build
🔇 Additional comments (14)
AndroidApp/.idea/inspectionProfiles/Project_Default.xml (1)
20-20: LGTM!
ConstPropertyNameインスペクションを無効化する設定は、チームのコーディング規約に基づいた合理的な変更です。AndroidApp/data/repository/src/main/kotlin/me/nya_n/notificationnotifier/data/repository/impl/AppRepositoryImpl.kt (1)
13-17: LGTM!
CoroutineDispatcherの依存性注入パターンは、テスト時にTestDispatcherを注入できるようになり、テスタビリティが向上します。デフォルト値Dispatchers.IOにより、既存のDI設定との後方互換性も維持されています。AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ImportDataUseCaseImpl.kt (1)
17-21: LGTM!
CoroutineDispatcherの依存性注入により、テスト時にディスパッチャーを制御できるようになりました。AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyUseCaseImpl.kt (1)
12-15: LGTM!
CoroutineDispatcherの依存性注入パターンが適切に実装されています。Socket通信などのブロッキングI/O操作に対して、テスト時にディスパッチャーを制御できるようになりました。AndroidApp/domain/src/main/AndroidManifest.xml (1)
2-4: LGTM - テスト用途でのINTERNET権限追加コメントにあるように、テストで通知を送信するために必要な権限です。ドメイン層のマニフェストに追加することで、テストAPKにマージされます。
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ExportDataUseCaseImpl.kt (2)
15-19: LGTM - CoroutineDispatcher DIの導入デフォルト値
Dispatchers.IOにより、既存のプロダクションコード（App.ktで2引数のみ渡している箇所）は変更不要で動作し、テストではカスタムディスパッチャーを注入可能になります。
29-33: LGTM - 注入されたディスパッチャーの使用
withContextで注入されたディスパッチャーを使用することで、テスト時にI/O操作のスレッド制御が可能になります。AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt (1)
11-15: LGTM - 一貫したDIパターン他のユースケース実装と同様のパターンで
CoroutineDispatcherを注入しています。AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/LoadAppUseCaseImpl.kt (2)
15-19: LGTM - CoroutineDispatcher DIの導入他のユースケースと一貫したパターンでディスパッチャーを注入しています。デフォルト値によりプロダクションコードへの影響はありません。
21-28: LGTM - 適切なディスパッチャー制御
invoke関数全体をwithContext(coroutineDispatcher)でラップし、I/O操作（loadInstalledAppList、loadTargetList）を適切なディスパッチャーで実行しています。AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (4)
57-58: LGTM - テスト用ディスパッチャーの設定
UnconfinedTestDispatcherを使用することで、コルーチンのテストが即座に実行され、テストの予測可能性が向上します。
98-105: LGTM - インメモリDBとテストディスパッチャーの設定
isInMemory = trueでインメモリデータベースを使用し、AppRepositoryImplにtestDispatcherを注入することで、テストの独立性と速度を確保しています。
255-268: LGTM - ローカル専用テストの適切なガード
@LocalOnlyアノテーションとassumeTrueによるCI環境変数チェックで、ネットワークアクセスが必要なテストをCI環境から除外しています。IPアドレスをテスト環境に合わせて変更する必要がある点もコメントで明示されており、適切です。
270-314: LGTM - バックアップ・復元テストの包括的なカバレッジターゲットアプリ、フィルタ条件、アドレスを含む完全なバックアップ・復元フローをテストしています。復元後に各データが正しく復元されているかを検証しており、良いテストカバレッジです。
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt:
- Around line 79-84: The setup deletes File(exportFile, ExportFileName) but the
test creates the file with File.createTempFile(ExportFileName, null, exportFile)
which yields names like "export.jsonXXXX"; update the setup to consistently
remove the temp pattern (e.g., delete files in exportFile whose names startWith
ExportFileName) or change the test to create a fixed file named ExportFileName
(remove createTempFile usage), and rename the ExportFileName constant if its
semantics change; apply the same fix for the other occurrence around the second
test.
In
@AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt:
- Line 37: notifyUseCase("${title}\n${message}") returns a Result<Unit> that is
currently ignored, so failures inside its runCatching are silently swallowed;
update the call site (NotifyTargetAppNotificationUseCaseImpl -> notifyUseCase
invocation) to capture and check the returned Result<Unit> and propagate or
throw on failure (e.g., call getOrThrow() or rewrap/return a failed Result) so
that errors from notifyUseCase are not ignored and will surface to the outer
runCatching.
🧹 Nitpick comments (1)
AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (1)
63-64:loadAppUseCaseは具象型で宣言されています。他のユースケースはインターフェース型で宣言されていますが、
loadAppUseCaseのみLoadAppUseCaseImplとして宣言されています。これはloadTargetList()やloadInstalledAppList()などのメソッドがインターフェースに定義されていないため意図的と思われますが、一貫性のためにインターフェースの拡張を検討してください。
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
AndroidApp/domain/build.gradle.ktsAndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.ktAndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.ktAndroidApp/gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- AndroidApp/domain/build.gradle.kts
- AndroidApp/gradle/libs.versions.toml
🧰 Additional context used
🧬 Code graph analysis (2)
AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (2)
AndroidApp/ui/src/main/kotlin/me/nya_n/notificationnotifier/ui/screen/detail/DetailViewModel.kt (1)
loadFilterConditionUseCase(16-67)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/ImportDataUseCaseImpl.kt (1)
appRepository(37-39)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt (3)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyUseCaseImpl.kt (4)
userSettingsRepository(11-35)invoke(18-34)withContext(19-33)writer(28-30)AndroidApp/app/src/main/kotlin/me/nya_n/notificationnotifier/App.kt (2)
NotifyTargetAppNotificationUseCaseImpl(99-101)NotifyUseCaseImpl(102-102)AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/NotifyUseCase.kt (1)
invoke(4-4)
⏰ 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: build
🔇 Additional comments (4)
AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt (1)
8-11: サマリーとコードの不整合を確認してください。AIサマリーでは
coroutineDispatcher: CoroutineDispatcher = Dispatchers.IOパラメータが追加されたと記載されていますが、提供されたコードには含まれていません。他のユースケース（ExportDataUseCaseImpl、ImportDataUseCaseImplなど）ではディスパッチャの注入パターンが適用されているようですが、このファイルでは適用されていません。意図的な設計であれば問題ありませんが、このユースケースにもディスパッチャ注入が必要かご確認ください。
AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt (3)
76-121: テストセットアップの構成は適切です。
UnconfinedTestDispatcherによる同期的なテスト実行- インメモリDBの使用とテスト前のクリア
- ユースケースへのディスパッチャ注入によるテスト可能性の向上
コルーチンテストのベストプラクティスに沿った実装です。
116-116: このコメントは不要です。NotifyUseCaseImplのコンストラクタはuserSettingsRepositoryとcoroutineDispatcher（デフォルト値:Dispatchers.IO）の2つのパラメータを受け取り、テストコードで正しく両方の引数が渡されています。コンパイルエラーは発生しません。Likely an incorrect or invalid review comment.
256-259: 問題ありません。@LocalOnlyアノテーションはUseCaseTest.ktと同じパッケージ（me.nya_n.notificationnotifier）で定義されているため、明示的なインポート文が不要です。Kotlinでは同一パッケージ内の型は自動的にスコープ内にあります。
assumeTrueによるCI環境スキップの実装は適切です。Likely an incorrect or invalid review comment.
| exportFile = appContext.filesDir | ||
| File(exportFile, exportFileName).apply { | ||
| File(exportFile, ExportFileName).apply { | ||
| if (exists()) { | ||
| delete() | ||
| } | ||
| } |
There was a problem hiding this comment.
一時ファイル名がExportFileName定数と一致しません。
File.createTempFile(ExportFileName, null, exportFile)はexport.json<ランダム文字列>のようなファイル名を生成しますが、setUp()内の削除処理はFile(exportFile, ExportFileName)（つまりexport.json）を対象としています。
テスト間での一意性を確保するためであれば現状で問題ありませんが、その場合は削除処理の修正と定数名の変更を検討してください。
💡 修正案
一時ファイルを使う場合、定数名と削除処理を修正:
companion object {
- private const val ExportFileName: String = "export.json"
+ private const val ExportFilePrefix: String = "export"
+ private const val ExportFileSuffix: String = ".json"
}または固定ファイル名を使う場合:
-val uri = Uri.fromFile(File.createTempFile(ExportFileName, null, exportFile))
+val uri = Uri.fromFile(File(exportFile, ExportFileName))Also applies to: 272-272
🤖 Prompt for AI Agents
In
@AndroidApp/domain/src/androidTest/java/me/nya_n/notificationnotifier/UseCaseTest.kt
around lines 79 - 84, The setup deletes File(exportFile, ExportFileName) but the
test creates the file with File.createTempFile(ExportFileName, null, exportFile)
which yields names like "export.jsonXXXX"; update the setup to consistently
remove the temp pattern (e.g., delete files in exportFile whose names startWith
ExportFileName) or change the test to create a fixed file named ExportFileName
(remove createTempFile usage), and rename the ExportFileName constant if its
semantics change; apply the same fix for the other occurrence around the second
test.
| withContext(Dispatchers.IO) { | ||
| notifyUseCase("${title}\n${message}") | ||
| } | ||
| notifyUseCase("${title}\n${message}") |
There was a problem hiding this comment.
notifyUseCaseの戻り値が無視されており、通知失敗時のエラーが握りつぶされます。
notifyUseCaseはResult<Unit>を返しますが、この戻り値がチェックされていません。runCatchingブロック内でResult.failureが返された場合、例外はスローされないため、外側のrunCatchingは成功として扱い、通知失敗のエラーが静かに無視されます。
🐛 エラー伝播の修正案
- notifyUseCase("${title}\n${message}")
+ notifyUseCase("${title}\n${message}").getOrThrow()📝 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.
| notifyUseCase("${title}\n${message}") | |
| notifyUseCase("${title}\n${message}").getOrThrow() |
🤖 Prompt for AI Agents
In
@AndroidApp/domain/src/main/kotlin/me/nya_n/notificationnotifier/domain/usecase/impl/NotifyTargetAppNotificationUseCaseImpl.kt
at line 37, notifyUseCase("${title}\n${message}") returns a Result<Unit> that is
currently ignored, so failures inside its runCatching are silently swallowed;
update the call site (NotifyTargetAppNotificationUseCaseImpl -> notifyUseCase
invocation) to capture and check the returned Result<Unit> and propagate or
throw on failure (e.g., call getOrThrow() or rewrap/return a failed Result) so
that errors from notifyUseCase are not ignored and will surface to the outer
runCatching.
1 participant
Summary by CodeRabbit
リリースノート
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.