From adcdc442e4c509da5b6fda97993d67a372188ce0 Mon Sep 17 00:00:00 2001
From: austinried <4966622+austinried@users.noreply.github.com>
Date: Sun, 27 Sep 2026 07:29:49 +0000
Subject: [PATCH] address review: mark the fetch bound test-only; fix the
 concurrency test
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- The concurrency bound already existed; this parameter only exists so tests
  can force the sequential path, and no production path passes it. Mark it
  `@VisibleForTesting` so that is explicit rather than reading as a wired
  setting.
- `playlistSongsUseTheSameRowsAtAnyFetchBound` could not fail (bound has no
  effect on the rows) — delete it and its `normalized` helper.
- `playlistSongsRespectsASmallerFetchBound` relied on a 300ms overlap
  window; use latches so the bound is proven deterministically (fails when
  the semaphore ignores the bound).
---
 .../data/source/subsonic/SubsonicSource.kt    |  2 +
 .../source/subsonic/SubsonicSourceTest.kt     | 48 +++++++------------
 2 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
index 35e9d432..e849b5fd 100644
--- a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
+++ b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt
@@ -1,5 +1,6 @@
 package com.subtracks.data.source.subsonic
 
+import androidx.annotation.VisibleForTesting
 import com.subtracks.data.model.Album
 import com.subtracks.data.model.Artist
 import com.subtracks.data.model.Playlist
@@ -34,6 +35,7 @@ class SubsonicSource(
     private val maxBitrate: Int = 0,
     private val streamFormat: String? = null,
     private val maxPages: Int = MAX_PAGES,
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     private val maxConcurrentFetches: Int = MAX_CONCURRENT_FETCHES,
 ) : MusicSource {
     private var emptyQuerySearchSupported: Boolean? = null
diff --git a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
index 7e4e7905..4200d135 100644
--- a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
+++ b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt
@@ -1,6 +1,7 @@
 package com.subtracks.data.source.subsonic
 
-import com.subtracks.data.model.PlaylistSong
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.async
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.toList
 import kotlinx.coroutines.runBlocking
@@ -350,7 +351,8 @@ class SubsonicSourceTest {
             val bound = 2
             val inFlight = AtomicInteger()
             val maxInFlight = AtomicInteger()
-            val moreThanBoundStarted = CountDownLatch(bound + 1)
+            val started = CountDownLatch(bound)
+            val release = CountDownLatch(1)
             server.dispatcher =
                 object : Dispatcher() {
                     override fun dispatch(request: RecordedRequest): MockResponse {
@@ -358,42 +360,29 @@ class SubsonicSourceTest {
                         if (url.encodedPath != "/rest/getPlaylist.view") return MockResponse().setResponseCode(404)
                         val current = inFlight.incrementAndGet()
                         maxInFlight.updateAndGet { maxOf(it, current) }
-                        moreThanBoundStarted.countDown()
-                        moreThanBoundStarted.await(300, TimeUnit.MILLISECONDS)
+                        started.countDown()
+                        release.await(5, TimeUnit.SECONDS)
                         inFlight.decrementAndGet()
                         return MockResponse().setBody(playlistResponse(url.queryParameter("id")!!))
                     }
                 }
 
-            val entries =
-                SubsonicSource(1, client(), maxConcurrentFetches = bound)
-                    .playlistSongs(listOf("p1", "p2", "p3", "p4"))
-                    .toList()
-                    .flatten()
+            val fetching =
+                async(Dispatchers.IO) {
+                    SubsonicSource(1, client(), maxConcurrentFetches = bound)
+                        .playlistSongs(listOf("p1", "p2", "p3", "p4"))
+                        .toList()
+                        .flatten()
+                }
+
+            assertTrue("$bound fetches should have started together", started.await(5, TimeUnit.SECONDS))
+            release.countDown()
+            val entries = fetching.await()
 
             assertEquals(12, entries.size)
             assertEquals(bound, maxInFlight.get())
         }
 
-    @Test
-    fun playlistSongsUseTheSameRowsAtAnyFetchBound() =
-        runBlocking {
-            server.dispatcher =
-                object : Dispatcher() {
-                    override fun dispatch(request: RecordedRequest): MockResponse {
-                        val url = request.requestUrl!!
-                        if (url.encodedPath != "/rest/getPlaylist.view") return MockResponse().setResponseCode(404)
-                        return MockResponse().setBody(playlistResponse(url.queryParameter("id")!!))
-                    }
-                }
-            val ids = listOf("p1", "p2", "p3", "p4")
-
-            val sequential = SubsonicSource(1, client(), maxConcurrentFetches = 1).playlistSongs(ids).toList().flatten()
-            val concurrent = SubsonicSource(1, client(), maxConcurrentFetches = 4).playlistSongs(ids).toList().flatten()
-
-            assertEquals(normalized(sequential), normalized(concurrent))
-        }
-
     @Test
     fun albumSongsFetchesSequentiallyWhenTheBoundIsOne() =
         runBlocking {
@@ -568,9 +557,6 @@ class SubsonicSourceTest {
             (1..3).joinToString("") { "<entry id=\"$playlistId-s$it\" title=\"Title $it\"/>" } +
             "</playlist></subsonic-response>"
 
-    private fun normalized(entries: List<PlaylistSong>): List<PlaylistSong> =
-        entries.sortedWith(compareBy({ it.playlistId }, { it.position }))
-
     private fun albumListOf(count: Int) =
         "<subsonic-response status=\"ok\"><albumList2>" +
             (0 until count).joinToString("") {