Compare commits
33
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
98c0e4dba2 | ||
|
|
cf540f1ecc | ||
|
|
948d53b67e | ||
|
|
54932e97c6 | ||
|
|
ba16f5a89b | ||
|
|
bffcff92c7 | ||
|
|
9f06eb9988 | ||
|
|
06ca167034 | ||
|
|
c757565d64 | ||
|
|
4b02294cfb | ||
|
|
79097a0256 | ||
|
|
6004398a83 | ||
|
|
a354620bf5 | ||
|
|
17c91081cd | ||
|
|
20f718842d | ||
|
|
dec7089b59 | ||
|
|
b677a9ad02 | ||
|
|
34e4ab52a4 | ||
|
|
1437157a8f | ||
|
|
1041faf920 | ||
|
|
fe68f839c1 | ||
|
|
f65578b1f7 | ||
|
|
b38ad6a683 | ||
|
|
9a0f494e26 | ||
|
|
f3478706b3 | ||
|
|
61c400d2c6 | ||
|
|
d83775d5c6 | ||
|
|
e90f5a801c | ||
|
|
68015b3374 | ||
|
|
32ab54da3c | ||
|
|
e7caeeac43 | ||
|
|
ec2cae256f | ||
|
|
4e88de3045 |
@@ -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,58 @@ 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.
|
||||
|
||||
**Every number above is `testDebugUnitTest` only, and on 2026-09-05 the instrumented suite got its
|
||||
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E6**, tickets
|
||||
**#223-#230**. Four waves had been steered by a figure that **cannot see `app/src/androidTest` at
|
||||
all**, so nothing had ever asked what those 60 device tests pin, only that they were green.
|
||||
|
||||
**It found one test that passes while testing nothing, and it is the one that matters most.**
|
||||
`HardwareFallbackTest` is the only automated check of the hardware→software fallback against a
|
||||
*real* codec failure, and on run `34004304566` the API 33, 34, 35 and 37 legs each log
|
||||
`Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)` (API 36's logcat artifact on
|
||||
that run is truncated, so it is unread rather than different): emulators expose no
|
||||
hardware encoder, so the job never reaches Media3 and the `catch` it exists to prove is never
|
||||
entered. Its two assertions — succeeded, output non-empty — are true anyway, and it finishes in
|
||||
448 ms. **Deleting that `catch` reddens nothing on any leg** (#223).
|
||||
|
||||
Two things generalise from it. **A test can assert and still not reach**, which no coverage
|
||||
number and no "does it assert something" review would catch — the filter that works is *does this
|
||||
test's premise hold on the machine that runs it?*. And the codebase **already knew**: the sibling
|
||||
`ForcedFailureTest` pins `DeviceCodecs.PERMISSIVE` against exactly this hazard and writes out why,
|
||||
as does `ConversionWorkerTest`. The difference is that their assertions are about the *path*, so
|
||||
without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it
|
||||
passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ.
|
||||
|
||||
The read was a triage, not a test push, and five of its six findings are prose rather than code —
|
||||
the suite itself is in good shape. What had drifted is its self-description.
|
||||
|
||||
- **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 +462,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.
|
||||
|
||||
@@ -12,11 +12,17 @@ import kotlinx.coroutines.withTimeout
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Assume.assumeTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.codec.AndroidDeviceCodecs
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
import org.libremediaconverter.model.ConversionRouter
|
||||
import org.libremediaconverter.model.Engine
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
import org.libremediaconverter.model.QualityTier
|
||||
import org.libremediaconverter.model.VideoCodec
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import java.io.File
|
||||
|
||||
@@ -34,6 +40,47 @@ import java.io.File
|
||||
* hand — a regression test that silently skips is worse than no test, because the count
|
||||
* still reads as coverage.
|
||||
*
|
||||
* ## Why this skips on emulators, and why that is the honest answer (#223)
|
||||
*
|
||||
* **This test used to pass everywhere while proving nothing.** Two independent facts stop the
|
||||
* fallback happening on an emulator, and both were measured rather than reasoned:
|
||||
*
|
||||
* 1. **The router never sends the job to Media3.** A Fast MP4/H.265 job goes to the hardware path
|
||||
* only when `device.canEncode(H265)`, and emulators expose no hardware encoder — every leg of
|
||||
* run `34004304566` logged
|
||||
* `Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)`. The whole test
|
||||
* finished in 448 ms, which is not long enough to fail an export and then re-encode.
|
||||
* 2. **Forcing it to Media3 does not help either, which is the part that settles it.** Pinning
|
||||
* `ConversionDependencies.deviceCodecs` to [DeviceCodecs.PERMISSIVE] — the trick
|
||||
* [ForcedFailureTest] uses — makes the router choose Media3, and the export then *succeeds*.
|
||||
* Measured on a local API 34 emulator: `MediaCodecInfo` logs
|
||||
* `NoSupport [codec.profileLevel, avc1.F4000C, video/avc]` for **both**
|
||||
* `c2.goldfish.h264.decoder` and `c2.android.avc.decoder`, and ExoPlayer allocates the
|
||||
* goldfish decoder anyway, which decodes the file regardless of the profile it declares.
|
||||
* `c2.android.hevc.encoder` then encodes the result and the job reports `MEDIA3`.
|
||||
*
|
||||
* So the class KDoc above — "Media3 fails partway through the export on every device" — **is not
|
||||
* true of the emulator images**, and no amount of routing pressure makes this fixture force a
|
||||
* fallback there. The emulator cannot answer this question, so the test says so out loud instead
|
||||
* of passing.
|
||||
*
|
||||
* That is why the gate is [assumeTrue] on the *production* premise (`canEncode(H265)`) rather than
|
||||
* a pinned profile: pinning would also swap in software codecs, which is not the path a real
|
||||
* device takes and is what made the forced run succeed. **This is now the third permanent skip**;
|
||||
* the other two are [org.libremediaconverter.bench.RealMediaBenchmark]'s.
|
||||
*
|
||||
* `ForcedFailureTest.hardwareFailureFallsBackToSoftware` still covers the fallback *wiring* on
|
||||
* every leg, with an `ExplodingHardware` double. What only a device with a real hardware encoder
|
||||
* can show is two real engines disagreeing about a real file, and that is what this is for.
|
||||
*
|
||||
* ## Why the assertion is a pair
|
||||
*
|
||||
* `KEY_ENGINE_USED` is `FFMPEG` whether the fallback fired **or** the router went straight there,
|
||||
* so asserting it alone would not have caught any of the above. The premise is asserted
|
||||
* separately: [ConversionRouter.route] chooses `MEDIA3` for this request on this device. Static
|
||||
* routing wanted hardware, the runtime result was software — together, and only together, that is
|
||||
* the fallback.
|
||||
*
|
||||
* The fixture was produced with x264, which the host toolchain cannot do (Fedora's
|
||||
* ffmpeg ships openh264, which is Constrained Baseline only):
|
||||
*
|
||||
@@ -66,6 +113,15 @@ class HardwareFallbackTest {
|
||||
|
||||
@Test
|
||||
fun aFileMedia3CannotDecodeStillConvertsViaFfmpeg(): Unit = runBlocking {
|
||||
// See "Why this skips on emulators" on the class. Without a real hardware encoder the
|
||||
// router never chooses Media3, and forcing it makes the export succeed instead of fail --
|
||||
// so there is no fallback to observe and a green run would mean nothing.
|
||||
assumeTrue(
|
||||
"no hardware HEVC encoder, so the router cannot choose Media3 and there is no " +
|
||||
"fallback to exercise",
|
||||
AndroidDeviceCodecs.get().canEncode(VideoCodec.H265),
|
||||
)
|
||||
|
||||
val request = ConversionWorker.request(
|
||||
inputUri = Uri.fromFile(input),
|
||||
displayName = SAMPLE,
|
||||
@@ -75,6 +131,19 @@ class HardwareFallbackTest {
|
||||
// the tier where the fallback has to rescue the conversion.
|
||||
quality = QualityTier.FAST,
|
||||
)
|
||||
// The premise, asserted rather than assumed: this request is one the router wants to send
|
||||
// to hardware on this device. Without it the test is green whether the fallback fired or
|
||||
// the job never went near Media3, which is exactly how #223 stayed invisible.
|
||||
val decision = ConversionRouter.route(
|
||||
ConversionRequest(OutputFormat.MP4_H265.spec, quality = QualityTier.FAST),
|
||||
AndroidDeviceCodecs.get(),
|
||||
)
|
||||
assertEquals(
|
||||
"this test only means something if the router sends this job to Media3",
|
||||
Engine.MEDIA3,
|
||||
decision.engine,
|
||||
)
|
||||
|
||||
workManager.enqueue(request).result.get()
|
||||
|
||||
val terminal = withTimeout(TIMEOUT_MS) {
|
||||
@@ -88,6 +157,14 @@ class HardwareFallbackTest {
|
||||
terminal?.state,
|
||||
)
|
||||
|
||||
// The outcome. Paired with the routing assertion above this is the fallback and nothing
|
||||
// else: hardware was chosen, software is what ran.
|
||||
assertEquals(
|
||||
"the router chose Media3, so a successful job must have fallen back to FFmpeg",
|
||||
Engine.FFMPEG.name,
|
||||
terminal?.outputData?.getString(ConversionWorker.KEY_ENGINE_USED),
|
||||
)
|
||||
|
||||
val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!)
|
||||
assertTrue("no output produced", out.exists() && out.length() > 0)
|
||||
out.delete()
|
||||
|
||||
@@ -4,7 +4,16 @@ import android.media.MediaExtractor
|
||||
import android.media.MediaFormat
|
||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||
import androidx.test.platform.app.InstrumentationRegistry
|
||||
import com.arthenica.ffmpegkit.FFmpegKit
|
||||
import com.arthenica.ffmpegkit.FFmpegSession
|
||||
import com.arthenica.ffmpegkit.ReturnCode
|
||||
import com.arthenica.ffmpegkit.SessionState
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.cancelAndJoin
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
@@ -113,6 +122,11 @@ class FFmpegEngineTest {
|
||||
fun encodesFlacLosslessAudio() {
|
||||
val out = convert(OutputFormat.FLAC)
|
||||
assertTrue("no FLAC produced", out.exists() && out.length() > 0)
|
||||
// "fLaC", the native FLAC stream marker. Without this the test passed on any non-empty
|
||||
// file, so a builder arm emitting the wrong encoder into a .flac name shipped green
|
||||
// (#228) -- the same shape the five assertions above already guard against.
|
||||
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
|
||||
assertEquals("fLaC", magic)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -127,6 +141,141 @@ class FFmpegEngineTest {
|
||||
fun encodesOpus() {
|
||||
val out = convert(OutputFormat.OPUS)
|
||||
assertTrue("no Opus produced", out.exists() && out.length() > 0)
|
||||
// OutputFormat.OPUS is Container.OGG, so the file is an Ogg stream: "OggS" (#228).
|
||||
// Deliberately the container marker rather than the codec -- it is what the other
|
||||
// container-level assertions in this class check, and it is four bytes at offset 0.
|
||||
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
|
||||
assertEquals("OggS", magic)
|
||||
}
|
||||
|
||||
/**
|
||||
* The percentage itself, which every other test in this class computes and none of them reads.
|
||||
*
|
||||
* `FFmpegEngine` derives progress as `stats.time / durationMs * 100`, and the statistics
|
||||
* callback runs on every conversion here — but every call site omits `onProgress`, so until
|
||||
* this test nothing on any source set had ever looked at the number (#229). #196 covered the
|
||||
* *worker's* progress lambda, and did it with a fake engine that reports whatever the test
|
||||
* tells it to; `ProgressNotificationTest` covers throttling the same way. The arithmetic was
|
||||
* the one part with no reader.
|
||||
*
|
||||
* ## Why the duration is deliberately wrong
|
||||
*
|
||||
* `sample_h264.mp4` is exactly 3.000 s, and this passes **30 s** as the duration. So the
|
||||
* conversion still encodes the whole clip, `stats.time` still climbs to about 3000 ms, and the
|
||||
* reported percentage tops out around **10** rather than 100.
|
||||
*
|
||||
* That is what makes the assertion bite. A range check alone is worthless here: replacing
|
||||
* `percent` with a constant `0` satisfies "every value is in 0..100" and "the values never go
|
||||
* backwards", and so does a list of `[0, 100]`. Pinning the *band* rejects every constant, and
|
||||
* — because the band is a tenth of the way up — it also rejects an implementation that ignores
|
||||
* `durationMs`, which would report ~100 for the same run.
|
||||
*
|
||||
* The bound is deliberately loose (5..25 for an expected 10). The last statistics callback can
|
||||
* land slightly before the final frame, so the peak is "about 3000 ms of a claimed 30 000",
|
||||
* not exactly it.
|
||||
*/
|
||||
@Test
|
||||
fun progressIsReportedAsAFractionOfTheDurationItWasGiven() {
|
||||
val seen = mutableListOf<Int>()
|
||||
val out = outputFor("out_progress.mp4")
|
||||
runBlocking {
|
||||
engine.run(
|
||||
request = ConversionRequest(spec = OutputFormat.MP4_H264.spec, quality = QualityTier.BEST),
|
||||
inputPath = input.absolutePath,
|
||||
output = out,
|
||||
// Ten times the fixture's real 3 s. See the KDoc.
|
||||
durationMs = 30_000,
|
||||
onProgress = { percent -> seen += percent },
|
||||
)
|
||||
}
|
||||
|
||||
assertTrue("the statistics callback never reported progress", seen.isNotEmpty())
|
||||
assertTrue("progress out of range: $seen", seen.all { it in 0..100 })
|
||||
assertEquals("progress went backwards: $seen", seen.sorted(), seen)
|
||||
// The band. Rejects any constant, and rejects ignoring durationMs (which would read ~100).
|
||||
val peak = seen.max()
|
||||
assertTrue(
|
||||
"3 s of media against a claimed 30 s should peak near 10%, got $peak from $seen",
|
||||
peak in 5..25,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Cancelling a *running* conversion actually stops the native session.
|
||||
*
|
||||
* Nothing on any source set did this before (#224). Every `cancel` in `app/src/androidTest` is
|
||||
* `WorkManager.cancelWorkById` against work that is **queued or already finished** — the two in
|
||||
* `ReattachOnLaunchTest` cancel a job carrying a one-hour initial delay, and one immediately
|
||||
* after enqueue. On the JVM, `WorkerCancellationTest` and `HardwareFallbackTest`'s cancellation
|
||||
* case drive a `SoftwareTranscoder` double that records the call. No test had ever asked a real
|
||||
* native session to stop. This is `docs/defect-audit.md` **D10**'s forcing condition.
|
||||
*
|
||||
* It is the one path where cancelling wrong is silently expensive rather than loudly broken: a
|
||||
* missed `FFmpegKit.cancel` leaves the native process encoding to completion while the UI says
|
||||
* the job is cancelled, and nothing reports the battery and thermal cost.
|
||||
*
|
||||
* ## Why the assertion is the session's return code, not the output file
|
||||
*
|
||||
* The obvious assertion — the partial output is gone — **cannot fail**, so it would have been a
|
||||
* vacuous test. `invokeOnCancellation` deletes the path, and on POSIX unlinking a file ffmpeg
|
||||
* still holds open leaves ffmpeg writing to the unlinked inode; the path stays gone whether or
|
||||
* not the cancel ever reached the session. Deleting `FFmpegKit.cancel` and keeping
|
||||
* `output.delete()` passes that check every time.
|
||||
*
|
||||
* What distinguishes them is the session's own verdict: a cancelled session ends with the
|
||||
* cancel return code, a completed one ends successfully. That is a fact about the session
|
||||
* rather than about timing, so it is read *after* waiting for the session to leave
|
||||
* [SessionState.RUNNING] rather than at a fixed delay.
|
||||
*
|
||||
* ## Why it cancels on RUNNING rather than on the first progress callback
|
||||
*
|
||||
* Measured, and this is the part worth keeping. Cancelling from the first `onProgress` was
|
||||
* tried first and **failed on a local API 34 emulator with `state=COMPLETED rc=0`** — every
|
||||
* committed fixture is 2-3 s at 320x240, and the encode finishes before the first statistics
|
||||
* callback has been delivered and acted on. The progress callback is proof the session is
|
||||
* running, but it arrives too late to interrupt anything.
|
||||
*
|
||||
* `FFmpegKit.listSessions` shows the session as [SessionState.RUNNING] far earlier, so that is
|
||||
* what is waited on. `QualityTier.BEST` is deliberate for the same reason: `-preset medium`
|
||||
* leaves more of the encode ahead of the cancel than `veryfast` would.
|
||||
*
|
||||
* The session is identified by diffing against the ids present before the run, because this
|
||||
* class has already produced eight of them by the time this executes.
|
||||
*/
|
||||
@Test
|
||||
fun cancellingARunningConversionCancelsTheNativeSession(): Unit = runBlocking {
|
||||
val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet()
|
||||
val out = outputFor("out_cancelled.mp4")
|
||||
|
||||
val job = launch(Dispatchers.IO) {
|
||||
engine.run(
|
||||
request = ConversionRequest(spec = OutputFormat.MP4_H265.spec, quality = QualityTier.BEST),
|
||||
inputPath = input.absolutePath,
|
||||
output = out,
|
||||
durationMs = 3_000,
|
||||
)
|
||||
}
|
||||
|
||||
// Interrupt as early as the session can be observed at all. See the KDoc: waiting for
|
||||
// progress instead lost the race outright.
|
||||
val ours = withTimeout(TIMEOUT_MS) {
|
||||
var found: FFmpegSession? = null
|
||||
while (found?.getState() != SessionState.RUNNING) {
|
||||
found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before }
|
||||
if (found?.getState() != SessionState.RUNNING) delay(POLL_MS)
|
||||
}
|
||||
found
|
||||
}
|
||||
job.cancelAndJoin()
|
||||
|
||||
withTimeout(TIMEOUT_MS) {
|
||||
while (ours.getState() == SessionState.RUNNING) delay(POLL_MS)
|
||||
}
|
||||
|
||||
assertTrue(
|
||||
"the native session was not cancelled: state=${ours.getState()} rc=${ours.getReturnCode()}",
|
||||
ReturnCode.isCancel(ours.getReturnCode()),
|
||||
)
|
||||
}
|
||||
|
||||
// --- the quality tier the GPL licence was taken for --------------------
|
||||
@@ -166,4 +315,10 @@ class FFmpegEngineTest {
|
||||
}.exceptionOrNull()
|
||||
assertTrue("expected an FFmpegException, got $failure", failure is FFmpegEngine.FFmpegException)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
/** Generous: it bounds a hang, and every wait here normally settles in well under a second. */
|
||||
const val TIMEOUT_MS = 30_000L
|
||||
const val POLL_MS = 50L
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
@@ -203,6 +207,8 @@ import org.libremediaconverter.ui.TestTags
|
||||
* driven there at all. That is why this gap survived as long as it did.
|
||||
* `tools/local-emulator/run-e2e.sh` runs API 33-36 on the development host, and both tests pass
|
||||
* there: **59 / 0 / 0 / 2 at API 33 and again at API 36**, whole suite, 2026-08-24.
|
||||
* (Since #223 the skip column reads 3 on an emulator — `HardwareFallbackTest` now announces
|
||||
* that it cannot run without a hardware HEVC encoder rather than passing vacuously.)
|
||||
*
|
||||
* ### Why only the rotation test carries [FailsOnEmulatorApi37]
|
||||
*
|
||||
@@ -251,6 +257,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 +291,7 @@ class SafPickerRoundTripTest {
|
||||
*/
|
||||
@After
|
||||
fun restoreOrientation() {
|
||||
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
|
||||
if (!rotated) return
|
||||
device.setOrientationNatural()
|
||||
device.unfreezeRotation()
|
||||
@@ -303,9 +325,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 +699,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 +757,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() }
|
||||
}
|
||||
}
|
||||
|
||||
@@ -673,13 +673,23 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
else -> null
|
||||
}
|
||||
|
||||
private fun currentInput(): InputFile? = when (val s = _state.value) {
|
||||
is ConversionState.Ready -> s.input
|
||||
is ConversionState.Converting -> s.input
|
||||
is ConversionState.Waiting -> s.input
|
||||
is ConversionState.Converted -> s.input
|
||||
else -> null
|
||||
}
|
||||
/**
|
||||
* The input `convert()` may act on, which is only ever the one on a `Ready` screen.
|
||||
*
|
||||
* This used to answer for `Converting`, `Waiting` and `Converted` as well. Those arms were not
|
||||
* reachable by tapping Convert -- the button renders only in the `Ready` branch -- but they
|
||||
* were reachable through the POST_NOTIFICATIONS **result**, which `ConverterScreen.kt:91` wires
|
||||
* to `convert()` rather than to the button. Reaching one of them enqueued a *second* job over a
|
||||
* live one: `activeWorkId` was overwritten, and the first job kept running with its foreground
|
||||
* notification orphaned and nothing left holding its id to cancel it.
|
||||
*
|
||||
* Narrowed under #202 rather than tested as it stood, because a test written against the old
|
||||
* shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode.
|
||||
*
|
||||
* `JoinViewModel.join()` has been `(_state.value as? JoinState.Ready)?.inputs ?: return` all
|
||||
* along. The two screens are the same shape and only one of them was over-general.
|
||||
*/
|
||||
private fun currentInput(): InputFile? = (_state.value as? ConversionState.Ready)?.input
|
||||
|
||||
private companion object {
|
||||
/**
|
||||
|
||||
@@ -5,7 +5,6 @@ import android.net.Uri
|
||||
import android.util.Log
|
||||
import com.arthenica.ffmpegkit.FFmpegKit
|
||||
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
||||
import com.arthenica.ffmpegkit.ReturnCode
|
||||
import kotlinx.coroutines.suspendCancellableCoroutine
|
||||
import org.libremediaconverter.convert.ConcatJoiner
|
||||
import org.libremediaconverter.convert.MediaProbe
|
||||
@@ -66,16 +65,16 @@ class ConcatEngine(private val context: Context) : ConcatJoiner {
|
||||
private suspend fun execute(args: List<String>) = suspendCancellableCoroutine { cont ->
|
||||
Log.i(TAG, "ffmpeg ${args.joinToString(" ")}")
|
||||
val session = FFmpegKit.executeWithArgumentsAsync(args.toTypedArray()) { completed ->
|
||||
val rc = completed.getReturnCode()
|
||||
when {
|
||||
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
|
||||
ReturnCode.isCancel(rc) -> cont.cancel()
|
||||
else -> cont.resumeWithException(
|
||||
FFmpegEngine.FFmpegException(
|
||||
"Joining failed (${rc?.value}): " +
|
||||
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(),
|
||||
),
|
||||
)
|
||||
val outcome = sessionOutcome(
|
||||
rc = completed.getReturnCode(),
|
||||
prefix = "Joining",
|
||||
failStackTrace = { completed.getFailStackTrace() },
|
||||
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
|
||||
)
|
||||
when (outcome) {
|
||||
SessionOutcome.Success -> cont.resume(Unit)
|
||||
SessionOutcome.Cancelled -> cont.cancel()
|
||||
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegEngine.FFmpegException(outcome.message))
|
||||
}
|
||||
}
|
||||
cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }
|
||||
|
||||
@@ -4,7 +4,6 @@ import android.util.Log
|
||||
import com.arthenica.ffmpegkit.FFmpegKit
|
||||
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
||||
import com.arthenica.ffmpegkit.Level
|
||||
import com.arthenica.ffmpegkit.ReturnCode
|
||||
import kotlinx.coroutines.suspendCancellableCoroutine
|
||||
import org.libremediaconverter.convert.SoftwareTranscoder
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
@@ -51,19 +50,16 @@ class FFmpegEngine : SoftwareTranscoder {
|
||||
val session = FFmpegKit.executeWithArgumentsAsync(
|
||||
args.toTypedArray(),
|
||||
{ completed ->
|
||||
val rc = completed.getReturnCode()
|
||||
when {
|
||||
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
|
||||
ReturnCode.isCancel(rc) ->
|
||||
cont.cancel()
|
||||
else -> cont.resumeWithException(
|
||||
FFmpegException(
|
||||
"FFmpeg failed (${rc?.value}): " +
|
||||
completed.getFailStackTrace().orEmpty().ifBlank {
|
||||
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty()
|
||||
},
|
||||
),
|
||||
)
|
||||
val outcome = sessionOutcome(
|
||||
rc = completed.getReturnCode(),
|
||||
prefix = "FFmpeg",
|
||||
failStackTrace = { completed.getFailStackTrace() },
|
||||
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
|
||||
)
|
||||
when (outcome) {
|
||||
SessionOutcome.Success -> cont.resume(Unit)
|
||||
SessionOutcome.Cancelled -> cont.cancel()
|
||||
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegException(outcome.message))
|
||||
}
|
||||
},
|
||||
{ log -> Log.d(TAG, log.message.trimEnd()) },
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
package org.libremediaconverter.ffmpeg
|
||||
|
||||
import com.arthenica.ffmpegkit.ReturnCode
|
||||
|
||||
/**
|
||||
* What a finished FFmpegKit session means, as a function of its return code.
|
||||
*
|
||||
* Both engines had their own copy of this `when`, twelve lines apart in two files, and the copies
|
||||
* had drifted: [FFmpegEngine] preferred the fail stack trace and fell back to the log tail, while
|
||||
* [ConcatEngine] only ever read the log tail. Neither was tested — both live inside a callback
|
||||
* handed to `FFmpegKit`, which does not run on the JVM — so the divergence was invisible.
|
||||
*
|
||||
* #203 decided to unify on the stack trace, so a join failure now carries the diagnostics a
|
||||
* conversion failure always did. The *prefix* stays per-engine: unifying the strategy must not
|
||||
* unify the sentence, since "FFmpeg failed" and "Joining failed" describe different jobs.
|
||||
*/
|
||||
internal sealed interface SessionOutcome {
|
||||
|
||||
/** rc 0. The suspension resumes normally. */
|
||||
data object Success : SessionOutcome
|
||||
|
||||
/** rc 255. The suspension is cancelled rather than failed — the user asked for this. */
|
||||
data object Cancelled : SessionOutcome
|
||||
|
||||
/** Anything else, with the sentence the user is shown. */
|
||||
data class Failed(val message: String) : SessionOutcome
|
||||
}
|
||||
|
||||
/**
|
||||
* Maps a return code onto the outcome, and builds the failure sentence when there is one.
|
||||
*
|
||||
* **The two message parts arrive as lambdas, deliberately.** `getAllLogsAsString` and
|
||||
* `getFailStackTrace` are calls onto a native session, and only the failure arm needs either. Taking
|
||||
* them by value would put both on the happy path of every successful conversion, which is a cost the
|
||||
* shape this replaced did not have — the old code read them inside the `else` branch. That is the
|
||||
* same reason [org.libremediaconverter.codec.AndroidDeviceCodecs.capabilitiesFrom] takes a
|
||||
* `Sequence`: a seam should not change what runs when.
|
||||
*
|
||||
* A null [rc] is a real input rather than a defensive one — `getReturnCode()` is nullable, and a
|
||||
* session killed before it reported anything has none. It is neither success nor cancellation, so
|
||||
* it fails, and the sentence says `null` where the number would be.
|
||||
*/
|
||||
internal fun sessionOutcome(
|
||||
rc: ReturnCode?,
|
||||
prefix: String,
|
||||
failStackTrace: () -> String?,
|
||||
logTail: () -> String?,
|
||||
): SessionOutcome = when {
|
||||
ReturnCode.isSuccess(rc) -> SessionOutcome.Success
|
||||
ReturnCode.isCancel(rc) -> SessionOutcome.Cancelled
|
||||
else -> SessionOutcome.Failed(
|
||||
"$prefix failed (${rc?.value}): " + failStackTrace().orEmpty().ifBlank { logTail().orEmpty() },
|
||||
)
|
||||
}
|
||||
@@ -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")
|
||||
|
||||
@@ -0,0 +1,147 @@
|
||||
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 kotlinx.coroutines.Dispatchers
|
||||
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.join.JoinState
|
||||
import org.libremediaconverter.join.JoinViewModel
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
|
||||
/**
|
||||
* An answer that arrives after the screen has moved on does nothing.
|
||||
*
|
||||
* Four refusal arms, cold before this file:
|
||||
*
|
||||
* ```
|
||||
* convert/ConversionViewModel.kt:513 currentInput() ?: return
|
||||
* convert/ConversionViewModel.kt:600 pendingSave() ?: return
|
||||
* join/JoinViewModel.kt:316 (as? Ready)?.inputs ?: return
|
||||
* join/JoinViewModel.kt:390 pendingSave() ?: return
|
||||
* ```
|
||||
*
|
||||
* They are not merely defensive. `ConverterScreen.kt:91` wires `convert()` to the
|
||||
* **POST_NOTIFICATIONS result**, and `:83` wires `save()` to the CreateDocument result — so both
|
||||
* are entered by a system callback rather than by a tap, and a result redelivered after process
|
||||
* death arrives at a brand-new ViewModel sitting on `Idle`.
|
||||
*
|
||||
* ## The production change that came with this
|
||||
*
|
||||
* `currentInput()` used to answer for `Converting`, `Waiting` and `Converted` as well as `Ready`.
|
||||
* Those arms were unreachable by tapping Convert but reachable through that permission callback,
|
||||
* and reaching one enqueued a **second** job over a live one — `activeWorkId` overwritten, the
|
||||
* first job still running with an orphaned notification and nothing holding its id.
|
||||
*
|
||||
* #202 decided to narrow rather than to test it as it stood, because a test written against the old
|
||||
* shape would have frozen the double-enqueue as intended behaviour. `JoinViewModel.join()` has been
|
||||
* `(_state.value as? JoinState.Ready)?.inputs ?: return` all along; the two screens are the same
|
||||
* shape and only one was over-general.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class StaleLauncherResultTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var workManager: WorkManager
|
||||
private lateinit var staged: java.io.File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
val publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
// A real staged file, because a SUCCEEDED job with no output path maps to Failed rather
|
||||
// than Converted -- and Converted is the state this file's second case has to reach.
|
||||
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
installTestWorkManager(
|
||||
app,
|
||||
workDataOf(
|
||||
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
||||
ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mp4",
|
||||
ConversionWorker.KEY_MIME_TYPE to "video/mp4",
|
||||
),
|
||||
)
|
||||
workManager = WorkManager.getInstance(app)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() = ConversionDependencies.reset()
|
||||
|
||||
@Test
|
||||
fun `a permission answer arriving on an empty screen enqueues nothing`() {
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
|
||||
|
||||
viewModel.convert()
|
||||
|
||||
assertEquals(ConversionState.Idle, viewModel.state.value)
|
||||
assertEquals("nothing may be enqueued for a file that is not there", 0, conversionJobs())
|
||||
}
|
||||
|
||||
/**
|
||||
* The narrowing itself: a permission answer that arrives while a conversion is already running
|
||||
* must not start a second one.
|
||||
*
|
||||
* Reached by converting once — the synchronous test WorkManager finishes it inline, so the
|
||||
* screen is `Converted`, which is one of the three arms `currentInput()` used to answer for.
|
||||
* Calling `convert()` again from there is precisely what the permission callback can do.
|
||||
*/
|
||||
@Test
|
||||
fun `a permission answer arriving after the job finished does not start a second one`() {
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
|
||||
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
||||
viewModel.convert()
|
||||
val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
|
||||
assertEquals("the fixture needs exactly one job to start with", 1, conversionJobs())
|
||||
|
||||
viewModel.convert()
|
||||
|
||||
assertEquals("a second job must not be enqueued over the first", 1, conversionJobs())
|
||||
assertEquals("and the screen must not move", converted, viewModel.state.value)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a save answer arriving on an empty screen does nothing`() {
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
|
||||
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
assertEquals(ConversionState.Idle, viewModel.state.value)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join answer arriving on an empty screen enqueues nothing`() {
|
||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
||||
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
|
||||
|
||||
viewModel.join()
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
assertEquals(JoinState.Idle, viewModel.state.value)
|
||||
assertEquals(0, joinJobs())
|
||||
}
|
||||
|
||||
private fun conversionJobs() = jobsTagged(ConversionWorker::class.java.name)
|
||||
|
||||
private fun joinJobs() = jobsTagged(ConcatWorker::class.java.name)
|
||||
|
||||
private fun jobsTagged(tag: String) = workManager.getWorkInfosByTag(tag).get().size
|
||||
|
||||
private companion object {
|
||||
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,127 @@
|
||||
package org.libremediaconverter.ffmpeg
|
||||
|
||||
import com.arthenica.ffmpegkit.ReturnCode
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* What a finished FFmpegKit session means, for both engines at once.
|
||||
*
|
||||
* `FFmpegEngine` and `ConcatEngine` each carried their own copy of this `when`, and the copies had
|
||||
* drifted: one preferred the fail stack trace and fell back to the log tail, the other only ever
|
||||
* read the log tail. Neither was tested, because both live inside a callback handed to `FFmpegKit`,
|
||||
* which does not run on the JVM — so nothing could see that the two disagreed.
|
||||
*
|
||||
* **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar shows
|
||||
* `ReturnCode(int)` as a plain public constructor with `SUCCESS`/`CANCEL` int constants and pure
|
||||
* static `isSuccess`/`isCancel`; its `<clinit>` is constant initialisation and loads no native
|
||||
* library.
|
||||
*
|
||||
* The unification is #203's decision, so the tests pin it as one: a join failure now carries the
|
||||
* stack trace a conversion failure always did, while the two prefixes stay distinct.
|
||||
*/
|
||||
class SessionOutcomeTest {
|
||||
|
||||
@Test
|
||||
fun `a return code of zero is success`() {
|
||||
assertEquals(SessionOutcome.Success, outcome(ReturnCode(ReturnCode.SUCCESS)))
|
||||
}
|
||||
|
||||
/**
|
||||
* Cancellation is a separate outcome from failure, and the distinction is the point: the engines
|
||||
* resume the continuation *cancelled* rather than exceptionally, so a user who pressed Cancel
|
||||
* does not get an error card.
|
||||
*/
|
||||
@Test
|
||||
fun `a return code of 255 is a cancellation, not a failure`() {
|
||||
assertEquals(SessionOutcome.Cancelled, outcome(ReturnCode(ReturnCode.CANCEL)))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `any other return code fails, and the sentence carries the number`() {
|
||||
val failed = outcome(ReturnCode(1), stackTrace = "boom") as SessionOutcome.Failed
|
||||
|
||||
assertTrue("the code belongs in the message, got: ${failed.message}", failed.message.contains("(1)"))
|
||||
}
|
||||
|
||||
/**
|
||||
* The half that was different between the two engines before #203, now the same in both.
|
||||
*/
|
||||
@Test
|
||||
fun `the stack trace is preferred over the log tail`() {
|
||||
val failed = outcome(ReturnCode(1), stackTrace = "the real cause", logTail = "…noise…")
|
||||
as SessionOutcome.Failed
|
||||
|
||||
assertTrue(failed.message.contains("the real cause"))
|
||||
assertTrue("the log tail must not be appended as well", !failed.message.contains("noise"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a blank stack trace falls back to the log tail`() {
|
||||
val blank = outcome(ReturnCode(1), stackTrace = " ", logTail = "the last few lines") as SessionOutcome.Failed
|
||||
val absent = outcome(ReturnCode(1), stackTrace = null, logTail = "the last few lines") as SessionOutcome.Failed
|
||||
|
||||
assertTrue(blank.message.contains("the last few lines"))
|
||||
assertTrue("a null stack trace is a blank one", absent.message.contains("the last few lines"))
|
||||
}
|
||||
|
||||
/**
|
||||
* Both sources empty still has to produce a sentence. A message ending in a dangling colon is
|
||||
* thin, but it is what the user gets when FFmpeg said nothing at all, and it must not be an
|
||||
* exception on the way to the screen.
|
||||
*/
|
||||
@Test
|
||||
fun `a failure with nothing to say still names the code`() {
|
||||
val failed = outcome(ReturnCode(1), stackTrace = null, logTail = null) as SessionOutcome.Failed
|
||||
|
||||
assertEquals("FFmpeg failed (1): ", failed.message)
|
||||
}
|
||||
|
||||
/**
|
||||
* `getReturnCode()` is nullable and a session killed before it reported anything has none.
|
||||
* Neither success nor cancellation, so it fails — and the sentence says so rather than throwing.
|
||||
*/
|
||||
@Test
|
||||
fun `a session with no return code at all fails`() {
|
||||
val failed = outcome(null, logTail = "whatever was logged") as SessionOutcome.Failed
|
||||
|
||||
assertTrue("got: ${failed.message}", failed.message.startsWith("FFmpeg failed (null): "))
|
||||
}
|
||||
|
||||
/**
|
||||
* Unifying the *strategy* must not unify the *sentence*: the two engines describe different
|
||||
* jobs, and a join that reports "FFmpeg failed" is a worse message than the one it replaced.
|
||||
*/
|
||||
@Test
|
||||
fun `each engine keeps its own prefix`() {
|
||||
val join = sessionOutcome(ReturnCode(1), "Joining", { "cause" }, { null }) as SessionOutcome.Failed
|
||||
|
||||
assertTrue(join.message.startsWith("Joining failed (1): "))
|
||||
}
|
||||
|
||||
/**
|
||||
* Neither message source is read unless the outcome is a failure.
|
||||
*
|
||||
* They are calls onto a native session, and reading them on the happy path is work every
|
||||
* successful conversion would do for nothing — which the shape this replaced did not, since it
|
||||
* read them inside the `else` branch. That is why the parameters are lambdas, and this is what
|
||||
* would notice if they stopped being.
|
||||
*/
|
||||
@Test
|
||||
fun `a session that succeeded reads neither the stack trace nor the log`() {
|
||||
var reads = 0
|
||||
fun counted(): String? {
|
||||
reads++
|
||||
return null
|
||||
}
|
||||
|
||||
sessionOutcome(ReturnCode(ReturnCode.SUCCESS), "FFmpeg", ::counted, ::counted)
|
||||
sessionOutcome(ReturnCode(ReturnCode.CANCEL), "FFmpeg", ::counted, ::counted)
|
||||
|
||||
assertEquals("neither source may be touched unless the session failed", 0, reads)
|
||||
}
|
||||
|
||||
private fun outcome(rc: ReturnCode?, stackTrace: String? = null, logTail: String? = null) =
|
||||
sessionOutcome(rc, "FFmpeg", { stackTrace }, { logTail })
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -0,0 +1,327 @@
|
||||
# E2E-read findings
|
||||
|
||||
**Status:** six findings, none fixed, none urgent — **plus one confirmed vacuous test, which is a
|
||||
ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from
|
||||
the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation —
|
||||
something a new test would not fix, because the test already exists and the problem is what it
|
||||
claims rather than what it runs.
|
||||
**Scope:** what reading all 60 instrumented tests turned up that writing a 61st would not fix.
|
||||
**Last verified:** `main` at `4b02294`, 2026-09-05. **60 `@Test` methods in 12 classes**, three
|
||||
carrying `@FailsOnEmulatorApi37`, gating API 37 leg 57.
|
||||
|
||||
## Why this document exists, and why it is separate from the other two
|
||||
|
||||
`docs/coverage-read-findings.md` (`F1`–`F10`) came from reading a **JaCoCo report**, and JaCoCo
|
||||
measures `testDebugUnitTest` only. So four waves of coverage work have been shaped by a number that
|
||||
**cannot see `app/src/androidTest` at all**. The instrumented suite has never had the equivalent
|
||||
read: nothing has asked what those 60 tests actually pin, only that they are green.
|
||||
|
||||
That is the gap this read is in. It is a **triage, not a test push** — the same shape as wave 4's
|
||||
read, which "moved no number at all, and that is its result".
|
||||
|
||||
`docs/defect-audit.md` (`D1`–`D16`) is the record of things *wrong at runtime*. Nothing here is
|
||||
wrong at runtime. These are tests whose names, KDoc or reputation overstate what they execute.
|
||||
|
||||
Entry ids are `E1`–`E6` so they cannot be confused with `F1`–`F10` or `D1`–`D16`.
|
||||
|
||||
## How to read the confidence labels
|
||||
|
||||
Same vocabulary as the other two documents, deliberately:
|
||||
|
||||
- **Confirmed by inspection** — the control flow is fully readable and the finding follows from it.
|
||||
- **Confirmed by measurement** — observed in a CI artifact, with the run id recorded.
|
||||
- **No action** — recorded because it looks like a finding and is not.
|
||||
|
||||
## The method, and the one filter that found everything
|
||||
|
||||
A coverage number is useless here by construction, so the read used a different question, applied
|
||||
to every one of the 60 tests:
|
||||
|
||||
> **If the behaviour this test is named for stopped working, would it go red?**
|
||||
|
||||
Three answers, and only the third is a gap:
|
||||
|
||||
- **yes** — the test bites. Most of the suite.
|
||||
- **no, and that is deliberate and written down** — `RealMediaBenchmark` asserts nothing on purpose
|
||||
(E2); `transcodesH264ToH265AndReportsProgress` declines to assert progress for a stated reason
|
||||
(E3). These are entries here, not tickets.
|
||||
- **no, and nothing says so** — the gap. One test, and it is the most important one in the suite.
|
||||
|
||||
**The reusable part is the second filter**, because "does it assert something?" would have cleared
|
||||
the vacuous test — it asserts two things. What it does not do is *reach the code it names*:
|
||||
|
||||
> **Does the test's own premise hold on the machine that runs it?**
|
||||
|
||||
`HardwareFallbackTest` asserts `SUCCEEDED` and a non-empty output, and both are true of a
|
||||
conversion that never went near the path it exists to prove (**#223**). See
|
||||
[Not covered here](#not-covered-here); it is filed rather than recorded here because a test fixes it.
|
||||
|
||||
---
|
||||
|
||||
## E1 — `RemuxTest`'s class KDoc argues for engine assertions three of its tests do not make, and they are right not to
|
||||
|
||||
**Severity: low · Confirmed by inspection · the KDoc is what is wrong, not the tests**
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:31-42
|
||||
```
|
||||
|
||||
The class KDoc is headed **"Why these assert the engine, not just the file"** and makes a specific
|
||||
argument:
|
||||
|
||||
> A remux routed to FFmpeg produces a perfectly correct file — `-c copy` moves the same samples
|
||||
> into the same container. So an output-only assertion passes whether the hardware transmux path
|
||||
> ran or never executed at all […] which makes "silently always FFmpeg" the most likely way for
|
||||
> this feature to regress.
|
||||
|
||||
Five of its seven tests run a conversion. **Three assert no engine at all:**
|
||||
|
||||
| test | output container | asserts engine? |
|
||||
|---|---|---|
|
||||
| `mkvToMp4RemuxesOnHardware` | MP4 | **yes** — `MEDIA3` |
|
||||
| `mp4ToMkvRemuxesOnFFmpeg` | MKV | **yes** — `FFMPEG` |
|
||||
| `webmToMkvKeepsVp9WithoutReencoding` | MKV | no |
|
||||
| `audioOnlySourceRemuxesIntoMka` | MKV (`.mka`) | no |
|
||||
| `mp4ToMpegTsAndAviProduceTheirOwnContainers` | MPEG-TS, then AVI | **TS only**; the AVI half does not |
|
||||
|
||||
### Why this is not a gap
|
||||
|
||||
`ConversionRouter.MEDIA3_CONTAINERS = setOf(Container.MP4)` (`ConversionRouter.kt:37`), and every
|
||||
one of the three produces MKV or AVI. **They can only ever be FFmpeg**, so the regression the KDoc
|
||||
names — "silently always FFmpeg" — is not a thing that can happen to them. The two tests where the
|
||||
hardware path is genuinely at risk are exactly the two that assert it.
|
||||
|
||||
An engine assertion on the other three would be near-tautological given today's router. It would
|
||||
catch one thing: somebody adding MKV or AVI to `MEDIA3_CONTAINERS` without a muxer to match — which
|
||||
is what `Media3MuxersTest` is for, on the JVM, where it does not need a device.
|
||||
|
||||
### Why it is recorded rather than dropped
|
||||
|
||||
**This was the strongest-looking candidate of the whole read and it dissolved on tracing**, which
|
||||
is the same shape as `F5` in the coverage document (filed as a test gap, and only stopped being one
|
||||
when someone went looking for its callers). Recorded so the next read does not re-file it.
|
||||
|
||||
**The fix is one line of KDoc**, not three tests: the class asserts the engine *where the engine is
|
||||
in doubt*, which is a better rule than the one it currently states.
|
||||
|
||||
---
|
||||
|
||||
## E2 — three of the 60 instrumented tests assert nothing, and two of them never run
|
||||
|
||||
**Severity: n/a · No action — deliberate, documented, and load-bearing as documentation**
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt:25-53
|
||||
```
|
||||
|
||||
`reportDeviceEncoderCapabilities` logs and asserts nothing. `hardwareVersusSoftwareOnRealVideo` and
|
||||
`av1InputRoutesAccordingToDeviceDecodeSupport` are `assumeTrue`-guarded on media that is **not
|
||||
committed** and must be staged by hand into the app's internal `filesDir`, so they skip in every
|
||||
automated run — they are the "2 skipped" every green leg reports, and `docs/local-emulator.md:305`
|
||||
says so.
|
||||
|
||||
The class KDoc is unambiguous: *"This is a benchmark, not part of the automated suite […] Not a
|
||||
correctness test — the assertions are deliberately loose."*
|
||||
|
||||
**No action.** Recorded for one reason: **the suite's headline number is 60, and three of those 60
|
||||
are not tests.** Any future statement of the form "60 instrumented tests cover X" is off by three,
|
||||
and two of the three have never executed on CI at all.
|
||||
|
||||
**It is the opposite of E-nothing, though** — `reportDeviceEncoderCapabilities` runs on every leg
|
||||
and logs `BENCH can-encode:`, and **that log line is what confirmed the vacuous test this read
|
||||
found** (**#223**). An assertion-free test that prints the machine's capabilities turned out to be
|
||||
the only oracle in the suite. See [Not covered here](#not-covered-here).
|
||||
|
||||
---
|
||||
|
||||
## E3 — `transcodesH264ToH265AndReportsProgress` does not assert that progress was reported
|
||||
|
||||
**Severity: low · No action on the test; the name is the inaccurate part**
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt:73, :90-93
|
||||
```
|
||||
|
||||
```kotlin
|
||||
// Deliberately NOT asserting that progress fired. Polling is on a 250 ms tick,
|
||||
// and a 3 s 320x240 clip can finish inside one tick on fast hardware, which
|
||||
// would make the assertion fail intermittently for no real defect.
|
||||
seen.forEach { assertTrue("progress out of range: $it", it in 0..100) }
|
||||
```
|
||||
|
||||
`seen` is empty-safe: `forEach` on an empty list asserts nothing, so replacing `onProgress` with a
|
||||
no-op reddens nothing here. The reasoning is sound and the alternative really is a flaky test.
|
||||
|
||||
**No action on the body.** The name says `AndReportsProgress` and the body says it does not check
|
||||
that, which is the `probeForConcat` shape from `CLAUDE.md` — *a passing test with a wrong
|
||||
explanation is its own failure mode* — in its mildest form, since here the KDoc immediately corrects
|
||||
the name.
|
||||
|
||||
**Contrast the FFmpeg side, which is a real gap and is filed as #229**: `FFmpegEngine`'s percentage
|
||||
arithmetic is executed by every FFmpeg test and observed by none, because every call site omits
|
||||
`onProgress` entirely. Media3's is unasserted; FFmpeg's is unobserved. Only the second is a ticket.
|
||||
|
||||
---
|
||||
|
||||
## E4 — the marker's KDoc says removing it grows the gating leg by two; three tests carry it
|
||||
|
||||
**Severity: low · Confirmed by inspection · one line**
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt:20
|
||||
```
|
||||
|
||||
> Delete the annotation from the tests, and the advisory job goes empty and the gating one grows by
|
||||
> **two**.
|
||||
|
||||
Three tests carry it — `Media3EngineTest:72`, `Media3EngineTest:135`, `SafPickerRoundTripTest:320` —
|
||||
and `FAILS_ON_EMULATOR_API37_BASELINE = 3` eleven lines further down the same file, where the count
|
||||
is machine-checked by `.github/scripts/e2e-report-shape.sh`.
|
||||
|
||||
The third marker was added when the SAF rotation test was excluded; the sentence was not updated
|
||||
with it. **Everything that is checked is consistent at three**; only the prose says two, which is
|
||||
exactly why it drifted — and a good argument for the baseline const being a const.
|
||||
|
||||
---
|
||||
|
||||
## E5 — `coverage-read-findings.md`'s F7 calls covered code uncovered
|
||||
|
||||
**Severity: low · Confirmed by inspection · half of F7 is stale**
|
||||
|
||||
F7 says `probeWithExtractor`'s catch (`MediaProbe.kt:180-182`) is unreachable on Robolectric and
|
||||
"stays device-only", measured across four URI shapes. **The unreachability claim is correct and
|
||||
stands.** The implication readers take from it — that nothing exercises it — does not:
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:111
|
||||
```
|
||||
|
||||
`probeDistinguishesAudioFromImagesFromRubbish` feeds it a file of random bytes and asserts
|
||||
`InputKind.UNPARSEABLE`, on a device, on every gating leg.
|
||||
|
||||
**"Device-only" holds; "uncovered" does not** — and the difference matters, because F7 is one of the
|
||||
six entries that document calls "no action", on the grounds that a test would not help. A test
|
||||
already exists. The entry should say so.
|
||||
|
||||
**This is the failure mode the split between the two documents was meant to prevent**, and it caught
|
||||
this repo out: a JaCoCo-derived document cannot see `androidTest`, so it will keep re-deriving
|
||||
"uncovered" for anything the instrumented suite covers. That is a structural reason for this
|
||||
document to exist, not a one-off correction.
|
||||
|
||||
---
|
||||
|
||||
## E6 — the suite's one device-capability assertion derives its expectation from the call it is testing
|
||||
|
||||
**Severity: low · Confirmed by inspection · no independent oracle exists**
|
||||
|
||||
```
|
||||
app/src/androidTest/java/org/libremediaconverter/work/ConversionWorkerTest.kt:151-152
|
||||
```
|
||||
|
||||
```kotlin
|
||||
val hasHardwareHevc = AndroidDeviceCodecs.get().canEncode(VideoCodec.H265)
|
||||
```
|
||||
|
||||
and then the expectation is `if (hasHardwareHevc) MEDIA3 else FFMPEG`. The test asks
|
||||
`AndroidDeviceCodecs` what to expect and then checks that the router agreed with
|
||||
`AndroidDeviceCodecs`. **If the whole enumeration returned empty, this would still pass** — and
|
||||
empty is precisely what the `runCatching` fallback returns (the reason `#194` was worth cutting;
|
||||
it logs "assuming permissive" while making `canEncode` answer *no* for everything).
|
||||
|
||||
Its KDoc defends the choice, and the defence is good:
|
||||
|
||||
> Asserting MEDIA3 unconditionally tests the test machine, not the router.
|
||||
|
||||
That is true, and there is no third source of truth on a device: `MediaCodecList` is what
|
||||
`AndroidDeviceCodecs` reads, so any oracle built from it is the same oracle.
|
||||
|
||||
**No action, but read it with #223.** It is the same missing oracle that makes the
|
||||
vacuous-test fix a judgement call rather than a one-liner — you cannot assert "this device has
|
||||
hardware HEVC" from inside the suite without asking the class under test. The honest options are a
|
||||
visible skip or a red test, and that decision is the ticket's.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| ID | Finding | Severity | Evidence | Action |
|
||||
|---|---|---|---|---|
|
||||
| E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right |
|
||||
| E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 |
|
||||
| E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it |
|
||||
| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fix the sentence** |
|
||||
| E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not |
|
||||
| E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** |
|
||||
|
||||
**Five of the six are prose, not code**, and that is the shape of this read. The instrumented suite
|
||||
is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation
|
||||
recipes, and the one class that asserts nothing says so in its first line. What this read found is
|
||||
that **the suite's self-description has drifted from the suite** in five small places and one large
|
||||
one.
|
||||
|
||||
**The large one is not in this table**, because a test fixes it: **#223**.
|
||||
|
||||
## Not covered here
|
||||
|
||||
**The vacuous test.** `HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` passes on
|
||||
every CI leg without ever entering the fallback it exists to prove. It is **#223**, not an entry
|
||||
here, because a test fixes it — and it is the reason this read happened rather than an aside from it.
|
||||
|
||||
Measured, not inferred, on run **`34004304566`** (all legs green), from each leg's own
|
||||
`e2e-diagnostics-api*` logcat:
|
||||
|
||||
```
|
||||
I/AndroidDeviceCodecs: Hardware video encoders: []
|
||||
I/RealMediaBenchmark: BENCH can-encode: COPY=true, H264=false, H265=false, VP9=false, VP8=false, AV1=false
|
||||
I/ConversionWorker: Routing sample_h264_444.mp4 -> OutputSpec(container=MP4, videoCodec=H265,
|
||||
audioCodec=AAC) via FFMPEG (NO_HARDWARE_ENCODER)
|
||||
```
|
||||
|
||||
Identical on **API 33, 34, 35 and 37**. (API 36's logcat artifact on that run is truncated to 838 KB
|
||||
and carries no test output at all, so it is unread rather than different.) The job is routed
|
||||
**straight to FFmpeg before Media3 is attempted**, the `catch` in `runMedia3OrFallBack` is never
|
||||
entered, and the test's two assertions — `SUCCEEDED`, output non-empty — are true anyway. It ran in
|
||||
448 ms.
|
||||
|
||||
**The repository already knew.** `ForcedFailureTest.hardwareFailureFallsBackToSoftware`, in the same
|
||||
package, pins `ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }` and says why:
|
||||
|
||||
> most emulators expose no hardware video encoder at all -- so the router would legitimately send
|
||||
> the job straight to FFmpeg and the hardware path would never be attempted. Without this the test
|
||||
> passes on a Pixel and fails on every emulator, which says nothing about the code under test.
|
||||
|
||||
`ConversionWorkerTest.routesAFastMp4JobByDeviceCapability` records the same fact a third time. The
|
||||
knowledge is in two sibling files; `HardwareFallbackTest` is the one that walked into it — and
|
||||
because its assertions are about the *output* rather than the *path*, it passes where
|
||||
`ForcedFailureTest` would have failed. **That asymmetry is why nobody noticed.**
|
||||
|
||||
**State it precisely.** The fallback *wiring* is covered on every leg by `ForcedFailureTest`, with
|
||||
fakes. What has never run on any emulator is a fallback triggered by a **real** mid-export codec
|
||||
failure — which is the case `HardwareFallbackTest` exists for, and the only reason
|
||||
`sample_h264_444.mp4` is committed at all. That fixture, generated with x264 because Fedora's
|
||||
ffmpeg ships openh264 and cannot produce High 4:4:4, does nothing on any CI leg today.
|
||||
|
||||
The fix is not one assertion. `KEY_ENGINE_USED` is `FFMPEG` **whether the fallback fired or the
|
||||
router went straight there** — asserting it changes nothing. The vacuity guard is two facts
|
||||
together: the router chose `MEDIA3` for this request on this device, *and* the worker reported
|
||||
`FFMPEG`. Whether to reach that with `assumeTrue` (a visible skip on emulators, and the "2 skipped"
|
||||
becomes 3) or with an assertion (red on emulators, announcing it cannot test what it claims) is a
|
||||
decision, not a detail — see **E6** for why no third option exists — and **#223** leaves it open.
|
||||
|
||||
**The other e2e gaps this read found are tickets too**, and are not repeated here:
|
||||
|
||||
| # | Gap |
|
||||
|---|---|
|
||||
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg |
|
||||
| **#224** | Cancelling a *running* native session, in any of the three engines |
|
||||
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider`, and the SAF premise it rests on |
|
||||
| **#227** | The notification's Cancel action has never been fired |
|
||||
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file |
|
||||
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere |
|
||||
| **#230** | *(spike)* whether a running conversion's process can be killed under instrumentation |
|
||||
|
||||
**Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is
|
||||
the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before
|
||||
they shipped. #223 is the one that shows why the criterion matters: it has two passing assertions and
|
||||
still tests nothing.
|
||||
@@ -312,6 +312,18 @@ on sample media that is deliberately not committed. Its third test,
|
||||
`reportDeviceEncoderCapabilities`, has no such guard and runs. A level reporting 0 skipped
|
||||
would mean someone had staged sample files, not that something improved.
|
||||
|
||||
**Since #223 there is a third, and it is the interesting one.**
|
||||
`HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` is `assumeTrue`-guarded on
|
||||
`AndroidDeviceCodecs.get().canEncode(H265)`, which is false on every emulator image — so it now
|
||||
skips here and runs only on the Pixel. It used to *pass* on emulators without ever attempting the
|
||||
hardware path, which is worse. **Expect `skipped="3"` locally**, and note the guard is a property
|
||||
of the machine rather than of staged files: a level reporting 2 would mean an emulator image had
|
||||
gained a hardware HEVC encoder, which is worth knowing.
|
||||
|
||||
That test's KDoc carries the measurement, including the part that decides it: forcing the route to
|
||||
Media3 anyway does *not* produce a fallback, because the goldfish decoder decodes the High 4:4:4
|
||||
fixture despite declaring `NoSupport` for its profile.
|
||||
|
||||
### What the sweep adds, and what it does not
|
||||
|
||||
**The renderer rule held four more times.** No boot log contains the string
|
||||
|
||||
Reference in New Issue
Block a user