address review: mark the fetch bound test-only; fix the concurrency test

- 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).
This commit is contained in:
austinried
2026-10-04 00:30:01 +00:00
parent dd092ed7ad
commit adcdc442e4
2 changed files with 19 additions and 31 deletions
@@ -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
@@ -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("") {