🧪 Add unit tests for SettingsRepository - #190
Conversation
Co-authored-by: alvin000009238 <107313913+alvin000009238@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors SettingsRepository to support dependency injection by accepting DataStore<Preferences> in its primary constructor, and introduces a comprehensive suite of unit tests in SettingsRepositoryTest.kt. The review feedback correctly identifies an anti-pattern where a single TestScope is shared across multiple tests, which can lead to test interference or failures. It is recommended to isolate each test's scope using runTest(testDispatcher) and to safely resolve the temporary file path using File(tempFolder.root, ...) inside the PreferenceDataStoreFactory configuration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private val testDispatcher = UnconfinedTestDispatcher() | ||
| private val testScope = TestScope(testDispatcher) | ||
|
|
||
| private fun createRepository(): SettingsRepository { | ||
| val dataStore = PreferenceDataStoreFactory.create( | ||
| scope = testScope, | ||
| produceFile = { tempFolder.newFile("test_settings.preferences_pb") } | ||
| ) | ||
| return SettingsRepository(dataStore) | ||
| } |
There was a problem hiding this comment.
Sharing a single TestScope instance across multiple tests via a class property is an anti-pattern in kotlinx.coroutines.test. When a test finishes, the TestScope is completed/cancelled, which will cause subsequent tests using the same scope to fail or behave unpredictably.
Instead, each test should run in its own isolated TestScope by calling runTest(testDispatcher). We can refactor createRepository to be an extension function on TestScope so it can use the current test's scope.
Additionally, calling tempFolder.newFile(...) inside the produceFile lambda of PreferenceDataStoreFactory.create can throw an IOException if the file already exists or is initialized multiple times. It is safer to resolve the file path using File(tempFolder.root, ...) instead.
private val testDispatcher = UnconfinedTestDispatcher()
private fun TestScope.createRepository(): SettingsRepository {
val dataStore = PreferenceDataStoreFactory.create(
scope = this,
produceFile = { File(tempFolder.root, "test_settings.preferences_pb") }
)
return SettingsRepository(dataStore)
}| } | ||
|
|
||
| @Test | ||
| fun testDefaultSettings() = testScope.runTest { |
There was a problem hiding this comment.
Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.
| fun testDefaultSettings() = testScope.runTest { | |
| fun testDefaultSettings() = runTest(testDispatcher) { |
|
|
||
|
|
||
| @Test | ||
| fun testUpdateSettings() = testScope.runTest { |
There was a problem hiding this comment.
Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.
| fun testUpdateSettings() = testScope.runTest { | |
| fun testUpdateSettings() = runTest(testDispatcher) { |
| } | ||
|
|
||
| @Test | ||
| fun testMultipleUpdates() = testScope.runTest { |
There was a problem hiding this comment.
Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.
| fun testMultipleUpdates() = testScope.runTest { | |
| fun testMultipleUpdates() = runTest(testDispatcher) { |
There was a problem hiding this comment.
Pull request overview
Adds JVM unit test coverage for SettingsRepository’s local DataStore persistence and refactors the repository constructor to support dependency injection while preserving the existing Context-based construction path.
Changes:
- Introduced
SettingsRepositoryTestcovering defaults, individual updates, and consecutiveFlow<AppSettings>emissions (Turbine). - Refactored
SettingsRepositoryto acceptDataStore<Preferences>directly, with a secondaryContextconstructor for compatibility.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| android/app/src/test/java/com/clhs/score/data/SettingsRepositoryTest.kt | New unit test suite validating SettingsRepository default values and update behavior, including sequential Flow emissions. |
| android/app/src/main/java/com/clhs/score/data/SettingsRepository.kt | Constructor refactor to allow injecting a DataStore<Preferences> while keeping the Context constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import androidx.datastore.preferences.core.PreferenceDataStoreFactory | ||
| import java.io.File | ||
| import kotlinx.coroutines.flow.first |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cc2c5003c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private fun createRepository(): SettingsRepository { | ||
| val dataStore = PreferenceDataStoreFactory.create( | ||
| scope = testScope, |
There was a problem hiding this comment.
When these tests collect repository.settings, DataStore 1.1+ starts long-lived work in the scope passed to PreferenceDataStoreFactory.create; because that scope is the same TestScope that runs each runTest and is never cancelled as background work, runTest will wait for the active DataStore job and fail or time out with UncompletedCoroutinesError. Pass the current test's backgroundScope into the factory (or cancel a separate DataStore scope in teardown) so the DataStore job is cleaned up after each test.
Useful? React with 👍 / 👎.
🎯 What:
Implemented a unit test suite for
SettingsRepositoryto cover the previously missing test gap. RefactoredSettingsRepositoryslightly to accept aDataStore<Preferences>parameter for easier dependency injection during testing while keeping the Context-based constructor for backwards compatibility.📊 Coverage:
testDefaultSettings: Validates that the default values returned by the repository match expectations.testUpdateSettings: Validates that all setter functions correctly apply the corresponding configuration changes to the DataStore.testMultipleUpdates: UsesTurbineto confirm that theFlow<AppSettings>correctly emits updated values consecutively.✨ Result:
Significant improvement in reliability and coverage for local persistence logic in
SettingsRepository.PR created automatically by Jules for task 5137358925869291418 started by @alvin000009238