Compare commits

..
Author SHA1 Message Date
JMR-devandClaude Opus 5 79097a0256 Record wave 4's landed coverage, and that the hang watchdog has fired
Numbers re-measured on 6004398 rather than inherited: 94.2% line (2234/2372),
87.5% branch (1171/1338), 628 tests in 96 classes, missed 138 lines and 167
branches.

The entry explains its own branch move, because this file has a history of not
doing that. Wave 4's rise is almost entirely numerator -- up 80, with the
denominator moving -4 -- unlike the 2026-08-29 seam work, where the numerator
rose 37 while the denominator fell 70 and much of the gain was scaffolding
leaving the measurement. The line denominator grew 20, which is the seams the
wave cut rather than untested code.

Also records a methodological result worth more than the percentages: #218 fixed
an intermittent flake whose six-runs-per-arm comparison caught nothing either way
and proved nothing, and was settled by a deterministic mutation instead -- then
confirmed when the race reproduced on #217's leg, below the fix.

The JUnit-timeout trap gains the outcome it was waiting for: the jstack watchdog
caught #125's Room/WorkManager deadlock on CI in 10m57s with the hung test named,
against that ticket's prediction of a 60-minute cap and no cause. #125 is closed
as bounded; the inversion is internal to the two libraries and still live at
work-runtime 2.11.2 / room 2.7.0.

Instrumented counts are untouched: #221 is what changes them, so it carries them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 21:24:33 -05:00
Jason Ross 6004398a83 Merge pull request #219 from JMR-dev/fix/rotation-waits-for-recreation
Wait for the rotation to rebuild the Activity, not for the composition to idle
2026-09-05 20:43:47 -05:00
Jason Ross a354620bf5 Merge pull request #218 from JMR-dev/fix/injectable-startup-sweep-scope
Finish the startup sweep before onCreate returns, in the JVM suite
2026-09-05 20:43:24 -05:00
Jason Ross 17c91081cd Merge branch 'main' into fix/injectable-startup-sweep-scope 2026-09-05 20:35:39 -05:00
Jason Ross 20f718842d Merge pull request #217 from JMR-dev/test/session-outcome-seam
Give both engines one rc-to-outcome function, and unify the failure message
2026-09-05 20:29:45 -05:00
JMR-devandClaude Opus 5 32ab54da3c Wait for the rotation to rebuild the Activity, not for the composition to idle
thePickedInputSurvivesARealRotation synchronised a rotation with waitForIdle(),
which waits for the compose hierarchy to settle. Right after a rotation the
window manager has accepted but not yet delivered as a configuration change, the
old Activity's composition is already idle -- so it returns, composeRule.activity
still resolves to the old instance, and the guard reads an unchanged identity
hash. Nothing waited for MainActivity to be rebuilt.

That is the clean AssertionError on #217's API 33 gating leg, run 33698846104:
it failed the SECOND guard, so the first had passed and the display really had
rotated. The wedges this ticket opened with are the same race taken the other
way -- land while the composition is being torn down and waitForIdle has nothing
coherent to settle on.

awaitRecreation() waits on a counter fed by the runner's lifecycle monitor,
bounded at 15 s. Deliberately not polling composeRule.activity: that resolves
through scenario.onActivity, which blocks on the main thread, so polling it
across a recreation is a plausible reading of the very wedge being fixed.

Both guards stay. The identity-hash one is now a backstop rather than the
primary detector -- a configChanges attribute trips the barrier's timeout first,
with a message saying what was waited for.

Measured on the local API 33 emulator. With the fix, 60/60 green. With the
watcher mutated so the counter never increments, the test fails in 15 s naming
itself and its condition, and the run still reports received 60/60 completed
cleanly -- where the same missing recreation used to cost the leg 20 minutes and
name nothing. The pre-fix flake does not reproduce on this host, so that is a
demonstration of the timeout path, not a before/after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:42 -05:00
JMR-devandClaude Opus 5 e7caeeac43 Finish the startup sweep before onCreate returns, in the JVM suite
Robolectric builds an Application per test class that asks for one, and each
onCreate launched a staging sweep on Dispatchers.IO over the shared
<cacheDir>/conversions/. Nothing joined them, so a test asserting about a staged
file was racing every sweep the classes before it had left in flight (#159).
It was CI-only until wave 4 added ten Robolectric classes, at which point
OutputPublisherStagingTest started failing locally too.

LibreMediaConverterApp gains a protected open sweepScope and publishes the Job
onCreate started; the JVM suite substitutes TestLibreMediaConverterApp, whose
scope is Dispatchers.Unconfined so the sweep -- a plain function that never
suspends -- runs to completion inline. The SupervisorJob is kept so this differs
from production in the dispatcher alone.

Suite-wide rather than per-test: 27 of the 58 Robolectric classes touch that
directory, so opt-in was not a real option.

It costs one assertion, knowingly. AppStartSweepTest opened by asserting that
the manifest's android:name is what Robolectric instantiated. An application=
override replaces the manifest rather than being checked against it, and
applicationInfo.className reports the override too, so that claim is now
unobservable from this source set and a rewritten version would assert the
override against itself. The manifest link is device-only; the cast in setUp
still catches the test app ceasing to extend the real one.

AppStartSweepTest also joins the published Job instead of polling for ten
seconds -- a poll cannot tell "swept" from "not started yet" -- and gains a test
pinning that the sweep is complete when onCreate returns, which is the property
the substitution exists for and the only place it is checked.

Verified by mutation rather than by repetition. Putting the test app back on
Dispatchers.IO reddens that test 5 times out of 5, while running the whole suite
six times per arm caught nothing either way: at the rate #159 was observed at, a
clean six-run arm is roughly a coin flip, so the comparison was underpowered and
is not offered as evidence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-05 19:36:40 -05:00
7 changed files with 240 additions and 53 deletions
+56 -4
View File
@@ -130,9 +130,9 @@ install for code that can never run — and on API 37 the full APK does not fit
- The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
decision layer, where one branch is one documented user-visible outcome and the metric counts
answers rather than complexity. Every other rule still applies there.
- **Coverage is reported, not gated** — **92.8% of lines (2183/2352), 81.3% of branches
(1091/1342)**, measured 2026-09-02 with `./gradlew :app:jacocoTestReport`, against 584 JVM tests
in 87 classes.
- **Coverage is reported, not gated** — **94.2% of lines (2234/2372), 87.5% of branches
(1171/1338)**, measured 2026-09-05 with `./gradlew :app:jacocoTestReport`, against 628 JVM tests
in 96 classes.
**Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
@@ -283,6 +283,33 @@ install for code that can never run — and on API 37 the full APK does not fit
#194 before re-arguing either way — and note the reason it is worth cutting is not coverage but
that the `runCatching` fallback logs "assuming permissive" while returning empty sets, which makes
`canEncode` and `canDecode` answer *no* for everything.
**Wave 4's tests then landed on 2026-09-05**, as #206-#217 for the twelve tickets plus #218
(#159) and #219 (#122): 92.8% -> **94.2%** line, 81.3% -> **87.5%** branch, 584 -> 628 tests in 87
-> 96 classes. Missed lines 169 -> 138, missed branches 251 -> 167.
**Its branch move is a different animal from the 2026-08-29 seam work's, and the difference is the
point.** That one gained 6.3 branch points with the numerator up 37 (974 -> 1011) while the
denominator *fell* 70 (1410 -> 1340) — much of the rise was scaffolding leaving the measurement
rather than arms being covered. Here the numerator is up **80** (1091 -> 1171) and
the denominator moved **-4** (1342 -> 1338). So this one is almost entirely tests choosing arms
nothing had chosen, which is what the entry above warns to check before quoting a branch figure.
The line denominator rose the other way, 2352 -> 2372, and that is new production code rather than
untested code: the seams the wave cut — `capabilitiesFrom`, `ffprobeInfoFrom`, `sessionOutcome`,
and `sweepScope`/`startupSweep`.
**The two-filter method above is what found the work**, and its second filter earned its place:
the largest single gap of the wave (#192, the Cancel button never shown to reach WorkManager) sits
on lines that were already green and no line-level filter could see it.
One result worth carrying forward about *evidence* rather than coverage. #218 fixed a flake whose
reproduction is statistical, and running the whole suite six times per arm caught nothing either
way — at the observed rate a clean six-run arm is roughly a coin flip, so the comparison was
underpowered and proved nothing. What settled it was a deterministic mutation, and then the merge
train confirmed it by accident: the race reproduced on #217's Unit tests leg, which sits below
#218 and carries the unfixed scope. **Prefer a mutation that must go red to a repetition count**
when a fix is for something intermittent.
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
a change that is both needs both.
@@ -410,4 +437,29 @@ Because versions float, a build can change without a commit. `./gradlew :app:dep
run instead is `timeout` on the `Test` tasks plus the jstack watchdog beside it in
`app/build.gradle.kts`, neither of which moves a thread. `HangBoundTest` guards both numbers,
and **a timed-out run writes no XML for the class that hung** — the dump is its only
attribution, so do not delete the watchdog as stray config.
attribution, so do not delete the watchdog as stray config. It has since been exercised in anger:
on 2026-09-05 it caught #125's Room/WorkManager deadlock on CI, failing in 10m57s with the hung
test named, where that ticket had predicted a 60-minute cap and no cause. #125 is closed as
bounded on the strength of it — the inversion itself is internal to the two libraries and still
live at `work-runtime` 2.11.2 / `room` 2.7.0.
- **The JVM suite does not run `LibreMediaConverterApp`.** `app/src/test/resources/robolectric.properties`
names `TestLibreMediaConverterApp` for every test, and it differs from the real class in exactly
one thing: `sweepScope` is `Dispatchers.Unconfined`, so the startup staging sweep finishes before
`onCreate()` returns instead of running on `Dispatchers.IO`.
**That line is load-bearing — do not delete it as stray config.** Robolectric builds an
`Application` per test class that asks for one, and each `onCreate` launched a sweep over the
shared `<cacheDir>/conversions/` that nothing joined. So a test asserting about a staged file was
racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4
added ten Robolectric classes, at which point `OutputPublisherStagingTest` failed on roughly one
local run in six. Per-test opt-in was measured and rejected: **27 of the 58 Robolectric classes
touch that directory**. The `SupervisorJob` is kept in the test scope so a throwing sweep is
swallowed there exactly as in production — the dispatcher is the only intended difference.
**It cost one assertion, knowingly.** `AppStartSweepTest` used to open by asserting that the
manifest's `android:name` is what Robolectric instantiated, so the sweep is code that actually
runs. An `application=` override *replaces* the manifest rather than being checked against it, and
`applicationInfo.className` reports the override too — measured — so that claim is not merely
unasserted on the JVM now, it is unobservable, and a rewritten version would assert the override
against itself. **The manifest link is device-only.** What remains is the `as LibreMediaConverterApp`
cast in that class's `setUp`, which catches only the test app ceasing to extend the real one.
@@ -10,6 +10,9 @@ import androidx.compose.ui.test.performClick
import androidx.media3.common.util.UnstableApi
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
import androidx.test.runner.lifecycle.ActivityLifecycleCallback
import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry
import androidx.test.runner.lifecycle.Stage
import androidx.test.uiautomator.By
import androidx.test.uiautomator.BySelector
import androidx.test.uiautomator.Configurator
@@ -24,6 +27,7 @@ import org.junit.runner.RunWith
import org.libremediaconverter.FailsOnEmulatorApi37
import org.libremediaconverter.MainActivity
import org.libremediaconverter.ui.TestTags
import java.util.concurrent.atomic.AtomicInteger
/**
* Choosing a file, through the real system picker, and still having it after a rotation.
@@ -251,6 +255,21 @@ class SafPickerRoundTripTest {
/** Set by the one test that rotates, read by [restoreOrientation]. See its KDoc. */
private var rotated = false
/** Counts [MainActivity] creations from the moment [watchForRecreation] is called. */
private val recreations = AtomicInteger()
/**
* Counts a rotation's recreation without asking the Activity anything.
*
* Deliberately not `composeRule.activity`, which resolves through `scenario.onActivity` and so
* blocks on the main thread. Polling *that* across a recreation is a plausible reading of the
* 20-minute wedges in #122, which would make the obvious barrier the bug it is meant to fix.
* The runner's lifecycle monitor is a callback: reading the counter touches no looper.
*/
private val recreationWatcher = ActivityLifecycleCallback { activity, stage ->
if (activity is MainActivity && stage == Stage.CREATED) recreations.incrementAndGet()
}
/**
* Leave the device the way it was found — and only if this test moved it.
*
@@ -270,6 +289,7 @@ class SafPickerRoundTripTest {
*/
@After
fun restoreOrientation() {
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
if (!rotated) return
device.setOrientationNatural()
device.unfreezeRotation()
@@ -303,9 +323,11 @@ class SafPickerRoundTripTest {
// The identity hash rather than the Activity itself, so nothing here keeps a destroyed
// Activity reachable across the recreation it is being used to detect.
val before = System.identityHashCode(composeRule.activity)
watchForRecreation()
device.setOrientationLandscape()
rotated = true
awaitRecreation()
composeRule.waitForIdle()
// Two guards before the assertion that matters, because both of the ways this test could
@@ -675,6 +697,36 @@ class SafPickerRoundTripTest {
* `Condition still not satisfied after 30000 ms` — which names neither the node nor the test.
* With the description it says which affordance never arrived, which is the whole finding.
*/
/** Starts counting [MainActivity] creations, so [awaitRecreation] can wait for the next one. */
private fun watchForRecreation() {
recreations.set(0)
ActivityLifecycleMonitorRegistry.getInstance().addLifecycleCallback(recreationWatcher)
}
/**
* Waits for the rotation to actually rebuild [MainActivity], which `waitForIdle` does not.
*
* **This is #122.** `waitForIdle()` waits for the compose hierarchy to settle. Immediately
* after a rotation the window manager has accepted but not yet delivered as a configuration
* change, the *old* Activity's composition is already idle — so it returns, `composeRule
* .activity` still resolves to the old instance, and the guard below reads an unchanged
* identity hash. That is the clean `AssertionError` seen on the API 33 gating leg of #217, and
* the wedges on #122 are the same race taken the other way: land while the composition is
* being torn down and there is nothing coherent for `waitForIdle` to settle on.
*
* A bounded wait is worth having even if that second half is wrong. It turns a 20-minute
* `WEDGE_TIMEOUT` — which costs the leg and names no test — into a fast failure that says which
* test and what it was waiting for.
*/
private fun awaitRecreation() {
composeRule.waitUntil(
"the rotation did not recreate MainActivity within $RECREATION_TIMEOUT_MS ms",
RECREATION_TIMEOUT_MS,
) {
recreations.get() > 0
}
}
private fun awaitNode(tag: String) {
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
@@ -703,6 +755,15 @@ class SafPickerRoundTripTest {
*/
const val REOPENED_TIMEOUT_MS = 10_000L
/**
* How long a rotation is given to destroy and rebuild the Activity.
*
* Generous against the API 33 and 34 emulators #122 was measured on, where the rotation is
* slow enough for the gap this bound exists to cover to be observable at all — and still
* two orders of magnitude inside the 1200 s `WEDGE_TIMEOUT` it replaces.
*/
const val RECREATION_TIMEOUT_MS = 15_000L
/**
* How long the app is given to take the window focus back after a back press.
*
@@ -3,6 +3,7 @@ package org.libremediaconverter
import android.app.Application
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch
import org.libremediaconverter.convert.OutputPublisher
@@ -17,14 +18,36 @@ import org.libremediaconverter.convert.OutputPublisher
* ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the
* Activity. Process start is the one moment those leftovers are reliably observable.
*/
class LibreMediaConverterApp : Application() {
open class LibreMediaConverterApp : Application() {
/**
* Deliberately process-lifetime and never cancelled: the work it carries is a single
* short task that should outlive nothing in particular and be interrupted by nothing.
* A `SupervisorJob` so a failure here could never take a sibling down with it.
*
* **`protected open` for #159.** Robolectric builds an `Application` for every test that asks
* for one, so on the JVM this is not one background sweep but one *per test* — all of them on
* `Dispatchers.IO`, all touching the same `cacheDir`, none of them joined by anything. That is
* a race against any test asserting about a file under `conversions/`, and it grew with the
* suite: wave 4 added ten Robolectric classes and took it from CI-only to roughly one local run
* in six. The JVM suite substitutes a scope that runs the sweep inline — see
* `app/src/test/resources/robolectric.properties` and `TestLibreMediaConverterApp`.
*
* A constructor parameter would be the ordinary way to inject this and is not available: the
* framework builds this class, so the seam has to be a member.
*/
private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
protected open val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
/**
* The sweep [onCreate] last started, so a caller that needs it finished can wait for it.
*
* Nothing in production reads this — process start does not wait for its own housekeeping. It
* exists because the alternative for a test is a timed poll, and a poll cannot tell "the sweep
* has not run yet" from "the sweep ran and did nothing".
*/
@Volatile
var startupSweep: Job? = null
private set
override fun onCreate() {
super.onCreate()
@@ -53,6 +76,6 @@ class LibreMediaConverterApp : Application() {
//
// sweepStaging() also re-reads each timestamp immediately before deleting, which
// closes the window between listing the directory and acting on the listing.
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
startupSweep = sweepScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
}
}
@@ -1,8 +1,8 @@
package org.libremediaconverter
import org.junit.Assert.assertEquals
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@@ -10,7 +10,6 @@ import org.libremediaconverter.convert.StagingSweep
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.concurrent.TimeUnit
/**
* That process start actually sweeps.
@@ -23,8 +22,19 @@ import java.util.concurrent.TimeUnit
* output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity.
*
* `onCreate()` is called again rather than a second Application being built: it is what the
* framework calls at process start, the scope it launches on is already there, and the first test
* below is what pins that the framework calls it on *this* class.
* framework calls at process start, and the scope it launches on is already there.
*
* **What this class stopped covering in #159, deliberately.** It used to open by asserting that
* `RuntimeEnvironment.getApplication()` is a [LibreMediaConverterApp] — that the manifest's
* `android:name` points here, so the sweep is code that actually runs. That assertion cannot exist
* on the JVM any more: `robolectric.properties` now names [TestLibreMediaConverterApp] for the
* whole suite, and an `application=` override replaces the manifest rather than being checked
* against it — `applicationInfo.className` reports the override too, measured. So the manifest is
* not merely unasserted here, it is unobservable from this source set, and a rewritten version of
* that test would have asserted the override against itself. **The manifest link is a device-only
* guarantee now**, and it was traded knowingly for the race that override fixes. The cast in
* [setUp] still fails if [TestLibreMediaConverterApp] stops extending the real class, which is a
* smaller claim than the one withdrawn.
*/
@RunWith(RobolectricTestRunner::class)
class AppStartSweepTest {
@@ -34,17 +44,35 @@ class AppStartSweepTest {
@Before
fun setUp() {
// The cast is an assertion in itself: Robolectric builds the Application named in the
// merged manifest, so this fails if `android:name` ever stops pointing here -- in which
// case the sweep below would be perfectly correct code that never runs.
app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp
stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() }
stagingDir.listFiles()?.forEach { it.delete() }
}
/**
* The property the whole substitution exists for, asserted directly rather than waited on.
*
* #159 is not "the sweep is slow", it is "the sweep is still running while some later test
* reads the directory". [TestLibreMediaConverterApp] answers that by finishing the sweep before
* `onCreate()` returns, and this is the only place that claim is checked -- every other test in
* the suite benefits from it silently and would go back to racing without saying why.
*
* Deterministic in the direction that matters: `Dispatchers.Unconfined` runs a `launch` whose
* body never suspends to completion inline, so this cannot flake green-to-red. Putting the test
* app back on `Dispatchers.IO` makes it a race that the assertion loses essentially every time,
* which is what a six-run suite comparison could not show -- at the rate #159 was observed at,
* a clean six-run arm is a coin flip.
*/
@Test
fun `the application the manifest starts is the one that sweeps`() {
assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass)
fun `the sweep is finished before onCreate returns`() {
app.onCreate()
val sweep = app.startupSweep
assertNotNull("onCreate() started no sweep", sweep)
assertTrue(
"the JVM suite's sweep outlived onCreate(), so it is in flight during test bodies again",
sweep?.isCompleted == true,
)
}
@Test
@@ -64,35 +92,23 @@ class AppStartSweepTest {
app.onCreate()
awaitGone(abandoned)
// Joined rather than polled. `onCreate` publishes the sweep it started, so this waits for
// that exact sweep -- where a timed poll could not tell "swept" from "not started yet", and
// answered the second case by failing after ten seconds.
val sweep = app.startupSweep
assertNotNull("onCreate() started no sweep to wait for", sweep)
runBlocking { sweep?.join() }
assertTrue("process start left ${abandoned.name} in staging; nothing swept it", !abandoned.exists())
// The other half, and the one that says the sweep is a sweep rather than a
// `clearStaging()`: the directory is shared by the convert tab, the join tab and
// ConcatEngine's list file, so deleting everything could take a file from a running job.
assertTrue("a file written moments ago belongs to a live job", live.exists())
}
/**
* Waits for [file] to be deleted.
*
* The sweep runs on `Dispatchers.IO`, deliberately: it lists a directory and stats every entry
* on the path that decides how long the launcher icon stays unresponsive. So there is nothing
* to join, and the wait is a bounded poll — long enough for a directory listing, short enough
* that a sweep which never happens fails rather than hangs.
*/
private fun awaitGone(file: File) {
val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS)
while (System.nanoTime() < deadline) {
if (!file.exists()) return
Thread.sleep(POLL_INTERVAL_MS)
}
fail("process start left ${file.name} in staging; nothing swept it")
}
private fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) }
private companion object {
const val ONE_MINUTE_MS = 60L * 1000
const val AWAIT_TIMEOUT_SECONDS = 10L
const val POLL_INTERVAL_MS = 5L
}
}
@@ -0,0 +1,28 @@
package org.libremediaconverter
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
/**
* The [LibreMediaConverterApp] the JVM suite runs, differing from it in exactly one thing: the
* startup sweep runs inline on the thread that builds the Application instead of on
* `Dispatchers.Unconfined`.
*
* **This is #159.** Robolectric builds an `Application` per test class that asks for one, and each
* one launches a sweep over the shared `<cacheDir>/conversions/`. Nothing joins them, so a test
* asserting about a staged file is racing however many sweeps the classes before it left in
* flight — `OutputPublisherStagingTest` being the one that lost, at roughly one local run in six
* once wave 4 added ten more Robolectric classes. Making the sweep finish before `onCreate()`
* returns removes the race for every test at once rather than asking each to opt in; 27 of the
* suite's 58 Robolectric classes touch that directory, so opting in was not a real option.
*
* `Dispatchers.Unconfined` is what makes it inline: `sweepStaging()` is a plain function, so an
* `Unconfined` `launch` runs it to completion before returning. The `SupervisorJob` is kept so this
* differs from production in the dispatcher alone — a sweep that throws is logged and swallowed
* here exactly as it is there, rather than taking Application construction down with it and failing
* every test in the class for an unrelated reason.
*/
class TestLibreMediaConverterApp : LibreMediaConverterApp() {
override val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined)
}
@@ -122,24 +122,25 @@ class OutputPublisherStagingTest {
/**
* Makes `cacheDir/conversions` a regular file, which is the whole precondition of the test
* above -- and does it in a loop, because a single delete-then-write loses a race that CI
* caught and this machine does not reproduce.
* above -- and does it in a loop, because a single delete-then-write once lost a race that CI
* caught and this machine did not reproduce.
*
* `LibreMediaConverterApp.onCreate` ends with
* `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`, and
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric instantiates
* the application for every test that asks for one, so that background `mkdirs()` is in flight
* across the whole suite, on a thread the paused main looper does not control. Between deleting
* this path and writing it there is a window where the path does not exist and that `mkdirs()`
* can win, which is `FileNotFoundException: ... (Is a directory)` out of `writeBytes` -- run
* 33069641674 on #149, once, against 468 tests that pass here.
* **That race is closed at the source as of #159, and the loop is kept anyway.**
* `LibreMediaConverterApp.onCreate` launched its staging sweep on `Dispatchers.IO`, and
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric builds an
* application for every test class that asks for one, so that background `mkdirs()` was in
* flight across the whole suite, on a thread the paused main looper does not control. Between
* deleting this path and writing it there is a window where the path does not exist and that
* `mkdirs()` could win -- `FileNotFoundException: ... (Is a directory)` out of `writeBytes`,
* run 33069641674 on #149, once, against 468 tests that passed here. The JVM suite now runs
* `TestLibreMediaConverterApp`, whose sweep finishes before `onCreate()` returns, so nothing is
* sweeping while a test body runs.
*
* Retrying closes it rather than narrowing it, because the race is not symmetric: `mkdirs()`
* fails on an existing regular file, so the invariant only has to survive being *established*.
* Once a write lands, nothing in the suite can turn this back into a directory.
*
* The wider problem -- application-scope IO work racing every Robolectric test that shares
* `cacheDir` -- is #159, and is deliberately not fixed here.
* The loop stays because it is what would catch that substitution being undone. Without it the
* regression returns as this one class failing rarely on CI -- the exact shape that took #159
* from a single run on #149 to a wave-4 flake before anyone chased it. Retrying closes the
* window rather than narrowing it, because the race is not symmetric: `mkdirs()` fails on an
* existing regular file, so the invariant only has to survive being *established*.
*/
private fun stagingPathAsRegularFile(): File {
val stagingPath = File(cacheDir, "conversions")
@@ -10,3 +10,9 @@
# Set here rather than in a @Config on each class so a later Robolectric test does not have
# to rediscover it. Remove it once Robolectric ships an android-all jar for 37.
sdk=36
# Every test gets TestLibreMediaConverterApp, whose only difference from the real one is that the
# startup sweep runs inline rather than on Dispatchers.IO. Set suite-wide because the race it fixes
# (#159) is suite-wide: any class that builds an Application leaves a sweep of the shared staging
# directory in flight for whatever runs next. TestLibreMediaConverterApp explains the choice.
application=org.libremediaconverter.TestLibreMediaConverterApp