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.

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<QueueEntry>,

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())
+}

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"

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()
     }
 

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()
     }
 

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()
     }

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<CoroutineScope>().coroutineContext[Job]?.cancelAndJoin()
-                koin.get<CoroutineScope>(named("io")).coroutineContext[Job]?.cancelAndJoin()
-                koin.get<CoroutineScope>(named("playback")).coroutineContext[Job]?.cancelAndJoin()
+        try {
+            if (::koin.isInitialized) {
+                cancelAndJoinBlocking(
+                    koin.get<CoroutineScope>(),
+                    koin.get<CoroutineScope>(named("io")),
+                    koin.get<CoroutineScope>(named("playback")),
+                )
             }
+        } finally {
+            stopKoin()
+            Dispatchers.resetMain()
         }
-        stopKoin()
-        Dispatchers.resetMain()
     }
 
     @Test

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)
 }

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<SongDownload> = 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)
 }

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)
 }

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 {
             "<subsonic-response status=\"failed\"><error code=\"40\" message=\"Wrong username\"/></subsonic-response>",
         )
 
-    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)
 }