From 1128af165da08719f0f33bf88c1a69d1643502f4 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:52:08 +0000 Subject: [PATCH] address review: join the controller scope, keep DB closes where nothing leaks PlaybackController.now exposes awaitStopped() so the tests that own its DB can join its scope before db.close(); close() stays cancel-only so AppModuleTest (Main dispatcher) cannot deadlock. Restores db.close() in the tests that have no leaking scope, closes the SyncManager AddSourceViewModelTest was leaking, replaces the duplicated poll with the existing awaitCoverArt helper, and trims the testLogging block to the one line that does anything. --- app/build.gradle.kts | 4 ---- .../com/subtracks/playback/PlaybackController.kt | 4 ++++ .../com/subtracks/data/db/ArtworkSeedDaoTest.kt | 6 ++++++ .../kotlin/com/subtracks/data/db/LibraryDaoTest.kt | 6 ++++++ .../com/subtracks/data/repo/DownloadRepositoryTest.kt | 1 + .../com/subtracks/data/repo/LibraryRepositoryTest.kt | 1 + .../kotlin/com/subtracks/data/repo/QueueKeysetTest.kt | 6 ++++++ .../com/subtracks/data/repo/QueueRepositoryTest.kt | 6 ++++++ .../com/subtracks/data/repo/SourceRepositoryTest.kt | 11 +++-------- .../kotlin/com/subtracks/data/sync/SyncManagerTest.kt | 1 + .../kotlin/com/subtracks/data/sync/SyncServiceTest.kt | 6 ++++++ .../com/subtracks/playback/PlaybackControllerTest.kt | 2 ++ .../subtracks/ui/downloads/DownloadsViewModelTest.kt | 2 ++ .../com/subtracks/ui/playback/QueueViewModelTest.kt | 2 ++ .../subtracks/ui/settings/AddSourceViewModelTest.kt | 10 +++++++--- 15 files changed, 53 insertions(+), 15 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 147f741b..214ff6c8 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -103,10 +103,6 @@ tasks.withType().configureEach { } testLogging { exceptionFormat = TestExceptionFormat.FULL - events("failed") - showStackTraces = true - showExceptions = true - showCauses = true } } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 349c58ad..4678a99a 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -173,6 +173,10 @@ class PlaybackController( scope.cancel() } + internal suspend fun awaitStopped() { + scope.coroutineContext[Job]?.join() + } + private val _ready = MutableStateFlow(false) val ready: StateFlow = _ready diff --git a/app/src/test/kotlin/com/subtracks/data/db/ArtworkSeedDaoTest.kt b/app/src/test/kotlin/com/subtracks/data/db/ArtworkSeedDaoTest.kt index bcb80b9f..cacf6417 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/ArtworkSeedDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/ArtworkSeedDaoTest.kt @@ -9,6 +9,7 @@ import com.subtracks.data.model.ArtworkSeed import com.subtracks.data.model.Source import com.subtracks.data.repo.ArtworkSeedRepository import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNull import org.junit.Before @@ -29,6 +30,11 @@ class ArtworkSeedDaoTest { .build() } + @After + fun tearDown() { + db.close() + } + @Test fun deletingASourceCascadesToItsSeeds() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt index ffc96f8a..253105f2 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -17,6 +17,7 @@ import com.subtracks.data.model.SongDownload import com.subtracks.data.model.Source import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue @@ -38,6 +39,11 @@ class LibraryDaoTest { .build() } + @After + fun tearDown() { + db.close() + } + @Test fun albumSortsAndStarredFilter() = runTest { 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 2bf36d3b..0c97abfc 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -93,6 +93,7 @@ class DownloadRepositoryTest { fun tearDown() { repository.close() sources.close() + db.close() dir.deleteRecursively() } 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 18419087..a007b322 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt @@ -51,6 +51,7 @@ class LibraryRepositoryTest { @After fun tearDown() { sourceRepository.close() + db.close() } @Test diff --git a/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt index d3bf7c26..b1ba328e 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt @@ -12,6 +12,7 @@ import com.subtracks.data.model.PlaylistSong import com.subtracks.data.model.Song import com.subtracks.data.model.Source import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Before @@ -34,6 +35,11 @@ class QueueKeysetTest { repository = QueueRepository(db) } + @After + fun tearDown() { + db.close() + } + @Test fun albumKeysetFromMatchesOffsetWindowAtADeepPosition() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt index 08ccc52e..5e22631a 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueRepositoryTest.kt @@ -15,6 +15,7 @@ import com.subtracks.data.model.QueueKind import com.subtracks.data.model.Song import com.subtracks.data.model.Source import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue @@ -38,6 +39,11 @@ class QueueRepositoryTest { repository = QueueRepository(db) } + @After + fun tearDown() { + db.close() + } + @Test fun wholeAlbumEntryResolvesEveryTrackInOrder() = runTest { 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 72e1ee1b..ca52d943 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt @@ -61,6 +61,7 @@ class SourceRepositoryTest { @After fun tearDown() { repository.close() + db.close() } @Test @@ -267,15 +268,9 @@ class SourceRepositoryTest { repository.setOfflineMode(true) withTimeout(5_000) { repository.offline.first { it } } - var ref: CoverArtRef? = repository.coverArt("art-1") - withTimeout(5_000) { - while (ref?.url?.startsWith("file:") != true) { - delay(10) - ref = repository.coverArt("art-1") - } - } + val ref = awaitCoverArt("art-1") - assertTrue("expected the stored file, was ${ref?.url}", ref?.url?.startsWith("file:") == true) + assertTrue("expected the stored file, was ${ref.url}", ref.url.startsWith("file:")) } private suspend fun awaitActiveSourceId(): Long = withTimeout(5_000) { repository.activeSourceId().first { it != null }!! } 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 bd8fd3a5..599eff16 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt @@ -59,6 +59,7 @@ class SyncManagerTest { fun tearDown() { manager.close() sourceRepository.close() + db.close() server.shutdown() } diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt index 9f078f1b..4e89b896 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -21,6 +21,7 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Before import org.junit.Test @@ -40,6 +41,11 @@ class SyncServiceTest { .build() } + @After + fun tearDown() { + db.close() + } + @Test fun syncInsertsAndPrunesRemovedRows() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index b962a347..1c8d3006 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -101,8 +101,10 @@ class PlaybackControllerTest { @After fun tearDown() { controller.close() + runBlocking { controller.awaitStopped() } sources.close() downloads.close() + db.close() dispatcher.close() } 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 2c9e99ed..e6bb8221 100644 --- a/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/downloads/DownloadsViewModelTest.kt @@ -93,8 +93,10 @@ class DownloadsViewModelTest { fun tearDown() { runBlocking { viewModel.viewModelScope.coroutineContext[Job]?.cancelAndJoin() } controller.close() + runBlocking { controller.awaitStopped() } downloadRepository.close() sourceRepository.close() + db.close() Dispatchers.resetMain() dispatcher.close() } 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 34a26054..2a8a4acd 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -87,8 +87,10 @@ class QueueViewModelTest { fun tearDown() { runBlocking { viewModel.viewModelScope.coroutineContext[Job]?.cancelAndJoin() } controller.close() + runBlocking { controller.awaitStopped() } downloads.close() sources.close() + db.close() Dispatchers.resetMain() dispatcher.close() } 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 6ad1bc80..c5c3b889 100644 --- a/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/settings/AddSourceViewModelTest.kt @@ -46,6 +46,7 @@ class AddSourceViewModelTest { private lateinit var downloadRepository: DownloadRepository private lateinit var server: MockWebServer private val createdViewModels = mutableListOf() + private val syncManagers = mutableListOf() @Before fun setUp() { @@ -74,9 +75,11 @@ class AddSourceViewModelTest { @After fun tearDown() { createdViewModels.forEach { runBlocking { it.viewModelScope.coroutineContext[Job]?.cancelAndJoin() } } + syncManagers.forEach { it.close() } server.shutdown() downloadRepository.close() sourceRepository.close() + db.close() Dispatchers.resetMain() dispatcher.close() } @@ -133,9 +136,10 @@ class AddSourceViewModelTest { assertNotNull("the active source must survive the refused delete", db.sourcesDao().sourceOnce(sourceId)) } - private fun viewModel(sourceId: Long? = null): AddSourceViewModel = - AddSourceViewModel(sourceRepository, SyncManager(db, sourceRepository, QueueRepository(db)), downloadRepository, sourceId) - .also { createdViewModels += it } + private fun viewModel(sourceId: Long? = null): AddSourceViewModel { + val syncManager = SyncManager(db, sourceRepository, QueueRepository(db)).also { syncManagers += it } + return AddSourceViewModel(sourceRepository, syncManager, downloadRepository, sourceId).also { createdViewModels += it } + } private fun ok() = MockResponse().setBody("")