From 26f58b75028ee8e21e313e755f4febb187d196e7 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Fri, 2 Oct 2026 11:59:03 +0000 Subject: [PATCH] address review: share the test helpers, guard AppModuleTest cleanup Use cancelAndJoinBlocking in the repo tests and AppModuleTest instead of hand-rolled runBlocking joins, put AppModuleTest's cleanup in a finally so a failed join cannot leave Main overridden and Koin started, and collapse the five polling await copies into one awaitUntil. upcomingItem now reads the queue snapshot under the lock but does the Room read outside it. --- .../subtracks/playback/PlaybackController.kt | 13 ++++++++---- .../test/kotlin/com/subtracks/TestSupport.kt | 13 +++++++++--- .../data/repo/DownloadRepositoryTest.kt | 13 ++++-------- .../data/repo/LibraryRepositoryTest.kt | 5 ++--- .../data/repo/SourceRepositoryTest.kt | 5 ++--- .../subtracks/data/sync/SyncManagerTest.kt | 5 ++--- .../kotlin/com/subtracks/di/AppModuleTest.kt | 21 ++++++++++--------- .../playback/PlaybackControllerTest.kt | 7 ++----- .../ui/downloads/DownloadsViewModelTest.kt | 8 ++----- .../ui/playback/QueueViewModelTest.kt | 7 ++----- .../ui/settings/AddSourceViewModelTest.kt | 8 ++----- 11 files changed, 48 insertions(+), 57 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 65bd1f8c..af943900 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -868,12 +868,17 @@ class PlaybackController( thumbnail: Boolean = false, ): CoverArtRef? = sourceRepository.coverArt(item?.coverArtId, thumbnail) - suspend fun upcomingItem(): QueueItem? = + suspend fun upcomingItem(): QueueItem? { + var current: QueueSnapshot? = null + var position: Long? = null startLock.withLock { - val snap = snapshot ?: return@withLock null - val position = currentPosition() ?: return@withLock null - queueRepository.itemAt(snap, position + 1)?.toQueueItem() + current = snapshot + position = currentPosition() } + val snapshot = current ?: return null + val at = position ?: return null + return queueRepository.itemAt(snapshot, at + 1)?.toQueueItem() + } private fun play( entries: List, diff --git a/app/src/test/kotlin/com/subtracks/TestSupport.kt b/app/src/test/kotlin/com/subtracks/TestSupport.kt index 6db81ee8..943ac7f7 100644 --- a/app/src/test/kotlin/com/subtracks/TestSupport.kt +++ b/app/src/test/kotlin/com/subtracks/TestSupport.kt @@ -4,12 +4,19 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertTrue -// Wall-clock budget for the tests that poll asynchronous playback/repository work on a real -// dispatcher. A loaded CI runner can starve those threads for seconds; this only needs to catch a -// real hang, not a slow machine. const val TEST_TIMEOUT_MS = 30_000L fun cancelAndJoinBlocking(vararg scopes: CoroutineScope) { runBlocking { scopes.forEach { it.coroutineContext[Job]?.cancelAndJoin() } } } + +fun awaitUntil( + message: String, + predicate: () -> Boolean, +) { + val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 + while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) + assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms $message", predicate()) +} diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 4fee5c9d..f5f8e6af 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -6,7 +6,8 @@ import androidx.room3.Room import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.awaitUntil +import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkFetcher import com.subtracks.data.download.ArtworkStore @@ -27,9 +28,7 @@ import com.subtracks.data.model.coverArtKey import com.subtracks.data.prefs.fakeUserPreferences import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking @@ -99,7 +98,7 @@ class DownloadRepositoryTest { @After fun tearDown() { - runBlocking { scope.coroutineContext[Job]?.cancelAndJoin() } + cancelAndJoinBlocking(scope) db.close() dir.deleteRecursively() } @@ -1159,11 +1158,7 @@ class DownloadRepositoryTest { file.writeText(contents) } - private fun await(predicate: () -> Boolean) { - val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 - while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) - assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms waiting for the expected download state", predicate()) - } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected download state", predicate) private companion object { const val ALBUM_ART = "art-al1" diff --git a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt index 3d907c71..5c9caba4 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt @@ -5,6 +5,7 @@ import androidx.room3.Room import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore import com.subtracks.data.model.Song @@ -12,9 +13,7 @@ import com.subtracks.data.prefs.fakeUserPreferences import com.subtracks.data.source.StarType import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch @@ -65,7 +64,7 @@ class LibraryRepositoryTest { @After fun tearDown() { - runBlocking { scope.coroutineContext[Job]?.cancelAndJoin() } + cancelAndJoinBlocking(scope) db.close() } diff --git a/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt index 4ef84fb5..f5575c7e 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt @@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore import com.subtracks.data.model.CoverArtRef @@ -15,10 +16,8 @@ import com.subtracks.data.prefs.UserPreferences import com.subtracks.data.prefs.fakeUserPreferences import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.async -import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.toList @@ -67,7 +66,7 @@ class SourceRepositoryTest { @After fun tearDown() { - runBlocking { scope.coroutineContext[Job]?.cancelAndJoin() } + cancelAndJoinBlocking(scope) db.close() } diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt index b8357dfc..cc588ae4 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt @@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore import com.subtracks.data.prefs.fakeUserPreferences @@ -13,9 +14,7 @@ import com.subtracks.data.repo.QueueRepository import com.subtracks.data.repo.SourceRepository import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout @@ -66,7 +65,7 @@ class SyncManagerTest { @After fun tearDown() { - runBlocking { scope.coroutineContext[Job]?.cancelAndJoin() } + cancelAndJoinBlocking(scope) db.close() server.shutdown() } diff --git a/app/src/test/kotlin/com/subtracks/di/AppModuleTest.kt b/app/src/test/kotlin/com/subtracks/di/AppModuleTest.kt index 8fdb3cfc..5aaaad19 100644 --- a/app/src/test/kotlin/com/subtracks/di/AppModuleTest.kt +++ b/app/src/test/kotlin/com/subtracks/di/AppModuleTest.kt @@ -3,14 +3,12 @@ package com.subtracks.di import android.content.Context import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.repo.DownloadRepository import com.subtracks.playback.PlaybackController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.Job -import kotlinx.coroutines.cancelAndJoin -import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain @@ -39,15 +37,18 @@ class AppModuleTest { @After fun tearDown() { - if (::koin.isInitialized) { - runBlocking { - koin.get().coroutineContext[Job]?.cancelAndJoin() - koin.get(named("io")).coroutineContext[Job]?.cancelAndJoin() - koin.get(named("playback")).coroutineContext[Job]?.cancelAndJoin() + try { + if (::koin.isInitialized) { + cancelAndJoinBlocking( + koin.get(), + koin.get(named("io")), + koin.get(named("playback")), + ) } + } finally { + stopKoin() + Dispatchers.resetMain() } - stopKoin() - Dispatchers.resetMain() } @Test diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index c7394227..d9c321e6 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -7,6 +7,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.awaitUntil import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore @@ -3079,9 +3080,5 @@ class PlaybackControllerTest { return PlaybackController(sources, queues, FakePlayerConnection(handle), downloads, scope = scope) } - private fun await(predicate: () -> Boolean) { - val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 - while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) - assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms waiting for the expected playback state", predicate()) - } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected playback state", predicate) } diff --git a/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt index 040c981f..42fa7121 100644 --- a/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt @@ -6,7 +6,7 @@ import androidx.room3.Room import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.awaitUntil import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore @@ -177,9 +177,5 @@ class DownloadsViewModelTest { private fun allRows(): List = runBlocking { db.downloadDao().all() } - private fun await(predicate: () -> Boolean) { - val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 - while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) - assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms waiting for the expected state", predicate()) - } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected state", predicate) } diff --git a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt index 5b4d18ec..8ab83c3f 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -7,6 +7,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.awaitUntil import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore @@ -518,9 +519,5 @@ class QueueViewModelTest { private fun runOnMain(block: () -> Unit) = runBlocking { withContext(dispatcher) { block() } } - private fun await(predicate: () -> Boolean) { - val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 - while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) - assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms waiting for the expected state", predicate()) - } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected state", predicate) } diff --git a/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt index 44fe0833..220579f4 100644 --- a/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt @@ -6,7 +6,7 @@ import androidx.room3.Room import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import com.subtracks.TEST_TIMEOUT_MS +import com.subtracks.awaitUntil import com.subtracks.cancelAndJoinBlocking import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.download.ArtworkStore @@ -151,9 +151,5 @@ class AddSourceViewModelTest { "", ) - private fun await(predicate: () -> Boolean) { - val deadline = System.nanoTime() + TEST_TIMEOUT_MS * 1_000_000 - while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) - assertTrue("Timed out after ${TEST_TIMEOUT_MS}ms waiting for the expected state", predicate()) - } + private fun await(predicate: () -> Boolean) = awaitUntil("waiting for the expected state", predicate) }