Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
948d53b67e | ||
|
|
54932e97c6 | ||
|
|
ba16f5a89b | ||
|
|
bffcff92c7 | ||
|
|
9f06eb9988 | ||
|
|
06ca167034 | ||
|
|
c757565d64 | ||
|
|
4b02294cfb | ||
|
|
79097a0256 | ||
|
|
6004398a83 | ||
|
|
a354620bf5 | ||
|
|
32ab54da3c |
@@ -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,7 +462,11 @@ 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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -113,6 +113,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 +132,63 @@ 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,
|
||||
)
|
||||
}
|
||||
|
||||
// --- the quality tier the GPL licence was taken for --------------------
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
|
||||
@@ -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