Let the user's pick keep the screen a reattachment was about to take
`reattach()` read `_state.value`, found it `Idle`, and then handed the job to `observe()` -- which launches a *separate* coroutine that cannot write until its `collect` has resumed with a `WorkInfo`. So the check happened at one moment and the write landed at another, with a whole pick able to fit in between: the user tapped, their metadata query suspended, the guard saw an empty screen, and the finished job from an earlier session wrote over `Ready(picked)` a moment later. The comment above that guard said "no suspension point between this check and the assignment below, so nothing can interleave". There is no assignment below, and the two lines are in different coroutines. That sentence is why this sat as flaky CI for two days rather than being read as the product race it is. `ScreenOwnership` makes the answer the test already encodes -- the user's pick wins -- true rather than probable. A claim is taken synchronously when the user acts; every write that lands after a suspension point checks the claim it was made under and drops itself if that claim has been superseded. Dropped, not reordered: a write that is dropped cannot come back later. Cancelling the superseded observer was never enough on its own. `Job.cancel` is honoured at the next suspension point, and a collector that has already resumed and is on its way to `_state.value = ...` has none left; the write lands anyway. It also cannot help at all in the case reported, where nothing supersedes the observation until after it has been launched. `JoinViewModel` had the identical shape and nothing watching it, so it gets the same fix and the counterpart test that was missing. Its pick dispatcher becomes injectable for the same reason `ConversionViewModel`'s already was: without that seam there is no way to ask what happens while a pick is still in flight. Closes #49 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,63 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import kotlinx.coroutines.CoroutineDispatcher
|
||||
import java.util.concurrent.ConcurrentLinkedQueue
|
||||
import kotlin.coroutines.CoroutineContext
|
||||
|
||||
/**
|
||||
* A dispatcher that holds a pick in flight until the test lets it finish.
|
||||
*
|
||||
* Issue #49 is about what a ViewModel does *while* a pick is between the tap and the write it
|
||||
* eventually makes. Both ViewModels put a blocking hop there — the metadata query, and on the
|
||||
* convert side the probe as well — and both hops go through an injectable dispatcher. Handing
|
||||
* them this one turns "the pick has been made but has not landed yet" from a window a test has
|
||||
* to race into a state it can simply sit in.
|
||||
*
|
||||
* Nothing here is a fake pick. The real `InputQuery.describe` still runs, on this thread,
|
||||
* whenever [runAll] is called; the only thing under the test's control is *when*.
|
||||
*
|
||||
* Confined to the thread that drives the test. Both ViewModels reach `withContext(pickDispatcher)`
|
||||
* from a coroutine on the main dispatcher, so [dispatch] is only ever called from there — the
|
||||
* queue is concurrent anyway, because a dispatcher that quietly dropped a block from another
|
||||
* thread would fail as a hang rather than as an assertion.
|
||||
*/
|
||||
class ParkedPickDispatcher : CoroutineDispatcher() {
|
||||
|
||||
private val parked = ConcurrentLinkedQueue<Runnable>()
|
||||
|
||||
/**
|
||||
* How many blocks are waiting.
|
||||
*
|
||||
* Asserted on before the interesting part of a test, because "the pick was in flight" is a
|
||||
* premise rather than a detail: a zero here means the pick had already landed and whatever
|
||||
* the test went on to prove was proved about a different situation.
|
||||
*/
|
||||
val parkedCount: Int get() = parked.size
|
||||
|
||||
override fun dispatch(context: CoroutineContext, block: Runnable) {
|
||||
parked += block
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes everything parked, oldest first, and hands it to the caller to run.
|
||||
*
|
||||
* What [runAll] cannot express: two picks are two hops through this dispatcher, and the defect
|
||||
* they can produce is the *first* one finishing last. Running them in the order they arrived
|
||||
* is the one order in which nothing goes wrong, so a test has to be able to choose.
|
||||
*/
|
||||
fun takeParked(): List<Runnable> = generateSequence { parked.poll() }.toList()
|
||||
|
||||
/**
|
||||
* Runs everything parked, and everything that parks as a result.
|
||||
*
|
||||
* The loop is not defensive: `ConversionViewModel.onInputPicked` makes two hops through this
|
||||
* dispatcher — the metadata query, then the probe — and the second is only enqueued once the
|
||||
* first has run. Draining once would leave the probe parked for the rest of the process.
|
||||
*/
|
||||
fun runAll() {
|
||||
while (true) {
|
||||
val next = parked.poll() ?: return
|
||||
next.run()
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,125 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNotNull
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* The half of issue #49 that is not about reattachment at all.
|
||||
*
|
||||
* `onInputPicked` makes two writes and both of them land after a hop off the main thread, so both
|
||||
* belong to whichever pick was in flight rather than to whichever pick the user last made. Nothing
|
||||
* was enforcing that. Two taps in quick succession — an easy thing to do while a `content://`
|
||||
* metadata query is slow — put the loser's file on screen if its query happened to come back
|
||||
* second, which is the same defect the ticket reported against reattachment with a different
|
||||
* coroutine on the losing side.
|
||||
*
|
||||
* Both cases below existed before the fix and neither was reported, because a pick that loses to
|
||||
* another pick still shows *a* file the user chose. That is what made this worth closing in the
|
||||
* same change: one rule covering every deferred write is checkable, where "the observer checks and
|
||||
* the pick does not" is a rule nobody can hold in their head.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class PickOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var parkedPick: ParkedPickDispatcher
|
||||
private lateinit var viewModel: ConversionViewModel
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
||||
ConversionDependencies.probe = { _, _ -> PROBE }
|
||||
installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/dev/null"))
|
||||
|
||||
parkedPick = ParkedPickDispatcher()
|
||||
viewModel = ConversionViewModel(app, pickDispatcher = parkedPick)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* Two taps, and the first one's metadata query is the slow one.
|
||||
*
|
||||
* The order is chosen rather than raced: both queries are parked, and this runs the second
|
||||
* before the first. Without an ownership check the straggler writes last and the screen ends
|
||||
* up showing a file the user moved off two taps ago.
|
||||
*/
|
||||
@Test
|
||||
fun `the slower of two picks does not land on top of the faster one`() {
|
||||
viewModel.onInputPicked(FIRST)
|
||||
viewModel.onInputPicked(SECOND)
|
||||
|
||||
val queries = parkedPick.takeParked()
|
||||
assertEquals("both picks should be in flight", 2, queries.size)
|
||||
// The second pick's query comes back first; the first pick's is the straggler.
|
||||
queries[1].run()
|
||||
queries[0].run()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertEquals(
|
||||
"a pick the user has already replaced took the screen: $current",
|
||||
SECOND,
|
||||
(current as ConversionState.Ready).input.uri,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The second of `onInputPicked`'s two writes, which lands a whole probe later.
|
||||
*
|
||||
* The probe hop is a native process spawn, so it is the longest gap in a pick and the easiest
|
||||
* one to pick again during. This used to be guarded by comparing URIs against the state, which
|
||||
* answers a narrower question than the one that matters — it cannot tell a second pick of the
|
||||
* same file from the first, and it reads a state that a later claim may not have written yet,
|
||||
* which is exactly this case: the newer pick has claimed the screen but its own query has not
|
||||
* come back, so the state still names the older file and the comparison waves it through.
|
||||
*/
|
||||
@Test
|
||||
fun `a probe from a pick the user has moved off does not fill the card in`() {
|
||||
viewModel.onInputPicked(FIRST)
|
||||
parkedPick.takeParked().single().run()
|
||||
assertEquals(FIRST, (viewModel.state.value as ConversionState.Ready).input.uri)
|
||||
|
||||
// The user picks again while the first pick is still probing.
|
||||
viewModel.onInputPicked(SECOND)
|
||||
val pending = parkedPick.takeParked()
|
||||
assertEquals("the first probe and the second query should both be waiting", 2, pending.size)
|
||||
pending[0].run()
|
||||
|
||||
assertNull(
|
||||
"a probe belonging to a pick the user replaced must not reach the card",
|
||||
(viewModel.state.value as ConversionState.Ready).input.probe,
|
||||
)
|
||||
|
||||
// And the pick that did win still fills its own card in, probe included.
|
||||
pending[1].run()
|
||||
parkedPick.runAll()
|
||||
val settled = viewModel.state.value as ConversionState.Ready
|
||||
assertEquals(SECOND, settled.input.uri)
|
||||
assertNotNull("the winning pick's own probe still has to land", settled.input.probe)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val FIRST: Uri = Uri.fromFile(File("/tmp/first.mp4"))
|
||||
val SECOND: Uri = Uri.fromFile(File("/tmp/second.mp4"))
|
||||
val PROBE = InputProbe(videoCodec = "h264")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,155 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.WorkManager
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* Issue #49, on the JVM and without the race.
|
||||
*
|
||||
* `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` has been catching this on
|
||||
* devices since 2026-08-24 — four times in 400 gating leg-attempts, on API 33, 35 and 36 — and
|
||||
* every occurrence passed on re-run, which is why it was read as flaky infrastructure for two
|
||||
* days. It is not. The assertion it fails on is `expected null, but was:<Converted>`: a finished
|
||||
* job from an earlier session taking a screen the user had already picked a file on.
|
||||
*
|
||||
* The defect is a check-then-act whose act is deferred into another coroutine. `reattach()` reads
|
||||
* `_state.value` and then calls `observe()`, which *launches* a collector that has to suspend on
|
||||
* `getWorkInfoByIdFlow(...).collect` before it can write anything. So the check happens at one
|
||||
* moment and the write lands at another:
|
||||
*
|
||||
* 1. `init` starts the tag query and suspends in it.
|
||||
* 2. The user picks a file; `onInputPicked` suspends in its metadata query.
|
||||
* 3. The query comes back. `_state.value` is still `Idle` — step 2 has not written yet — so the
|
||||
* guard passes and an observation of the old job is launched.
|
||||
* 4. The pick lands. `Ready(picked)`. The user owns the screen.
|
||||
* 5. The observation's first `WorkInfo` arrives and writes `Converted(yesterday)` over it.
|
||||
*
|
||||
* The comment above that guard claimed "no suspension point between this check and the assignment
|
||||
* below, so nothing can interleave". There is no assignment below, and the check and the write
|
||||
* are in different coroutines.
|
||||
*
|
||||
* [ReattachGuardsTest] covers the case where the pick has already *landed*, which the plain guard
|
||||
* does catch. This covers the one where it is still in flight, which it does not.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class ReattachmentOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: RecordingPublisher
|
||||
private lateinit var workManager: WorkManager
|
||||
private lateinit var staged: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
|
||||
staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath))
|
||||
workManager = WorkManager.getInstance(app)
|
||||
// The situation reattachment exists for: a conversion that finished in a process that is
|
||||
// gone, with its output still in the cache and nothing in the UI holding its id.
|
||||
workManager.enqueue(
|
||||
ConversionWorker.request(
|
||||
inputUri = Uri.parse("content://test/holiday.mp4"),
|
||||
displayName = "holiday.mp4",
|
||||
sizeBytes = 4_096L,
|
||||
),
|
||||
).result.get()
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* The race, made into a state the test can sit in rather than one it has to catch.
|
||||
*
|
||||
* The pick is parked on a dispatcher this test owns, so it stays in flight — issued, not yet
|
||||
* written — for as long as the assertions need it to be. Everything else is production: a
|
||||
* real `WorkManager` holding a real finished job, the real `reattach`, the real `observe`.
|
||||
*
|
||||
* Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query hops
|
||||
* to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only come back
|
||||
* as a message posted to the main looper — and that looper is paused, so it cannot run until
|
||||
* something pumps it. `onInputPicked` is an ordinary synchronous call from this thread. The
|
||||
* pick is therefore *always* issued before the guard runs; none of it is left to timing, which
|
||||
* is the whole point of writing this here rather than relying on the 1-in-130 device sighting.
|
||||
*/
|
||||
@Test
|
||||
fun `a conversion found while the user was picking never reaches the screen`() {
|
||||
val picked = Uri.fromFile(File(app.cacheDir, "beach.mp4").apply { writeBytes(ByteArray(2048)) })
|
||||
val parkedPick = ParkedPickDispatcher()
|
||||
val viewModel = ConversionViewModel(app, pickDispatcher = parkedPick)
|
||||
viewModel.onInputPicked(picked)
|
||||
|
||||
assertEquals(
|
||||
"the pick must still be in flight, or this proves something about a different situation",
|
||||
1,
|
||||
parkedPick.parkedCount,
|
||||
)
|
||||
|
||||
// The control, and the reason this test does not rest on a settle window being long
|
||||
// enough. A second ViewModel with nothing to supersede it reattaches to the same job
|
||||
// through the same code; when it has arrived, the whole query-guard-observe-write path has
|
||||
// demonstrably run to completion. `viewModel` started its own reattachment first, so it
|
||||
// has had at least as long. Waiting on this rather than on a sleep is what makes the
|
||||
// assertion below "it did not happen" rather than "it had not happened yet".
|
||||
reattachmentHasRunToCompletion()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertTrue("reattachment took the screen from the user: $current", current is ConversionState.Idle)
|
||||
|
||||
// And the pick, when it lands, is what stays there.
|
||||
parkedPick.runAll()
|
||||
val ready = awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
||||
assertEquals(picked, (ready as ConversionState.Ready).input.uri)
|
||||
reattachmentHasRunToCompletion()
|
||||
assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value)
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of the contract: a reattachment nobody has superseded still takes the screen.
|
||||
*
|
||||
* Without this, dropping every reattachment on the floor would pass the test above. Same job,
|
||||
* same WorkManager, same production path — only the pick is missing.
|
||||
*/
|
||||
@Test
|
||||
fun `a conversion nobody has superseded still reaches the screen`() {
|
||||
val converted = awaitState(ConversionViewModel(app).state, "Converted") {
|
||||
it is ConversionState.Converted
|
||||
}
|
||||
|
||||
assertEquals(staged.absolutePath, (converted as ConversionState.Converted).staged.absolutePath)
|
||||
assertEquals("holiday.mp4", converted.input.displayName)
|
||||
}
|
||||
|
||||
/**
|
||||
* Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed.
|
||||
*
|
||||
* [awaitState] pumps the main looper, which is what runs every reattachment continuation
|
||||
* waiting on it — this one's, and the one belonging to the ViewModel under test, which was
|
||||
* posted earlier and therefore runs first.
|
||||
*/
|
||||
private fun reattachmentHasRunToCompletion() {
|
||||
awaitState(ConversionViewModel(app).state, "Converted") { it is ConversionState.Converted }
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,87 @@
|
||||
package org.libremediaconverter.join
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.ParkedPickDispatcher
|
||||
import org.libremediaconverter.convert.RecordingPublisher
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
|
||||
/**
|
||||
* `PickOwnershipTest`'s case on the join side.
|
||||
*
|
||||
* `onInputsPicked` makes one write and it lands after a hop off the main thread, so it belongs to
|
||||
* whichever pick was in flight rather than to whichever set of files the user last chose. Two
|
||||
* selections in quick succession — likelier here than on the convert side, since a join picks
|
||||
* several files at a time and the metadata query is per file — put the loser's files on screen if
|
||||
* its query came back second.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class JoinPickOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var parkedPick: ParkedPickDispatcher
|
||||
private lateinit var viewModel: JoinViewModel
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
||||
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
|
||||
|
||||
parkedPick = ParkedPickDispatcher()
|
||||
viewModel = JoinViewModel(app, pickDispatcher = parkedPick)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* Two selections, with the first one's metadata query the slow one.
|
||||
*
|
||||
* The order is chosen rather than raced: both queries are parked, and this runs the second
|
||||
* before the first.
|
||||
*/
|
||||
@Test
|
||||
fun `the slower of two selections does not land on top of the faster one`() {
|
||||
viewModel.onInputsPicked(FIRST)
|
||||
viewModel.onInputsPicked(SECOND)
|
||||
|
||||
val queries = parkedPick.takeParked()
|
||||
assertEquals("both selections should be in flight", 2, queries.size)
|
||||
// The second selection's query comes back first; the first one's is the straggler.
|
||||
queries[1].run()
|
||||
queries[0].run()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertEquals(
|
||||
"a selection the user has already replaced took the screen: $current",
|
||||
SECOND,
|
||||
(current as JoinState.Ready).inputs.map { it.uri },
|
||||
)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val FIRST = listOf(
|
||||
Uri.parse("content://test/first-a.mp4"),
|
||||
Uri.parse("content://test/first-b.mp4"),
|
||||
)
|
||||
val SECOND = listOf(
|
||||
Uri.parse("content://test/second-a.mp4"),
|
||||
Uri.parse("content://test/second-b.mp4"),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,166 @@
|
||||
package org.libremediaconverter.join
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.WorkManager
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.ParkedPickDispatcher
|
||||
import org.libremediaconverter.convert.RecordingPublisher
|
||||
import org.libremediaconverter.convert.awaitState
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* Issue #49 on the join side, where nothing was watching for it.
|
||||
*
|
||||
* `reattach()` checks that the screen is still free, and then hands the answer to `observe()`,
|
||||
* which writes from a *different* coroutine that has to suspend on `collect` before it can write
|
||||
* anything at all. So the check happens at one moment and the write lands at another, with a
|
||||
* whole pick able to fit in between:
|
||||
*
|
||||
* 1. `init` starts the tag query and suspends in it.
|
||||
* 2. The user picks files; `onInputsPicked` suspends in its metadata query.
|
||||
* 3. The query comes back. The screen is still `Idle` — step 2 has not written yet — so the
|
||||
* guard passes and an observation of the old job is launched.
|
||||
* 4. The pick lands. `Ready(picked)`. The user owns the screen.
|
||||
* 5. The observation's first `WorkInfo` arrives and writes `Joined(yesterday's file)` over it.
|
||||
*
|
||||
* The comment above that guard used to say "no suspension point between this check and the
|
||||
* assignment below, so nothing can interleave". There is no assignment below, and the two lines
|
||||
* are in different coroutines.
|
||||
*
|
||||
* The convert side has been failing this on CI for two days at roughly 1-in-130 and was read as
|
||||
* flaky infrastructure. `JoinViewModel` has the identical shape and no test at all, which is why
|
||||
* this one was written before the fix rather than after it.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class JoinReattachmentOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: RecordingPublisher
|
||||
private lateinit var workManager: WorkManager
|
||||
private lateinit var staged: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
|
||||
staged = publisher.createStagingFile("joined-yesterday.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
installTestWorkManager(
|
||||
app,
|
||||
workDataOf(
|
||||
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
||||
ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name,
|
||||
),
|
||||
)
|
||||
workManager = WorkManager.getInstance(app)
|
||||
// The situation reattachment exists for: a join that finished in a process that is gone,
|
||||
// with its output still in the cache and nothing in the UI holding its id.
|
||||
workManager.enqueue(
|
||||
ConcatWorker.request(
|
||||
inputs = listOf(
|
||||
Uri.parse("content://test/yesterday-a.mp4"),
|
||||
Uri.parse("content://test/yesterday-b.mp4"),
|
||||
),
|
||||
totalBytes = 8_192L,
|
||||
),
|
||||
).result.get()
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* The race, made into a state the test can sit in rather than one it has to catch.
|
||||
*
|
||||
* The pick is parked on a dispatcher this test owns, so it is in flight — issued, not yet
|
||||
* written — for as long as the assertions need it to be. Everything else is real: a real
|
||||
* `WorkManager` holding a real finished job, the production `reattach`, the production
|
||||
* `observe`.
|
||||
*
|
||||
* Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query
|
||||
* hops to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only
|
||||
* come back as a message posted to the main looper — and that looper is paused, so it cannot
|
||||
* run until something pumps it. `onInputsPicked` is an ordinary synchronous call from this
|
||||
* thread. The pick is therefore always issued before the guard runs, with nothing left to
|
||||
* timing.
|
||||
*/
|
||||
@Test
|
||||
fun `a join found while the user was picking never reaches the screen`() {
|
||||
val parkedPick = ParkedPickDispatcher()
|
||||
val viewModel = JoinViewModel(app, pickDispatcher = parkedPick)
|
||||
viewModel.onInputsPicked(PICKED)
|
||||
|
||||
assertEquals(
|
||||
"the pick must still be in flight, or this proves something about a different situation",
|
||||
1,
|
||||
parkedPick.parkedCount,
|
||||
)
|
||||
|
||||
// The control, and the reason this test does not rest on a settle window being long
|
||||
// enough. A second ViewModel with nothing to supersede it reattaches to the same job
|
||||
// through the same code; when it has arrived, the whole query-guard-observe-write path
|
||||
// has demonstrably run to completion. `viewModel` started its own reattachment first, so
|
||||
// it has had at least as long. Waiting on this rather than on a sleep is what makes the
|
||||
// assertion below "it did not happen" instead of "it had not happened yet".
|
||||
reattachmentHasRunToCompletion()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertTrue("reattachment took the screen from the user: $current", current is JoinState.Idle)
|
||||
|
||||
// And the pick, when it lands, is what stays there.
|
||||
parkedPick.runAll()
|
||||
val ready = awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
|
||||
assertEquals(PICKED, (ready as JoinState.Ready).inputs.map { it.uri })
|
||||
reattachmentHasRunToCompletion()
|
||||
assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value)
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of the contract: a reattachment nobody has superseded still takes the screen.
|
||||
*
|
||||
* Without this, dropping every reattachment on the floor would pass the test above. It is the
|
||||
* same job, the same WorkManager and the same production path — only the pick is missing.
|
||||
*/
|
||||
@Test
|
||||
fun `a join nobody has superseded still reaches the screen`() {
|
||||
val joined = awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined }
|
||||
|
||||
assertEquals(staged.absolutePath, (joined as JoinState.Joined).staged.absolutePath)
|
||||
}
|
||||
|
||||
/**
|
||||
* Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed.
|
||||
*
|
||||
* [awaitState] pumps the main looper, which is what runs every reattachment continuation
|
||||
* waiting on it — this one's and the one belonging to the ViewModel under test, which was
|
||||
* posted earlier and therefore runs first.
|
||||
*/
|
||||
private fun reattachmentHasRunToCompletion() {
|
||||
awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined }
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val PICKED = listOf(
|
||||
Uri.parse("content://test/clip-one.mp4"),
|
||||
Uri.parse("content://test/clip-two.mp4"),
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user