Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
da344b0fe4 |
@@ -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
|
- 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
|
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.
|
answers rather than complexity. Every other rule still applies there.
|
||||||
- **Coverage is reported, not gated** — **94.2% of lines (2234/2372), 87.5% of branches
|
- **Coverage is reported, not gated** — **92.8% of lines (2183/2352), 81.3% of branches
|
||||||
(1171/1338)**, measured 2026-09-05 with `./gradlew :app:jacocoTestReport`, against 628 JVM tests
|
(1091/1342)**, measured 2026-09-02 with `./gradlew :app:jacocoTestReport`, against 584 JVM tests
|
||||||
in 96 classes.
|
in 87 classes.
|
||||||
|
|
||||||
**Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
|
**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
|
Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
|
||||||
@@ -184,29 +184,11 @@ install for code that can never run — and on API 37 the full APK does not fit
|
|||||||
- **coverage gaps** — the line never executes. Filtered to sites where JaCoCo reports `mi > 0`, a
|
- **coverage gaps** — the line never executes. Filtered to sites where JaCoCo reports `mi > 0`, a
|
||||||
concrete instruction no test runs, which is what separates a real gap from a partial branch on
|
concrete instruction no test runs, which is what separates a real gap from a partial branch on
|
||||||
a compound condition. That filter cut the candidate list roughly in half and was right to.
|
a compound condition. That filter cut the candidate list roughly in half and was right to.
|
||||||
**Wave 4 found it wrong in both directions, though — use the two filters below instead.**
|
|
||||||
- **assertion gaps** — JaCoCo is green and nothing checks the answer. `MainActivity`'s rail and
|
- **assertion gaps** — JaCoCo is green and nothing checks the answer. `MainActivity`'s rail and
|
||||||
bottom bar were both *executed* by `AppRootRestorationTest` and **transposing them passed the
|
bottom bar were both *executed* by `AppRootRestorationTest` and **transposing them passed the
|
||||||
entire suite**; so did swapping the two progress-notification strings, and swapping `Content`'s
|
entire suite**; so did swapping the two progress-notification strings, and swapping `Content`'s
|
||||||
two destinations. No coverage number would ever have found any of the three.
|
two destinations. No coverage number would ever have found any of the three.
|
||||||
|
|
||||||
**Wave 4 (2026-09-02) corrected that first filter, and the correction is the reusable part.**
|
|
||||||
`mi > 0` fails in both directions. It *over-reports* on Compose: `JoinScreen.kt:222` reads
|
|
||||||
`mi=10` and also `ci=38`, and `JoinStateAffordancesTest` already clicks that Save button and
|
|
||||||
asserts `save:joined.mp4` — the missed instructions are the synthesized `$changed`/`$dirty`
|
|
||||||
recomposition-skip path, the same codegen this file already warns about for *branch* counts,
|
|
||||||
showing up in the instruction count too. And it *under-reports* on warm methods with cold arms:
|
|
||||||
`ConversionViewModel.cancel()` misses no line, yet `activeWorkId?.let(...)` had only ever been
|
|
||||||
entered on the null side in 584 tests. Use two filters together instead:
|
|
||||||
|
|
||||||
- **`ci == 0`** — the line never executed. This is JaCoCo's own missed-line definition, so it
|
|
||||||
totals exactly the reported missed-line count and needs no judgement.
|
|
||||||
- **`ci > 0 && mb > 0` at method level** — a covered method with an arm nothing takes. This is
|
|
||||||
the only one that finds the `cancel()` shape.
|
|
||||||
|
|
||||||
Of wave 4's 251 missed branches, just **18** sat on lines that do execute, so the branch gap and
|
|
||||||
the line gap are largely the same gap; the second filter is about which of them are reachable.
|
|
||||||
|
|
||||||
So **every ticket named the mutation that had to go red, and that was its acceptance criterion
|
So **every ticket named the mutation that had to go red, and that was its acceptance criterion
|
||||||
rather than a coverage delta**. It caught **two vacuous tests written in the same session**,
|
rather than a coverage delta**. It caught **two vacuous tests written in the same session**,
|
||||||
before either shipped:
|
before either shipped:
|
||||||
@@ -254,87 +236,6 @@ install for code that can never run — and on API 37 the full APK does not fit
|
|||||||
|
|
||||||
And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
|
And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
|
||||||
earlier, and was already three points stale by the time it was ready to merge.
|
earlier, and was already three points stale by the time it was ready to merge.
|
||||||
|
|
||||||
**Wave 4's read (2026-09-02) moved no number at all, and that is its result.** It was a triage
|
|
||||||
rather than a test push: twelve tickets (**#192-#203**), four deferred candidates (**#204**), and
|
|
||||||
five findings (**F6-F10** in `docs/coverage-read-findings.md`). What it establishes is the shape
|
|
||||||
of what is left, which is different again from wave 3's:
|
|
||||||
|
|
||||||
- Of 169 never-executed lines, **81 are native or device edges and stay that way** —
|
|
||||||
`FFmpegEngine` 33, `Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10 — their
|
|
||||||
zeroes being the `testDebugUnitTest`-only measurement boundary that #84, #85, #86 and #88 each
|
|
||||||
recorded before. A further **34 are device-bound only until a seam moves them**:
|
|
||||||
`AndroidDeviceCodecs` 20 (#194) and the 14 of `MediaProbe`'s 26 that are `readMediaInformation`
|
|
||||||
(#195). Do not read that second group as exempt — the two tickets exist because it is not.
|
|
||||||
- Most of the rest is **already closed with a reason on record**, or compiler-generated: default-arg
|
|
||||||
bridges, DI factory lambdas, synthetic `NoWhenBranchMatchedException` arms, coroutine completion.
|
|
||||||
- Six of the ten findings in that document are now "no action" or "not a test gap". By this point
|
|
||||||
the report's remaining red is mostly arms nothing can reach, members nothing calls, and arms a
|
|
||||||
test *can* reach but cannot pin — and a coverage number tells none of them apart.
|
|
||||||
|
|
||||||
**The biggest single gap it found was not a missed line.** `ConversionViewModel.cancel()` and
|
|
||||||
`JoinViewModel.cancel()` report every line covered; only the null arm of
|
|
||||||
`activeWorkId?.let(workManager::cancelWorkById)` had ever been entered, so nothing in 584 tests
|
|
||||||
connected the Cancel button to WorkManager (#192). That is what the second filter above is for.
|
|
||||||
|
|
||||||
It also re-opened a mechanism, not a close: #86 and #133 ruled `AndroidDeviceCodecs.probe()` out
|
|
||||||
**through `ShadowMediaCodecList`**, on the grounds that the builder cannot set `isAlias` or
|
|
||||||
`canonicalName`. A pure seam does not have that constraint, and #133 did not evaluate one. Read
|
|
||||||
#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
|
- **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 —
|
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.
|
a change that is both needs both.
|
||||||
@@ -462,29 +363,4 @@ 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
|
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,
|
`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
|
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. It has since been exercised in anger:
|
attribution, so do not delete the watchdog as stray config.
|
||||||
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,17 +12,11 @@ import kotlinx.coroutines.withTimeout
|
|||||||
import org.junit.After
|
import org.junit.After
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
import org.junit.Assert.assertTrue
|
import org.junit.Assert.assertTrue
|
||||||
import org.junit.Assume.assumeTrue
|
|
||||||
import org.junit.Before
|
import org.junit.Before
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
import org.junit.runner.RunWith
|
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.OutputFormat
|
||||||
import org.libremediaconverter.model.QualityTier
|
import org.libremediaconverter.model.QualityTier
|
||||||
import org.libremediaconverter.model.VideoCodec
|
|
||||||
import org.libremediaconverter.work.ConversionWorker
|
import org.libremediaconverter.work.ConversionWorker
|
||||||
import java.io.File
|
import java.io.File
|
||||||
|
|
||||||
@@ -40,47 +34,6 @@ import java.io.File
|
|||||||
* hand — a regression test that silently skips is worse than no test, because the count
|
* hand — a regression test that silently skips is worse than no test, because the count
|
||||||
* still reads as coverage.
|
* 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
|
* The fixture was produced with x264, which the host toolchain cannot do (Fedora's
|
||||||
* ffmpeg ships openh264, which is Constrained Baseline only):
|
* ffmpeg ships openh264, which is Constrained Baseline only):
|
||||||
*
|
*
|
||||||
@@ -113,15 +66,6 @@ class HardwareFallbackTest {
|
|||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun aFileMedia3CannotDecodeStillConvertsViaFfmpeg(): Unit = runBlocking {
|
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(
|
val request = ConversionWorker.request(
|
||||||
inputUri = Uri.fromFile(input),
|
inputUri = Uri.fromFile(input),
|
||||||
displayName = SAMPLE,
|
displayName = SAMPLE,
|
||||||
@@ -131,19 +75,6 @@ class HardwareFallbackTest {
|
|||||||
// the tier where the fallback has to rescue the conversion.
|
// the tier where the fallback has to rescue the conversion.
|
||||||
quality = QualityTier.FAST,
|
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()
|
workManager.enqueue(request).result.get()
|
||||||
|
|
||||||
val terminal = withTimeout(TIMEOUT_MS) {
|
val terminal = withTimeout(TIMEOUT_MS) {
|
||||||
@@ -157,14 +88,6 @@ class HardwareFallbackTest {
|
|||||||
terminal?.state,
|
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)!!)
|
val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!)
|
||||||
assertTrue("no output produced", out.exists() && out.length() > 0)
|
assertTrue("no output produced", out.exists() && out.length() > 0)
|
||||||
out.delete()
|
out.delete()
|
||||||
|
|||||||
@@ -113,11 +113,6 @@ class FFmpegEngineTest {
|
|||||||
fun encodesFlacLosslessAudio() {
|
fun encodesFlacLosslessAudio() {
|
||||||
val out = convert(OutputFormat.FLAC)
|
val out = convert(OutputFormat.FLAC)
|
||||||
assertTrue("no FLAC produced", out.exists() && out.length() > 0)
|
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
|
@Test
|
||||||
@@ -132,63 +127,6 @@ class FFmpegEngineTest {
|
|||||||
fun encodesOpus() {
|
fun encodesOpus() {
|
||||||
val out = convert(OutputFormat.OPUS)
|
val out = convert(OutputFormat.OPUS)
|
||||||
assertTrue("no Opus produced", out.exists() && out.length() > 0)
|
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 --------------------
|
// --- the quality tier the GPL licence was taken for --------------------
|
||||||
|
|||||||
@@ -10,9 +10,6 @@ import androidx.compose.ui.test.performClick
|
|||||||
import androidx.media3.common.util.UnstableApi
|
import androidx.media3.common.util.UnstableApi
|
||||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||||
import androidx.test.platform.app.InstrumentationRegistry
|
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.By
|
||||||
import androidx.test.uiautomator.BySelector
|
import androidx.test.uiautomator.BySelector
|
||||||
import androidx.test.uiautomator.Configurator
|
import androidx.test.uiautomator.Configurator
|
||||||
@@ -27,7 +24,6 @@ import org.junit.runner.RunWith
|
|||||||
import org.libremediaconverter.FailsOnEmulatorApi37
|
import org.libremediaconverter.FailsOnEmulatorApi37
|
||||||
import org.libremediaconverter.MainActivity
|
import org.libremediaconverter.MainActivity
|
||||||
import org.libremediaconverter.ui.TestTags
|
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.
|
* Choosing a file, through the real system picker, and still having it after a rotation.
|
||||||
@@ -207,8 +203,6 @@ import java.util.concurrent.atomic.AtomicInteger
|
|||||||
* driven there at all. That is why this gap survived as long as it did.
|
* 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
|
* `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.
|
* 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]
|
* ### Why only the rotation test carries [FailsOnEmulatorApi37]
|
||||||
*
|
*
|
||||||
@@ -257,21 +251,6 @@ class SafPickerRoundTripTest {
|
|||||||
/** Set by the one test that rotates, read by [restoreOrientation]. See its KDoc. */
|
/** Set by the one test that rotates, read by [restoreOrientation]. See its KDoc. */
|
||||||
private var rotated = false
|
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.
|
* Leave the device the way it was found — and only if this test moved it.
|
||||||
*
|
*
|
||||||
@@ -291,7 +270,6 @@ class SafPickerRoundTripTest {
|
|||||||
*/
|
*/
|
||||||
@After
|
@After
|
||||||
fun restoreOrientation() {
|
fun restoreOrientation() {
|
||||||
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
|
|
||||||
if (!rotated) return
|
if (!rotated) return
|
||||||
device.setOrientationNatural()
|
device.setOrientationNatural()
|
||||||
device.unfreezeRotation()
|
device.unfreezeRotation()
|
||||||
@@ -325,11 +303,9 @@ class SafPickerRoundTripTest {
|
|||||||
// The identity hash rather than the Activity itself, so nothing here keeps a destroyed
|
// 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.
|
// Activity reachable across the recreation it is being used to detect.
|
||||||
val before = System.identityHashCode(composeRule.activity)
|
val before = System.identityHashCode(composeRule.activity)
|
||||||
watchForRecreation()
|
|
||||||
|
|
||||||
device.setOrientationLandscape()
|
device.setOrientationLandscape()
|
||||||
rotated = true
|
rotated = true
|
||||||
awaitRecreation()
|
|
||||||
composeRule.waitForIdle()
|
composeRule.waitForIdle()
|
||||||
|
|
||||||
// Two guards before the assertion that matters, because both of the ways this test could
|
// Two guards before the assertion that matters, because both of the ways this test could
|
||||||
@@ -699,36 +675,6 @@ class SafPickerRoundTripTest {
|
|||||||
* `Condition still not satisfied after 30000 ms` — which names neither the node nor the test.
|
* `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.
|
* 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) {
|
private fun awaitNode(tag: String) {
|
||||||
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
|
composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) {
|
||||||
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
|
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
|
||||||
@@ -757,15 +703,6 @@ class SafPickerRoundTripTest {
|
|||||||
*/
|
*/
|
||||||
const val REOPENED_TIMEOUT_MS = 10_000L
|
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.
|
* How long the app is given to take the window focus back after a back press.
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -3,7 +3,6 @@ package org.libremediaconverter
|
|||||||
import android.app.Application
|
import android.app.Application
|
||||||
import kotlinx.coroutines.CoroutineScope
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.Dispatchers
|
import kotlinx.coroutines.Dispatchers
|
||||||
import kotlinx.coroutines.Job
|
|
||||||
import kotlinx.coroutines.SupervisorJob
|
import kotlinx.coroutines.SupervisorJob
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
import org.libremediaconverter.convert.OutputPublisher
|
import org.libremediaconverter.convert.OutputPublisher
|
||||||
@@ -18,36 +17,14 @@ import org.libremediaconverter.convert.OutputPublisher
|
|||||||
* ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the
|
* 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.
|
* Activity. Process start is the one moment those leftovers are reliably observable.
|
||||||
*/
|
*/
|
||||||
open class LibreMediaConverterApp : Application() {
|
class LibreMediaConverterApp : Application() {
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Deliberately process-lifetime and never cancelled: the work it carries is a single
|
* 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.
|
* 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.
|
* 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.
|
|
||||||
*/
|
*/
|
||||||
protected open val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
|
private val appScope = 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() {
|
override fun onCreate() {
|
||||||
super.onCreate()
|
super.onCreate()
|
||||||
@@ -76,6 +53,6 @@ open class LibreMediaConverterApp : Application() {
|
|||||||
//
|
//
|
||||||
// sweepStaging() also re-reads each timestamp immediately before deleting, which
|
// sweepStaging() also re-reads each timestamp immediately before deleting, which
|
||||||
// closes the window between listing the directory and acting on the listing.
|
// closes the window between listing the directory and acting on the listing.
|
||||||
startupSweep = sweepScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
|
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -21,11 +21,6 @@ import org.libremediaconverter.model.VideoCodec
|
|||||||
* words, "cannot be tested for correctness". It is a hint, not a guarantee, which is
|
* words, "cannot be tested for correctness". It is a hint, not a guarantee, which is
|
||||||
* why the router treats a failed hardware export as a signal to fall back rather
|
* why the router treats a failed hardware export as a signal to fall back rather
|
||||||
* than trusting this up front.
|
* than trusting this up front.
|
||||||
* - **An enumeration that fails answers no to everything**, which sends every job to
|
|
||||||
* FFmpeg. Empty sets are not a permissive default: `canEncode` looks a MIME type up in
|
|
||||||
* [hardwareEncodeMimes] and finds nothing there. That is the intended answer — FFmpeg
|
|
||||||
* can do whatever Media3 can, only slower — but it is the opposite of what this class
|
|
||||||
* said until #194, so it is written down rather than left to be re-derived.
|
|
||||||
*/
|
*/
|
||||||
class AndroidDeviceCodecs private constructor(
|
class AndroidDeviceCodecs private constructor(
|
||||||
private val hardwareEncodeMimes: Set<String>,
|
private val hardwareEncodeMimes: Set<String>,
|
||||||
@@ -51,62 +46,22 @@ class AndroidDeviceCodecs private constructor(
|
|||||||
|
|
||||||
fun get(): AndroidDeviceCodecs = cached ?: synchronized(this) { cached ?: probe().also { cached = it } }
|
fun get(): AndroidDeviceCodecs = cached ?: synchronized(this) { cached ?: probe().also { cached = it } }
|
||||||
|
|
||||||
/**
|
private fun probe(): AndroidDeviceCodecs {
|
||||||
* One entry of the platform's codec list, reduced to what the rules below read.
|
|
||||||
*
|
|
||||||
* The five booleans and the type list are the whole of what [capabilitiesFrom] needs, and
|
|
||||||
* none of them can be set on a `MediaCodecInfo` from a test: Robolectric ships
|
|
||||||
* `MediaCodecInfoBuilder`, but it has no `setIsAlias` and no `setCanonicalName`, which is
|
|
||||||
* exactly the objection #133 raised against reaching this code through
|
|
||||||
* `ShadowMediaCodecList`. That objection is about the shadow. It does not apply to a
|
|
||||||
* function that takes its own entry type, which is why this exists.
|
|
||||||
*/
|
|
||||||
internal data class CodecEntry(
|
|
||||||
val canonicalName: String,
|
|
||||||
val isAlias: Boolean,
|
|
||||||
val isEncoder: Boolean,
|
|
||||||
val isHardwareAccelerated: Boolean,
|
|
||||||
val isSoftwareOnly: Boolean,
|
|
||||||
val supportedTypes: List<String>,
|
|
||||||
)
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The enumeration rules, over entries a caller chooses.
|
|
||||||
*
|
|
||||||
* [probe] is the only production caller and supplies the real codec list; a test supplies
|
|
||||||
* its own, which is the point — the two rules this class's KDoc calls out as easy to get
|
|
||||||
* wrong, the alias skip and the canonical-name dedup, are unreachable any other way.
|
|
||||||
*
|
|
||||||
* **`enumerate` returns a `Sequence`, deliberately.** The `runCatching` has to wrap the
|
|
||||||
* *iteration* rather than a list built before it, because a `MediaCodecInfo` whose
|
|
||||||
* properties throw does so partway through — and when that happens the codecs already read
|
|
||||||
* are kept. Taking a `List` here would move that throw outside the loop and silently turn a
|
|
||||||
* partial answer into an empty one. That behaviour predates this seam; a `List` parameter
|
|
||||||
* would have changed it as a side effect of a refactor.
|
|
||||||
*
|
|
||||||
* **An enumeration that fails answers restrictively, and that is deliberate.** The sets
|
|
||||||
* come back empty, and `"video/avc" in emptySet()` is `false`, so [canEncode] and
|
|
||||||
* [canDecode] both answer no and every job routes to FFmpeg. FFmpeg can do everything
|
|
||||||
* Media3 can, only slower, so refusing the hardware path is the safe reading of "we could
|
|
||||||
* not find out what this device supports". This used to log "assuming permissive", which
|
|
||||||
* described the opposite of what the code does.
|
|
||||||
*/
|
|
||||||
internal fun capabilitiesFrom(enumerate: () -> Sequence<CodecEntry>): AndroidDeviceCodecs {
|
|
||||||
val encoders = mutableSetOf<String>()
|
val encoders = mutableSetOf<String>()
|
||||||
val decoders = mutableSetOf<String>()
|
val decoders = mutableSetOf<String>()
|
||||||
val seen = mutableSetOf<String>()
|
val seen = mutableSetOf<String>()
|
||||||
|
|
||||||
runCatching {
|
runCatching {
|
||||||
enumerate().forEach { entry ->
|
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.forEach { info ->
|
||||||
// Aliases point at the same underlying codec; counting both would
|
// Aliases point at the same underlying codec; counting both would
|
||||||
// double-count capabilities.
|
// double-count capabilities.
|
||||||
if (entry.isAlias) return@forEach
|
if (info.isAlias) return@forEach
|
||||||
if (!seen.add(entry.canonicalName)) return@forEach
|
if (!seen.add(info.canonicalName)) return@forEach
|
||||||
|
|
||||||
entry.supportedTypes.forEach { mime ->
|
info.supportedTypes.forEach { mime ->
|
||||||
if (!mime.startsWith("video/")) return@forEach
|
if (!mime.startsWith("video/")) return@forEach
|
||||||
if (entry.isEncoder) {
|
if (info.isEncoder) {
|
||||||
if (entry.isHardwareAccelerated && !entry.isSoftwareOnly) {
|
if (info.isHardwareAccelerated && !info.isSoftwareOnly) {
|
||||||
encoders += mime
|
encoders += mime
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
@@ -114,32 +69,12 @@ class AndroidDeviceCodecs private constructor(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}.onFailure { Log.w(TAG, "Codec enumeration failed; routing everything to FFmpeg.", it) }
|
}.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) }
|
||||||
|
|
||||||
Log.i(TAG, "Hardware video encoders: $encoders")
|
Log.i(TAG, "Hardware video encoders: $encoders")
|
||||||
return AndroidDeviceCodecs(encoders, decoders)
|
return AndroidDeviceCodecs(encoders, decoders)
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* The thin edge: the real codec list, mapped onto [CodecEntry] one at a time.
|
|
||||||
*
|
|
||||||
* Lazily, so a property that throws does it inside [capabilitiesFrom]'s `runCatching` and
|
|
||||||
* on the entry that caused it — see that function's note on why the parameter is a
|
|
||||||
* `Sequence`.
|
|
||||||
*/
|
|
||||||
private fun probe(): AndroidDeviceCodecs = capabilitiesFrom {
|
|
||||||
MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.asSequence().map { info ->
|
|
||||||
CodecEntry(
|
|
||||||
canonicalName = info.canonicalName,
|
|
||||||
isAlias = info.isAlias,
|
|
||||||
isEncoder = info.isEncoder,
|
|
||||||
isHardwareAccelerated = info.isHardwareAccelerated,
|
|
||||||
isSoftwareOnly = info.isSoftwareOnly,
|
|
||||||
supportedTypes = info.supportedTypes.toList(),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* `internal` rather than `private` so the cross-check test can ask what a [VideoCodec]
|
* `internal` rather than `private` so the cross-check test can ask what a [VideoCodec]
|
||||||
* means here and compare it with what [NAME_TO_MIME] says the same codec's names mean.
|
* means here and compare it with what [NAME_TO_MIME] says the same codec's names mean.
|
||||||
|
|||||||
@@ -673,23 +673,13 @@ class ConversionViewModel @JvmOverloads constructor(
|
|||||||
else -> null
|
else -> null
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
private fun currentInput(): InputFile? = when (val s = _state.value) {
|
||||||
* The input `convert()` may act on, which is only ever the one on a `Ready` screen.
|
is ConversionState.Ready -> s.input
|
||||||
*
|
is ConversionState.Converting -> s.input
|
||||||
* This used to answer for `Converting`, `Waiting` and `Converted` as well. Those arms were not
|
is ConversionState.Waiting -> s.input
|
||||||
* reachable by tapping Convert -- the button renders only in the `Ready` branch -- but they
|
is ConversionState.Converted -> s.input
|
||||||
* were reachable through the POST_NOTIFICATIONS **result**, which `ConverterScreen.kt:91` wires
|
else -> null
|
||||||
* 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 {
|
private companion object {
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -221,39 +221,12 @@ object MediaProbe {
|
|||||||
null
|
null
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
private fun readMediaInformation(path: String): FFprobeInfo? {
|
||||||
* The thin edge: spawn FFprobe, hand what it said to [ffprobeInfoFrom].
|
// ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these have to go
|
||||||
*
|
// through the Java getters rather than property syntax.
|
||||||
* Everything device-bound is on this line and the null check under it. What FFprobe *said* is a
|
val info: MediaInformation = FFprobeKit.getMediaInformation(path).getMediaInformation()
|
||||||
* `MediaInformation`, which is an ordinary object over a `JSONObject` — so the reading of it is
|
?: return null
|
||||||
* a decision a test can choose the inputs for, and it lives below rather than here.
|
|
||||||
*/
|
|
||||||
private fun readMediaInformation(path: String): FFprobeInfo? =
|
|
||||||
FFprobeKit.getMediaInformation(path).getMediaInformation()?.let(::ffprobeInfoFrom)
|
|
||||||
|
|
||||||
/**
|
|
||||||
* What FFprobe's answer means, as a function of the answer alone.
|
|
||||||
*
|
|
||||||
* `internal` for the same reason [Extracted] and [FFprobeInfo] are: a test cannot name it
|
|
||||||
* otherwise, and the JVM test source set is a friend of `main`.
|
|
||||||
*
|
|
||||||
* **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar:
|
|
||||||
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
|
|
||||||
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
|
|
||||||
* touches the native library — so a test builds its own without `libffmpegkit` being present.
|
|
||||||
* That is the whole reason this split is worth making: `readMediaInformation` was 114 missed
|
|
||||||
* instructions and 24 missed branches, of which exactly one line needed a device.
|
|
||||||
*
|
|
||||||
* The subtle part is the **second argument to [containerFrom]**. `matroska,webm` is reported
|
|
||||||
* for both MKV and WebM — they share a demuxer — so the video codec is the only thing that
|
|
||||||
* separates them, and dropping it silently turns every VP9 WebM into an MKV. `containerFrom`
|
|
||||||
* has thirty-three covered branches of its own and none of them can notice that, because the
|
|
||||||
* mistake is at the call rather than in the callee.
|
|
||||||
*
|
|
||||||
* ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these go through the
|
|
||||||
* Java getters rather than property syntax.
|
|
||||||
*/
|
|
||||||
internal fun ffprobeInfoFrom(info: MediaInformation): FFprobeInfo {
|
|
||||||
val streams = info.getStreams().orEmpty()
|
val streams = info.getStreams().orEmpty()
|
||||||
val video = streams.firstOrNull { it.getType() == "video" }
|
val video = streams.firstOrNull { it.getType() == "video" }
|
||||||
val audio = streams.firstOrNull { it.getType() == "audio" }
|
val audio = streams.firstOrNull { it.getType() == "audio" }
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import android.net.Uri
|
|||||||
import android.util.Log
|
import android.util.Log
|
||||||
import com.arthenica.ffmpegkit.FFmpegKit
|
import com.arthenica.ffmpegkit.FFmpegKit
|
||||||
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
||||||
|
import com.arthenica.ffmpegkit.ReturnCode
|
||||||
import kotlinx.coroutines.suspendCancellableCoroutine
|
import kotlinx.coroutines.suspendCancellableCoroutine
|
||||||
import org.libremediaconverter.convert.ConcatJoiner
|
import org.libremediaconverter.convert.ConcatJoiner
|
||||||
import org.libremediaconverter.convert.MediaProbe
|
import org.libremediaconverter.convert.MediaProbe
|
||||||
@@ -65,16 +66,16 @@ class ConcatEngine(private val context: Context) : ConcatJoiner {
|
|||||||
private suspend fun execute(args: List<String>) = suspendCancellableCoroutine { cont ->
|
private suspend fun execute(args: List<String>) = suspendCancellableCoroutine { cont ->
|
||||||
Log.i(TAG, "ffmpeg ${args.joinToString(" ")}")
|
Log.i(TAG, "ffmpeg ${args.joinToString(" ")}")
|
||||||
val session = FFmpegKit.executeWithArgumentsAsync(args.toTypedArray()) { completed ->
|
val session = FFmpegKit.executeWithArgumentsAsync(args.toTypedArray()) { completed ->
|
||||||
val outcome = sessionOutcome(
|
val rc = completed.getReturnCode()
|
||||||
rc = completed.getReturnCode(),
|
when {
|
||||||
prefix = "Joining",
|
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
|
||||||
failStackTrace = { completed.getFailStackTrace() },
|
ReturnCode.isCancel(rc) -> cont.cancel()
|
||||||
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
|
else -> cont.resumeWithException(
|
||||||
)
|
FFmpegEngine.FFmpegException(
|
||||||
when (outcome) {
|
"Joining failed (${rc?.value}): " +
|
||||||
SessionOutcome.Success -> cont.resume(Unit)
|
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(),
|
||||||
SessionOutcome.Cancelled -> cont.cancel()
|
),
|
||||||
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegEngine.FFmpegException(outcome.message))
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }
|
cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import android.util.Log
|
|||||||
import com.arthenica.ffmpegkit.FFmpegKit
|
import com.arthenica.ffmpegkit.FFmpegKit
|
||||||
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
||||||
import com.arthenica.ffmpegkit.Level
|
import com.arthenica.ffmpegkit.Level
|
||||||
|
import com.arthenica.ffmpegkit.ReturnCode
|
||||||
import kotlinx.coroutines.suspendCancellableCoroutine
|
import kotlinx.coroutines.suspendCancellableCoroutine
|
||||||
import org.libremediaconverter.convert.SoftwareTranscoder
|
import org.libremediaconverter.convert.SoftwareTranscoder
|
||||||
import org.libremediaconverter.model.ConversionRequest
|
import org.libremediaconverter.model.ConversionRequest
|
||||||
@@ -50,16 +51,19 @@ class FFmpegEngine : SoftwareTranscoder {
|
|||||||
val session = FFmpegKit.executeWithArgumentsAsync(
|
val session = FFmpegKit.executeWithArgumentsAsync(
|
||||||
args.toTypedArray(),
|
args.toTypedArray(),
|
||||||
{ completed ->
|
{ completed ->
|
||||||
val outcome = sessionOutcome(
|
val rc = completed.getReturnCode()
|
||||||
rc = completed.getReturnCode(),
|
when {
|
||||||
prefix = "FFmpeg",
|
ReturnCode.isSuccess(rc) -> cont.resume(Unit)
|
||||||
failStackTrace = { completed.getFailStackTrace() },
|
ReturnCode.isCancel(rc) ->
|
||||||
logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) },
|
cont.cancel()
|
||||||
)
|
else -> cont.resumeWithException(
|
||||||
when (outcome) {
|
FFmpegException(
|
||||||
SessionOutcome.Success -> cont.resume(Unit)
|
"FFmpeg failed (${rc?.value}): " +
|
||||||
SessionOutcome.Cancelled -> cont.cancel()
|
completed.getFailStackTrace().orEmpty().ifBlank {
|
||||||
is SessionOutcome.Failed -> cont.resumeWithException(FFmpegException(outcome.message))
|
completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty()
|
||||||
|
},
|
||||||
|
),
|
||||||
|
)
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
{ log -> Log.d(TAG, log.message.trimEnd()) },
|
{ log -> Log.d(TAG, log.message.trimEnd()) },
|
||||||
|
|||||||
@@ -1,54 +0,0 @@
|
|||||||
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
|
package org.libremediaconverter
|
||||||
|
|
||||||
import kotlinx.coroutines.runBlocking
|
import org.junit.Assert.assertEquals
|
||||||
import org.junit.Assert.assertNotNull
|
|
||||||
import org.junit.Assert.assertTrue
|
import org.junit.Assert.assertTrue
|
||||||
|
import org.junit.Assert.fail
|
||||||
import org.junit.Before
|
import org.junit.Before
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
import org.junit.runner.RunWith
|
import org.junit.runner.RunWith
|
||||||
@@ -10,6 +10,7 @@ import org.libremediaconverter.convert.StagingSweep
|
|||||||
import org.robolectric.RobolectricTestRunner
|
import org.robolectric.RobolectricTestRunner
|
||||||
import org.robolectric.RuntimeEnvironment
|
import org.robolectric.RuntimeEnvironment
|
||||||
import java.io.File
|
import java.io.File
|
||||||
|
import java.util.concurrent.TimeUnit
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* That process start actually sweeps.
|
* That process start actually sweeps.
|
||||||
@@ -22,19 +23,8 @@ import java.io.File
|
|||||||
* output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity.
|
* 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
|
* `onCreate()` is called again rather than a second Application being built: it is what the
|
||||||
* framework calls at process start, and the scope it launches on is already there.
|
* 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.
|
||||||
* **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)
|
@RunWith(RobolectricTestRunner::class)
|
||||||
class AppStartSweepTest {
|
class AppStartSweepTest {
|
||||||
@@ -44,35 +34,17 @@ class AppStartSweepTest {
|
|||||||
|
|
||||||
@Before
|
@Before
|
||||||
fun setUp() {
|
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
|
app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp
|
||||||
stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() }
|
stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() }
|
||||||
stagingDir.listFiles()?.forEach { it.delete() }
|
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
|
@Test
|
||||||
fun `the sweep is finished before onCreate returns`() {
|
fun `the application the manifest starts is the one that sweeps`() {
|
||||||
app.onCreate()
|
assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass)
|
||||||
|
|
||||||
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
|
@Test
|
||||||
@@ -92,23 +64,35 @@ class AppStartSweepTest {
|
|||||||
|
|
||||||
app.onCreate()
|
app.onCreate()
|
||||||
|
|
||||||
// Joined rather than polled. `onCreate` publishes the sweep it started, so this waits for
|
awaitGone(abandoned)
|
||||||
// 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
|
// 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
|
// `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.
|
// 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())
|
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 fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) }
|
||||||
|
|
||||||
private companion object {
|
private companion object {
|
||||||
const val ONE_MINUTE_MS = 60L * 1000
|
const val ONE_MINUTE_MS = 60L * 1000
|
||||||
|
const val AWAIT_TIMEOUT_SECONDS = 10L
|
||||||
|
const val POLL_INTERVAL_MS = 5L
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,28 +0,0 @@
|
|||||||
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)
|
|
||||||
}
|
|
||||||
@@ -1,201 +0,0 @@
|
|||||||
package org.libremediaconverter.codec
|
|
||||||
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertFalse
|
|
||||||
import org.junit.Assert.assertTrue
|
|
||||||
import org.junit.Test
|
|
||||||
import org.junit.runner.RunWith
|
|
||||||
import org.libremediaconverter.model.VideoCodec
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The rules `AndroidDeviceCodecs.probe()` applies to the platform's codec list.
|
|
||||||
*
|
|
||||||
* ## Why this is not a third run of the #86/#133 spike
|
|
||||||
*
|
|
||||||
* #86 closed `probe()` as device-bound. #133 re-opened the question with
|
|
||||||
* `ShadowMediaCodecList` in hand and closed it again, for a reason that was right about what it
|
|
||||||
* was answering: `MediaCodecInfoBuilder` "has no `setIsAlias` and no `setCanonicalName`, so the
|
|
||||||
* alias skip and the canonical-name dedup — the two things the class's KDoc calls out as easy to
|
|
||||||
* get wrong — are not reachable through it."
|
|
||||||
*
|
|
||||||
* **That objection is about the shadow.** It does not apply to a function that takes its own entry
|
|
||||||
* type, which is what `capabilitiesFrom` now does. The half #133 named as unreachable is the half
|
|
||||||
* this file spends most of its cases on.
|
|
||||||
*
|
|
||||||
* ## What made the seam worth cutting, which is not coverage
|
|
||||||
*
|
|
||||||
* The `runCatching` fallback logged *"assuming permissive"* and returned empty sets — and empty
|
|
||||||
* sets are **restrictive**: `"video/avc" in emptySet()` is `false`, so `canEncode` and `canDecode`
|
|
||||||
* both answer no and every job routes to FFmpeg. The code was right and the message described the
|
|
||||||
* opposite of it. That is pinned below, so whichever reading a future change takes, it has to say
|
|
||||||
* so out loud.
|
|
||||||
*
|
|
||||||
* Robolectric only because `capabilitiesFrom` logs what it found; the rules themselves are pure.
|
|
||||||
*/
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class CodecEnumerationTest {
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The alias skip, in the one arrangement where it is observable — and finding that arrangement
|
|
||||||
* is the whole of this test.
|
|
||||||
*
|
|
||||||
* A first attempt listed the alias *after* the codec it aliases and passed with the skip
|
|
||||||
* deleted, because `canonicalName` is shared and the dedup below catches the second entry
|
|
||||||
* either way. The two rules overlap, so a fixture that does not separate them tests neither.
|
|
||||||
*
|
|
||||||
* What separates them is **order**. `MediaCodecInfo.getCanonicalName()` on an alias returns the
|
|
||||||
* underlying codec's name, so an alias arriving first claims that name in `seen` and has its
|
|
||||||
* own `supportedTypes` credited — and then the real codec is dropped by the dedup. Without the
|
|
||||||
* alias skip the device is described by whichever entry the platform happened to list first.
|
|
||||||
*
|
|
||||||
* That also says what the rule is worth. With a `Set` accumulator, an alias declaring the same
|
|
||||||
* types as its codec changes nothing whichever order they arrive in; the skip earns its place
|
|
||||||
* only when the two disagree, which is exactly when believing the wrong one matters.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `an alias listed before the codec it aliases does not describe the device`() {
|
|
||||||
val codecs = capabilities(
|
|
||||||
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC), alias = true),
|
|
||||||
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertTrue("the real codec's types are the device's", codecs.canEncode(VideoCodec.H264))
|
|
||||||
assertFalse(
|
|
||||||
"an alias must not be credited with types the codec it aliases never claimed",
|
|
||||||
codecs.canEncode(VideoCodec.H265),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `two entries sharing a canonical name are read once`() {
|
|
||||||
val codecs = capabilities(
|
|
||||||
entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)),
|
|
||||||
entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC)),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertEquals(setOf(AVC), codecs.hardwareEncoders())
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* Both halves of the hardware predicate, one arm at a time.
|
|
||||||
*
|
|
||||||
* A vendor may declare a codec hardware-accelerated *and* software-only; the class KDoc is
|
|
||||||
* explicit that the first flag "cannot be tested for correctness", so the second is what stops
|
|
||||||
* a mislabelled software encoder being treated as the fast path.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `an encoder counts as hardware only when it is accelerated and not software-only`() {
|
|
||||||
assertEquals(
|
|
||||||
setOf(AVC),
|
|
||||||
capabilities(entry("hw", encoder = true, accelerated = true, types = listOf(AVC))).hardwareEncoders(),
|
|
||||||
)
|
|
||||||
assertEquals(
|
|
||||||
emptySet<String>(),
|
|
||||||
capabilities(entry("sw", encoder = true, accelerated = false, types = listOf(AVC))).hardwareEncoders(),
|
|
||||||
)
|
|
||||||
assertEquals(
|
|
||||||
"a codec claiming both must not be trusted as hardware",
|
|
||||||
emptySet<String>(),
|
|
||||||
capabilities(
|
|
||||||
entry("both", encoder = true, accelerated = true, softwareOnly = true, types = listOf(AVC)),
|
|
||||||
).hardwareEncoders(),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* Decoders are collected regardless of the hardware flags, and that asymmetry is the design.
|
|
||||||
*
|
|
||||||
* `canDecode` asks whether the platform can read the input at all — a software decoder answers
|
|
||||||
* that as well as a hardware one. `canEncode` asks whether the *fast path* exists, which is a
|
|
||||||
* different question and why only encoders are filtered.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a software decoder still counts as something the platform can read`() {
|
|
||||||
val codecs = capabilities(
|
|
||||||
entry(
|
|
||||||
"c2.android.avc.decoder",
|
|
||||||
encoder = false,
|
|
||||||
accelerated = false,
|
|
||||||
softwareOnly = true,
|
|
||||||
types = listOf(AVC),
|
|
||||||
),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertTrue(codecs.canDecode("h264"))
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `audio types are ignored on both sides`() {
|
|
||||||
val codecs = capabilities(
|
|
||||||
entry("aac.encoder", encoder = true, accelerated = true, types = listOf("audio/mp4a-latm")),
|
|
||||||
entry("aac.decoder", encoder = false, types = listOf("audio/mp4a-latm")),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
|
|
||||||
// Not "the platform cannot decode AAC" -- `canDecode` is asked about *video* codec names,
|
|
||||||
// and an unknown name is answered permissively. The point is that nothing audio reached
|
|
||||||
// either set.
|
|
||||||
assertTrue("an unknown name stays permissive", codecs.canDecode("something-nobody-named"))
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The failure fallback, pinned as the restrictive answer it actually is.
|
|
||||||
*
|
|
||||||
* #194 decided this rather than assuming it: the code stays, the message changes. If a later
|
|
||||||
* change wants the permissive reading its old log line described, this test is what makes that
|
|
||||||
* a decision instead of a drift.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `an enumeration that fails sends every job to FFmpeg`() {
|
|
||||||
val codecs = AndroidDeviceCodecs.capabilitiesFrom { error("MediaCodecList exploded") }
|
|
||||||
|
|
||||||
assertFalse("a failed enumeration must not claim a hardware encoder", codecs.canEncode(VideoCodec.H264))
|
|
||||||
assertFalse(codecs.canDecode("h264"))
|
|
||||||
assertEquals(emptySet<String>(), codecs.hardwareEncoders())
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* A list that throws partway keeps what it already read.
|
|
||||||
*
|
|
||||||
* This predates the seam — `runCatching` has always wrapped the iteration rather than a list
|
|
||||||
* built before it — and it is asserted here because the seam is where it could quietly have
|
|
||||||
* been lost. Taking a `List` instead of a `Sequence` would move the throw outside the loop and
|
|
||||||
* turn this partial answer into an empty one, with no test to notice.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `codecs read before a failing entry are kept`() {
|
|
||||||
val codecs = AndroidDeviceCodecs.capabilitiesFrom {
|
|
||||||
sequence {
|
|
||||||
yield(entry("good", encoder = true, accelerated = true, types = listOf(AVC)))
|
|
||||||
error("the sixth codec's properties threw")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
assertEquals(setOf(AVC), codecs.hardwareEncoders())
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun capabilities(vararg entries: AndroidDeviceCodecs.Companion.CodecEntry) =
|
|
||||||
AndroidDeviceCodecs.capabilitiesFrom { entries.asSequence() }
|
|
||||||
|
|
||||||
private fun entry(
|
|
||||||
canonicalName: String,
|
|
||||||
encoder: Boolean,
|
|
||||||
accelerated: Boolean = true,
|
|
||||||
softwareOnly: Boolean = false,
|
|
||||||
alias: Boolean = false,
|
|
||||||
types: List<String>,
|
|
||||||
) = AndroidDeviceCodecs.Companion.CodecEntry(
|
|
||||||
canonicalName = canonicalName,
|
|
||||||
isAlias = alias,
|
|
||||||
isEncoder = encoder,
|
|
||||||
isHardwareAccelerated = accelerated,
|
|
||||||
isSoftwareOnly = softwareOnly,
|
|
||||||
supportedTypes = types,
|
|
||||||
)
|
|
||||||
|
|
||||||
private companion object {
|
|
||||||
const val AVC = "video/avc"
|
|
||||||
const val HEVC = "video/hevc"
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,180 +0,0 @@
|
|||||||
package org.libremediaconverter.convert
|
|
||||||
|
|
||||||
import android.app.Application
|
|
||||||
import androidx.media3.common.util.UnstableApi
|
|
||||||
import androidx.work.OneTimeWorkRequestBuilder
|
|
||||||
import androidx.work.WorkInfo
|
|
||||||
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.libremediaconverter.work.JobTags
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
import org.robolectric.RuntimeEnvironment
|
|
||||||
import java.util.UUID
|
|
||||||
import java.util.concurrent.TimeUnit
|
|
||||||
|
|
||||||
/**
|
|
||||||
* That `cancel()` cancels the job, on both screens.
|
|
||||||
*
|
|
||||||
* ## Why this was missing, which is the interesting part
|
|
||||||
*
|
|
||||||
* Both `cancel()` methods are one line — `activeWorkId?.let(workManager::cancelWorkById)` — and
|
|
||||||
* **JaCoCo reports every line of both as covered**. `SettingsEditsTest`'s
|
|
||||||
* `cancelling with no active job does nothing rather than throwing` runs the method, and its own
|
|
||||||
* comment names which half it drives: "`activeWorkId?.let(...)` -- the null side". The other side
|
|
||||||
* had never been entered, and `JoinViewModel.cancel()` had no test at all.
|
|
||||||
*
|
|
||||||
* So no line-level coverage filter could see this. What surfaces it is a method-level read —
|
|
||||||
* `mi=11, ci=7, mb=1, cb=1` on both — a covered method with an arm nothing takes. That is the
|
|
||||||
* second of the two filters #194 records, and this is the gap that argued for it.
|
|
||||||
*
|
|
||||||
* The affordance tests are not this. `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
|
|
||||||
* click `TestTags.CANCEL` and assert the *action* fires into a stub; `ScreenWiringTest` asserts the
|
|
||||||
* action calls `viewModel.cancel()`. Both halves were pinned and the join between them was not, so
|
|
||||||
* nothing in 584 tests connected the button to WorkManager.
|
|
||||||
*
|
|
||||||
* ## Why the job is enqueued with a delay
|
|
||||||
*
|
|
||||||
* The test WorkManager runs on a `SynchronousExecutor`, so an ordinary request finishes inline —
|
|
||||||
* which is exactly why only the null half was ever covered: by the time a test could call
|
|
||||||
* `cancel()`, `convert()`'s job was already terminal. `setInitialDelay` is what `TestScheduler`
|
|
||||||
* honours, so the job sits in `ENQUEUED` until the test lets it go, and it never does.
|
|
||||||
*
|
|
||||||
* **Production never sets a delay**, so the request is built here rather than through
|
|
||||||
* `ConversionWorker.request`. The *state* is not synthetic: `ENQUEUED` at `runAttemptCount == 0` is
|
|
||||||
* what every job passes through before the scheduler picks it up, `Reattachment.choose` ranks it
|
|
||||||
* `QUEUED`, and `conversionStateFrom` maps it to `Converting(input, 0)`. The delay changes how long
|
|
||||||
* the job stays in a real state, not which state it is in.
|
|
||||||
*
|
|
||||||
* ## What is asserted, and in which order
|
|
||||||
*
|
|
||||||
* WorkManager's own record first, then the screen. The screen alone would be a weaker claim than it
|
|
||||||
* looks: `CANCELLED` maps to `Idle` for a reattached job, and `Idle` is also where a ViewModel that
|
|
||||||
* did nothing at all would sit.
|
|
||||||
*/
|
|
||||||
@UnstableApi
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class CancelReachesWorkManagerTest {
|
|
||||||
|
|
||||||
private lateinit var app: Application
|
|
||||||
private lateinit var workManager: WorkManager
|
|
||||||
|
|
||||||
@Before
|
|
||||||
fun setUp() {
|
|
||||||
app = RuntimeEnvironment.getApplication()
|
|
||||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
|
||||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
|
||||||
installTestWorkManager(app, workDataOf())
|
|
||||||
workManager = WorkManager.getInstance(app)
|
|
||||||
}
|
|
||||||
|
|
||||||
@After
|
|
||||||
fun tearDown() {
|
|
||||||
ConversionDependencies.reset()
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `cancelling a queued conversion cancels that job`() {
|
|
||||||
val id = enqueueQueuedConversion()
|
|
||||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
|
||||||
awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
|
|
||||||
|
|
||||||
viewModel.cancel()
|
|
||||||
|
|
||||||
assertEquals(
|
|
||||||
"Cancel must reach WorkManager, not just the screen",
|
|
||||||
WorkInfo.State.CANCELLED,
|
|
||||||
stateOf(id),
|
|
||||||
)
|
|
||||||
awaitState(viewModel.state, "Idle") { it is ConversionState.Idle }
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `cancelling a queued join cancels that job`() {
|
|
||||||
val id = enqueueQueuedJoin()
|
|
||||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
|
||||||
awaitState(viewModel.state, "Joining") { it is JoinState.Joining }
|
|
||||||
|
|
||||||
viewModel.cancel()
|
|
||||||
|
|
||||||
assertEquals(
|
|
||||||
"Cancel must reach WorkManager, not just the screen",
|
|
||||||
WorkInfo.State.CANCELLED,
|
|
||||||
stateOf(id),
|
|
||||||
)
|
|
||||||
awaitState(viewModel.state, "Idle") { it is JoinState.Idle }
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The negative that bounds both: cancelling must cancel the job the screen is showing, and only
|
|
||||||
* that one.
|
|
||||||
*
|
|
||||||
* Without this, `cancel()` could cancel everything in the queue — `cancelAllWork()` in place of
|
|
||||||
* `cancelWorkById(activeWorkId)` — and both tests above would still pass.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `cancelling one conversion leaves another queued job alone`() {
|
|
||||||
val bystander = enqueueQueuedConversion(displayName = "beach.mp4")
|
|
||||||
val id = enqueueQueuedConversion(displayName = "holiday.mp4")
|
|
||||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
|
||||||
val converting = awaitState(viewModel.state, "Converting") { it is ConversionState.Converting }
|
|
||||||
val onScreen = (converting as ConversionState.Converting).input.displayName
|
|
||||||
|
|
||||||
viewModel.cancel()
|
|
||||||
|
|
||||||
// Which of the two the ViewModel reattached to is the query's business, not this test's --
|
|
||||||
// the comparator leaves queued jobs tied deliberately, per Reattachment's ordering notes.
|
|
||||||
// So assert the shape rather than the identity: exactly one is cancelled, and the other is
|
|
||||||
// untouched.
|
|
||||||
val cancelled = listOf(id, bystander).filter { stateOf(it) == WorkInfo.State.CANCELLED }
|
|
||||||
assertEquals(
|
|
||||||
"exactly one job may be cancelled, with $onScreen on screen",
|
|
||||||
1,
|
|
||||||
cancelled.size,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun stateOf(id: UUID): WorkInfo.State =
|
|
||||||
requireNotNull(workManager.getWorkInfoById(id).get()) { "no WorkInfo for $id" }.state
|
|
||||||
|
|
||||||
/**
|
|
||||||
* A conversion sitting in the queue, which is where every job starts.
|
|
||||||
*
|
|
||||||
* Built by hand rather than through `ConversionWorker.request` for the reason in the class
|
|
||||||
* KDoc; the display-name tag is included because `reattach()` reads it for the file card, and a
|
|
||||||
* job without one would exercise the `UNKNOWN_INPUT_NAME` fallback instead of this test's
|
|
||||||
* subject.
|
|
||||||
*/
|
|
||||||
private fun enqueueQueuedConversion(displayName: String = "holiday.mp4"): UUID {
|
|
||||||
val request = OneTimeWorkRequestBuilder<ConversionWorker>()
|
|
||||||
.addTag(JobTags.displayName(displayName))
|
|
||||||
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
|
|
||||||
.build()
|
|
||||||
workManager.enqueue(request).result.get()
|
|
||||||
return request.id
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun enqueueQueuedJoin(inputCount: Int = 2): UUID {
|
|
||||||
val request = OneTimeWorkRequestBuilder<ConcatWorker>()
|
|
||||||
.addTag(JobTags.inputCount(inputCount))
|
|
||||||
.setInitialDelay(QUEUE_HOLD_HOURS, TimeUnit.HOURS)
|
|
||||||
.build()
|
|
||||||
workManager.enqueue(request).result.get()
|
|
||||||
return request.id
|
|
||||||
}
|
|
||||||
|
|
||||||
private companion object {
|
|
||||||
/** Long enough that `TestScheduler` never releases the job during a test run. */
|
|
||||||
const val QUEUE_HOLD_HOURS = 1L
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,192 +0,0 @@
|
|||||||
package org.libremediaconverter.convert
|
|
||||||
|
|
||||||
import com.arthenica.ffmpegkit.MediaInformation
|
|
||||||
import com.arthenica.ffmpegkit.StreamInformation
|
|
||||||
import org.json.JSONObject
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertFalse
|
|
||||||
import org.junit.Assert.assertNull
|
|
||||||
import org.junit.Assert.assertTrue
|
|
||||||
import org.junit.Test
|
|
||||||
import org.junit.runner.RunWith
|
|
||||||
import org.libremediaconverter.model.Container
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
|
|
||||||
/**
|
|
||||||
* What FFprobe's answer means, read as a function of the answer alone.
|
|
||||||
*
|
|
||||||
* `readMediaInformation` was 114 missed instructions and 24 missed branches — the second-biggest
|
|
||||||
* block on the wave-4 report — of which **exactly one line needed a device**:
|
|
||||||
*
|
|
||||||
* ```kotlin
|
|
||||||
* FFprobeKit.getMediaInformation(path).getMediaInformation()
|
|
||||||
* ```
|
|
||||||
*
|
|
||||||
* Everything after it reads an ordinary object. `javap` over the committed AAR's runtime jar:
|
|
||||||
* `MediaInformation(JSONObject, List<StreamInformation>, List<Chapter>)` and
|
|
||||||
* `StreamInformation(JSONObject)` are plain public constructors, and neither class's `<clinit>`
|
|
||||||
* loads the native library — so the fixtures below are built without `libffmpegkit` present.
|
|
||||||
*
|
|
||||||
* ## The one that matters
|
|
||||||
*
|
|
||||||
* `containerFrom(formatName, video?.getCodec())`. FFprobe reports `matroska,webm` for **both** MKV
|
|
||||||
* and WebM, because they share a demuxer, so the video codec is the only thing separating them.
|
|
||||||
* `containerFrom` has thirty-three covered branches of its own and not one of them can notice the
|
|
||||||
* argument being dropped — the mistake would be at the call, not in the callee, and every existing
|
|
||||||
* `containerFrom` test would stay green while every VP9 WebM quietly became an MKV.
|
|
||||||
*
|
|
||||||
* Robolectric only for `org.json`, which is a stub in a plain JVM test.
|
|
||||||
*/
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class FFprobeMappingTest {
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `the video codec decides between matroska and webm`() {
|
|
||||||
assertEquals(
|
|
||||||
Container.WEBM,
|
|
||||||
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "vp9"))).container,
|
|
||||||
)
|
|
||||||
assertEquals(
|
|
||||||
Container.MKV,
|
|
||||||
MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "h264"))).container,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The same format name with no video stream at all, which is what makes the case above about
|
|
||||||
* the *argument* rather than about the format string.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a matroska container with no video track cannot be told from webm and is not guessed`() {
|
|
||||||
val read = MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("audio", "opus")))
|
|
||||||
|
|
||||||
assertEquals(Container.MKV, read.container)
|
|
||||||
assertNull(read.videoCodec)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `the first stream of each type wins`() {
|
|
||||||
val read = MediaProbe.ffprobeInfoFrom(
|
|
||||||
info(
|
|
||||||
"mov,mp4,m4a,3gp,3g2,mj2",
|
|
||||||
stream("video", "h264", width = 1920, height = 1080),
|
|
||||||
stream("video", "hevc", width = 640, height = 480),
|
|
||||||
stream("audio", "aac"),
|
|
||||||
stream("audio", "mp3"),
|
|
||||||
),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertEquals("h264", read.videoCodec)
|
|
||||||
assertEquals("aac", read.audioCodec)
|
|
||||||
assertEquals(1920, read.width)
|
|
||||||
assertEquals(1080, read.height)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* Dimensions come from the stream the codec came from, not from whichever stream has some.
|
|
||||||
*
|
|
||||||
* The fixture is deliberately awkward: the chosen video stream carries **no** dimensions and a
|
|
||||||
* later one does. That is a real shape — FFprobe omits `width`/`height` for a stream it could
|
|
||||||
* not measure — and it is the only arrangement that separates the two readings.
|
|
||||||
*
|
|
||||||
* A first version of this file asserted the dimensions inside the case above, where the chosen
|
|
||||||
* stream was also the first one carrying any. Replacing `video?.getWidth()` with
|
|
||||||
* `streams.firstNotNullOfOrNull { it.getWidth() }` gave the same answer there and **the
|
|
||||||
* mutation survived**. It reddens here.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a video stream with no dimensions reports none rather than borrowing another stream's`() {
|
|
||||||
val read = MediaProbe.ffprobeInfoFrom(
|
|
||||||
info(
|
|
||||||
"mov,mp4,m4a,3gp,3g2,mj2",
|
|
||||||
stream("video", "h264"),
|
|
||||||
stream("video", "hevc", width = 640, height = 480),
|
|
||||||
),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertEquals("h264", read.videoCodec)
|
|
||||||
assertEquals(0, read.width)
|
|
||||||
assertEquals(0, read.height)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* Stream order is the file's, not a promise. An audio-first container must read the same as a
|
|
||||||
* video-first one.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `an audio track listed first does not become the video track`() {
|
|
||||||
val read = MediaProbe.ffprobeInfoFrom(
|
|
||||||
info("mov,mp4,m4a,3gp,3g2,mj2", stream("audio", "aac"), stream("video", "h264")),
|
|
||||||
)
|
|
||||||
|
|
||||||
assertEquals("h264", read.videoCodec)
|
|
||||||
assertEquals("aac", read.audioCodec)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `a duration in seconds becomes milliseconds`() {
|
|
||||||
assertEquals(12_345L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "12.345")).durationMs)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* Both ways a duration can be absent, and neither may throw.
|
|
||||||
*
|
|
||||||
* FFprobe reports `"N/A"` for a stream it could not measure, and omits the key entirely for
|
|
||||||
* some containers. `toDoubleOrNull` is what keeps the second from being an exception on the
|
|
||||||
* file-pick path, where there is no user-visible failure to report it as.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a duration that is not a number is no duration rather than a crash`() {
|
|
||||||
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "N/A")).durationMs)
|
|
||||||
assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = null)).durationMs)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `a file with no streams reports nothing rather than defaults that look measured`() {
|
|
||||||
val read = MediaProbe.ffprobeInfoFrom(info("mp4"))
|
|
||||||
|
|
||||||
assertNull(read.videoCodec)
|
|
||||||
assertNull(read.audioCodec)
|
|
||||||
assertEquals(0, read.width)
|
|
||||||
assertEquals(0, read.height)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `an image format is reported as one`() {
|
|
||||||
assertTrue(MediaProbe.ffprobeInfoFrom(info("png_pipe", stream("video", "png"))).isImage)
|
|
||||||
assertFalse(MediaProbe.ffprobeInfoFrom(info("mp4", stream("video", "h264"))).isImage)
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun stream(type: String, codec: String, width: Int? = null, height: Int? = null) = StreamInformation(
|
|
||||||
JSONObject().apply {
|
|
||||||
put(StreamInformation.KEY_TYPE, type)
|
|
||||||
put(StreamInformation.KEY_CODEC, codec)
|
|
||||||
width?.let { put(StreamInformation.KEY_WIDTH, it) }
|
|
||||||
height?.let { put(StreamInformation.KEY_HEIGHT, it) }
|
|
||||||
},
|
|
||||||
)
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The format properties are **nested** under `"format"`, which is how FFprobe reports them and
|
|
||||||
* what `MediaInformation` reads: `getFormat()` resolves through `getStringFormatProperty`, not
|
|
||||||
* off the top-level object. A first version of this helper put the keys at the top level and
|
|
||||||
* every format-dependent case failed with a null container, which is worth recording here so
|
|
||||||
* the next fixture does not have to rediscover it.
|
|
||||||
*
|
|
||||||
* Streams are the other half and are *not* nested — they come from the constructor argument.
|
|
||||||
*/
|
|
||||||
private fun info(formatName: String, vararg streams: StreamInformation, duration: String? = "1.0") =
|
|
||||||
MediaInformation(
|
|
||||||
JSONObject().apply {
|
|
||||||
put(
|
|
||||||
MediaInformation.KEY_FORMAT_PROPERTIES,
|
|
||||||
JSONObject().apply {
|
|
||||||
put(MediaInformation.KEY_FORMAT, formatName)
|
|
||||||
duration?.let { put(MediaInformation.KEY_DURATION, it) }
|
|
||||||
},
|
|
||||||
)
|
|
||||||
},
|
|
||||||
streams.toList(),
|
|
||||||
emptyList(),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
@@ -51,13 +51,10 @@ import java.io.File
|
|||||||
* here needs. `OutputPublisherPublishTest` owns what a real publish writes.
|
* here needs. `OutputPublisherPublishTest` owns what a real publish writes.
|
||||||
* - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
|
* - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
|
||||||
* own what each state renders; this file owns what each state carries.
|
* own what each state renders; this file owns what each state carries.
|
||||||
* - ~~**`ConverterScreen`'s `destinationMime` line itself.**~~ **Withdrawn 2026-09-02 (#201).** The
|
* - **`ConverterScreen`'s `destinationMime` line itself.** It lives in the entry point, above the
|
||||||
* exemption read: "it lives in the entry point, above the `ScreenContent` seam, and reaching it
|
* `ScreenContent` seam, and reaching it needs a real ViewModel inside a composition. What it
|
||||||
* needs a real ViewModel inside a composition". That was true when written and is no longer:
|
* reads -- `pendingSave()?.mimeType` -- is asserted directly instead, which is why that
|
||||||
* `AdaptiveShellTest` (#173) established composing the real screens with real ViewModels, and
|
* derivation was moved out of the entry point in the first place.
|
||||||
* #200 added the `ShadowActivity` mechanics for reading what a launcher launched. `RetrySaveMimeTest`
|
|
||||||
* now asserts the line directly. What this file still owns is the half below the seam -- what each
|
|
||||||
* state *carries* -- which is why `pendingSave()?.mimeType` is also asserted here.
|
|
||||||
* - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state
|
* - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state
|
||||||
* without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither
|
* without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither
|
||||||
* branch renders a picker. It is a pre-existing path this change neither opens nor widens: the
|
* branch renders a picker. It is a pre-existing path this change neither opens nor widens: the
|
||||||
|
|||||||
@@ -205,34 +205,6 @@ class FileCardTest {
|
|||||||
assertNoRow("Length")
|
assertNoRow("Length")
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* A video the app knows a great deal about and cannot name the container of.
|
|
||||||
*
|
|
||||||
* Not an edge case. `InputProbe.container`'s own KDoc says `MediaExtractor` cannot report a
|
|
||||||
* container at all -- it comes from FFprobe -- so any run where FFprobe did not answer produces
|
|
||||||
* exactly this: real codec, real dimensions, real duration, `container = null`.
|
|
||||||
*
|
|
||||||
* **The twin was already tested and this one was not**, which is the argument for adding it.
|
|
||||||
* `FileCard` renders `probe.container?.label ?: "Unknown"` twice, once in the `AUDIO_ONLY`
|
|
||||||
* branch (`ConverterScreen.kt:660`) and once in the `VIDEO` branch (`:668`), and
|
|
||||||
* `an audio-only file nothing else could describe degrades one row at a time` drives only the
|
|
||||||
* first. Same expression, same fallback, one kind covered. That asymmetry is the same one
|
|
||||||
* `CLAUDE.md` records for including `ContainerCapabilities:94`.
|
|
||||||
*
|
|
||||||
* The other rows are asserted alongside so this is not a copy of the audio-only case: there,
|
|
||||||
* everything is unknown at once; here, one field is missing from a probe that is otherwise
|
|
||||||
* complete, and the rest must be unaffected by it.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a video file whose container nothing identified says so and keeps its other rows`() {
|
|
||||||
setFileCard(input(probe = VIDEO_PROBE.copy(container = null)))
|
|
||||||
|
|
||||||
assertRow("Container", "Unknown")
|
|
||||||
assertRow("Video", "${VideoCodec.H264.label} · 1920×1080")
|
|
||||||
assertRow("Audio", AudioCodec.AAC.label)
|
|
||||||
assertRow("Length", "1:30")
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The row is one node, not a label node beside a value node. A test matching on `"Container"`
|
* The row is one node, not a label node beside a value node. A test matching on `"Container"`
|
||||||
* alone would pass against either shape.
|
* alone would pass against either shape.
|
||||||
|
|||||||
@@ -1,141 +0,0 @@
|
|||||||
package org.libremediaconverter.convert
|
|
||||||
|
|
||||||
import android.app.Activity
|
|
||||||
import android.content.Intent
|
|
||||||
import android.net.Uri
|
|
||||||
import androidx.activity.ComponentActivity
|
|
||||||
import androidx.compose.ui.test.assertIsDisplayed
|
|
||||||
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
|
|
||||||
import androidx.compose.ui.test.onNodeWithTag
|
|
||||||
import androidx.compose.ui.test.performClick
|
|
||||||
import androidx.media3.common.util.UnstableApi
|
|
||||||
import androidx.work.Data
|
|
||||||
import org.junit.After
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Before
|
|
||||||
import org.junit.Rule
|
|
||||||
import org.junit.Test
|
|
||||||
import org.junit.runner.RunWith
|
|
||||||
import org.libremediaconverter.join.JoinScreen
|
|
||||||
import org.libremediaconverter.model.InputProbe
|
|
||||||
import org.libremediaconverter.ui.TestTags
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
import org.robolectric.RuntimeEnvironment
|
|
||||||
import org.robolectric.Shadows.shadowOf
|
|
||||||
import org.robolectric.shadows.ShadowActivity
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The launcher layer above the `ScreenContent` seam — registered, and until now never resulted.
|
|
||||||
*
|
|
||||||
* ## The hazard this exists for
|
|
||||||
*
|
|
||||||
* `ConversionViewModel.onInputPicked(uri: Uri)` and `.save(destination: Uri)` are **both
|
|
||||||
* `(Uri) -> Unit`**, so swapping the two launcher callbacks at `ConverterScreen.kt:70` and `:83`
|
|
||||||
* compiles, renders, and passes the entire suite. Picking a file would attempt a save to it, and
|
|
||||||
* choosing a destination would load it as input.
|
|
||||||
*
|
|
||||||
* That is precisely the defect class `ScreenWiringTest` exists for, on the one pair it declines to
|
|
||||||
* cover: it drives `converterActions` directly and says the launcher-backed actions stay
|
|
||||||
* parameters. Correct for the `actions` seam, and it leaves the edge above that seam unpinned.
|
|
||||||
*
|
|
||||||
* Join's equivalents (`JoinScreen.kt:45`, `:55`) are `List<Uri>` and `Uri`, so they are **not**
|
|
||||||
* transposable and need no such test. The picker filter is a different matter and is covered below
|
|
||||||
* for both screens.
|
|
||||||
*
|
|
||||||
* ## The two mechanics, verified before the assertions were written
|
|
||||||
*
|
|
||||||
* Neither is used anywhere else in the suite, so both were spiked first:
|
|
||||||
*
|
|
||||||
* - **Reading what was launched** — `shadowOf(activity).nextStartedActivityForResult`, which returns
|
|
||||||
* the `Intent` with its `EXTRA_MIME_TYPES` intact.
|
|
||||||
* - **Delivering a result** — `shadowOf(activity).receiveResult(...)`, which reaches
|
|
||||||
* `ComponentActivity`'s `ActivityResultRegistry` and fires the `rememberLauncherForActivityResult`
|
|
||||||
* callback.
|
|
||||||
*
|
|
||||||
* `createAndroidComposeRule`, as `AdaptiveShellTest` uses and for the reason it gives: the screens
|
|
||||||
* compose real ViewModels through `viewModel()`, and the plain rule supplies no `ViewModelStoreOwner`.
|
|
||||||
*/
|
|
||||||
@UnstableApi
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class LauncherWiringTest {
|
|
||||||
|
|
||||||
@get:Rule
|
|
||||||
val composeRule = createAndroidComposeRule<ComponentActivity>()
|
|
||||||
|
|
||||||
@Before
|
|
||||||
fun setUp() {
|
|
||||||
val app = RuntimeEnvironment.getApplication()
|
|
||||||
installTestWorkManager(app, Data.EMPTY)
|
|
||||||
// The real screen composes a real ViewModel; neither test here is about probing.
|
|
||||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
|
||||||
}
|
|
||||||
|
|
||||||
@After
|
|
||||||
fun tearDown() = ConversionDependencies.reset()
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The transposition guard. A picked file has to reach `onInputPicked`, which is observable as
|
|
||||||
* the screen arriving at `Ready` with the file card showing — `save()` from `Idle` returns at
|
|
||||||
* its own guard and leaves nothing behind.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a picked document is loaded as input rather than saved to`() {
|
|
||||||
composeRule.setContent { ConverterScreen() }
|
|
||||||
|
|
||||||
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
|
|
||||||
deliver(Uri.parse("content://test/holiday.mkv"))
|
|
||||||
|
|
||||||
composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME).assertIsDisplayed()
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* `ConverterScreen.kt:65-67` records why the all-types wildcard is load-bearing rather than lazy:
|
|
||||||
*
|
|
||||||
* > the picker is images and video only, offers no audio at all, and will not reliably surface
|
|
||||||
* > .mkv/.flac/.webm
|
|
||||||
*
|
|
||||||
* Narrowing it would make every audio conversion unreachable from the file picker, and nothing
|
|
||||||
* would have gone red. (The literal is spelled only in the assertion below: a KDoc cannot
|
|
||||||
* contain it, because the wildcard's second half closes the comment.)
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `the converter picker asks for every type, not just the ones a photo picker offers`() {
|
|
||||||
composeRule.setContent { ConverterScreen() }
|
|
||||||
|
|
||||||
composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick()
|
|
||||||
|
|
||||||
val intent = launched().intent
|
|
||||||
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
|
|
||||||
assertEquals(listOf("*/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `the join picker asks for video and accepts more than one file`() {
|
|
||||||
composeRule.setContent { JoinScreen() }
|
|
||||||
|
|
||||||
composeRule.onNodeWithTag(TestTags.Join.CHOOSE_FILES).performClick()
|
|
||||||
|
|
||||||
val intent = launched().intent
|
|
||||||
assertEquals(Intent.ACTION_OPEN_DOCUMENT, intent.action)
|
|
||||||
assertEquals(listOf("video/*"), intent.getStringArrayExtra(Intent.EXTRA_MIME_TYPES)?.toList())
|
|
||||||
// A join of one file is not a join; the contract is what asks for several.
|
|
||||||
assertEquals(true, intent.getBooleanExtra(Intent.EXTRA_ALLOW_MULTIPLE, false))
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun launched(): ShadowActivity.IntentForResult {
|
|
||||||
composeRule.waitForIdle()
|
|
||||||
return requireNotNull(shadowOf(composeRule.activity).nextStartedActivityForResult) {
|
|
||||||
"nothing was launched for a result"
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun deliver(uri: Uri) {
|
|
||||||
val started = launched()
|
|
||||||
shadowOf(composeRule.activity).receiveResult(
|
|
||||||
started.intent,
|
|
||||||
Activity.RESULT_OK,
|
|
||||||
Intent().setData(uri),
|
|
||||||
)
|
|
||||||
composeRule.waitForIdle()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,213 +0,0 @@
|
|||||||
package org.libremediaconverter.convert
|
|
||||||
|
|
||||||
import android.app.Application
|
|
||||||
import android.net.Uri
|
|
||||||
import androidx.media3.common.util.UnstableApi
|
|
||||||
import androidx.work.Data
|
|
||||||
import androidx.work.ListenableWorker
|
|
||||||
import androidx.work.testing.TestListenableWorkerBuilder
|
|
||||||
import androidx.work.workDataOf
|
|
||||||
import kotlinx.coroutines.Dispatchers
|
|
||||||
import kotlinx.coroutines.runBlocking
|
|
||||||
import org.junit.After
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertNotNull
|
|
||||||
import org.junit.Assert.assertTrue
|
|
||||||
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.ConversionRequest
|
|
||||||
import org.libremediaconverter.model.EnginePreference
|
|
||||||
import org.libremediaconverter.model.InputProbe
|
|
||||||
import org.libremediaconverter.model.OutputFormat
|
|
||||||
import org.libremediaconverter.work.ConcatWorker
|
|
||||||
import org.libremediaconverter.work.ConversionWorker
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
import org.robolectric.RuntimeEnvironment
|
|
||||||
import java.io.File
|
|
||||||
import java.util.UUID
|
|
||||||
|
|
||||||
/**
|
|
||||||
* A failure that says nothing still has to say something.
|
|
||||||
*
|
|
||||||
* Three sites, all `ci == 0` before this file, and all the same rule:
|
|
||||||
*
|
|
||||||
* ```
|
|
||||||
* work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE
|
|
||||||
* convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE
|
|
||||||
* join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE
|
|
||||||
* ```
|
|
||||||
*
|
|
||||||
* Every existing test throws *with* a message, so the right-hand side had never been evaluated
|
|
||||||
* anywhere in the suite. A `Throwable` carrying none is not exotic — `RuntimeException()`,
|
|
||||||
* `IOException()` and most platform exceptions raised without an argument all have a null message.
|
|
||||||
*
|
|
||||||
* ## Held in one class, against the ticket's suggestion
|
|
||||||
*
|
|
||||||
* #193 proposed putting each case beside the behaviour it neighbours. They are together instead,
|
|
||||||
* because they are one rule at three layers and because the trap below has to be explained once
|
|
||||||
* rather than three times. `FailedSaveRetryTest` sets the precedent for both ViewModels in one
|
|
||||||
* file; this extends it by one worker.
|
|
||||||
*
|
|
||||||
* ## The trap, which is why the worker case asserts what it does
|
|
||||||
*
|
|
||||||
* `ConversionStateMappingTest`'s *"a failure with nothing said still says something"* looks like it
|
|
||||||
* already covers the worker site. It does not: it drives the **read** side, `map(FAILED, Data.EMPTY)`,
|
|
||||||
* and that side has a fallback of its own (`ConversionViewModel.kt:147-149`):
|
|
||||||
*
|
|
||||||
* ```kotlin
|
|
||||||
* update.outputData.getString(ConversionWorker.KEY_ERROR)
|
|
||||||
* ?.takeIf { it.isNotBlank() }
|
|
||||||
* ?: ConversionWorker.GENERIC_FAILURE_MESSAGE
|
|
||||||
* ```
|
|
||||||
*
|
|
||||||
* So mutating the worker's fallback to `.orEmpty()` writes `KEY_ERROR to ""`, and the ViewModel
|
|
||||||
* turns that straight back into the same constant. **A test asserting on the resulting `Failed`
|
|
||||||
* state stays green under the mutation**, which is most likely why the write-side fallback survived
|
|
||||||
* three waves of test work. The worker case therefore reads `KEY_ERROR` off the worker's own
|
|
||||||
* `Result`, before anything downstream can repair it.
|
|
||||||
*
|
|
||||||
* The two save cases have no such second line: both write `_state.value` directly, so the state is
|
|
||||||
* the right thing to assert there.
|
|
||||||
*/
|
|
||||||
@UnstableApi
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class MessagelessFailureTest {
|
|
||||||
|
|
||||||
private lateinit var app: Application
|
|
||||||
private lateinit var publisher: RecordingPublisher
|
|
||||||
private lateinit var staged: File
|
|
||||||
|
|
||||||
@Before
|
|
||||||
fun setUp() {
|
|
||||||
app = RuntimeEnvironment.getApplication()
|
|
||||||
publisher = RecordingPublisher(app)
|
|
||||||
ConversionDependencies.publisher = { publisher }
|
|
||||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
|
||||||
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
|
|
||||||
}
|
|
||||||
|
|
||||||
@After
|
|
||||||
fun tearDown() {
|
|
||||||
ConversionDependencies.reset()
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The engine gives up without saying why, which is what a native crash looks like from here.
|
|
||||||
*
|
|
||||||
* Asserted on the worker's own output `Data` rather than on a screen — see the class KDoc.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `a conversion that fails without a message still reports one`() {
|
|
||||||
installTestWorkManager(app, Data.EMPTY)
|
|
||||||
ConversionDependencies.software = { MessagelessTranscoder }
|
|
||||||
|
|
||||||
val result = runBlocking { failingWorker().doWork() }
|
|
||||||
|
|
||||||
assertTrue("the job must fail rather than retry, got $result", result is ListenableWorker.Result.Failure)
|
|
||||||
assertEquals(
|
|
||||||
"a failure with no message must still put something on screen",
|
|
||||||
ConversionWorker.GENERIC_FAILURE_MESSAGE,
|
|
||||||
(result as ListenableWorker.Result.Failure).outputData.getString(ConversionWorker.KEY_ERROR),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `a save that fails without a message still reports one`() {
|
|
||||||
installTestWorkManager(app, conversionOutput())
|
|
||||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
|
||||||
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
|
|
||||||
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
|
||||||
viewModel.convert()
|
|
||||||
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
|
|
||||||
|
|
||||||
publisher.publishFailure = RuntimeException()
|
|
||||||
viewModel.save(DESTINATION)
|
|
||||||
|
|
||||||
val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed
|
|
||||||
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
|
|
||||||
// The handle travels even on the wordless path. Without this, a fallback that also dropped
|
|
||||||
// `pending` would pass -- and the file would be unreachable from the screen that just said
|
|
||||||
// the save failed.
|
|
||||||
assertNotNull("a wordless failure must still offer the file again", failed.retry)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `a join save that fails without a message still reports one`() {
|
|
||||||
installTestWorkManager(app, joinOutput())
|
|
||||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
|
||||||
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
|
|
||||||
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
|
|
||||||
viewModel.join()
|
|
||||||
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
|
|
||||||
|
|
||||||
publisher.publishFailure = RuntimeException()
|
|
||||||
viewModel.save(DESTINATION)
|
|
||||||
|
|
||||||
val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed
|
|
||||||
assertEquals(SAVE_FAILED_MESSAGE, failed.message)
|
|
||||||
assertNotNull("a wordless failure must still offer the file again", failed.retry)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* `FORCE_SOFTWARE` so the failure comes straight out of `runFFmpeg`.
|
|
||||||
*
|
|
||||||
* `AUTO` would enter `runMedia3OrFallBack`, whose catch runs the job a second time in software
|
|
||||||
* — the same exception would arrive, but through a path this test is not about and which
|
|
||||||
* `HardwareFallbackTest` already owns.
|
|
||||||
*/
|
|
||||||
private fun failingWorker(): ConversionWorker {
|
|
||||||
val spec = OutputFormat.MP4_H265.spec
|
|
||||||
return TestListenableWorkerBuilder<ConversionWorker>(
|
|
||||||
context = app,
|
|
||||||
inputData = workDataOf(
|
|
||||||
ConversionWorker.KEY_INPUT_URI to "file:///tmp/holiday.mp4",
|
|
||||||
ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4",
|
|
||||||
ConversionWorker.KEY_CONTAINER to spec.container.name,
|
|
||||||
ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name,
|
|
||||||
ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name,
|
|
||||||
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
|
|
||||||
),
|
|
||||||
runAttemptCount = 0,
|
|
||||||
).setId(JOB_ID).build()
|
|
||||||
}
|
|
||||||
|
|
||||||
private fun conversionOutput() = workDataOf(
|
|
||||||
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
|
||||||
ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
|
|
||||||
ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
|
|
||||||
)
|
|
||||||
|
|
||||||
private fun joinOutput() = workDataOf(
|
|
||||||
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
|
||||||
ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
|
|
||||||
ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
|
|
||||||
)
|
|
||||||
|
|
||||||
private companion object {
|
|
||||||
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
|
|
||||||
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000019a")
|
|
||||||
const val SUGGESTED_NAME = "holiday.mp4"
|
|
||||||
const val JOB_MIME_TYPE = "video/mp4"
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* An engine that gives up without saying why.
|
|
||||||
*
|
|
||||||
* `RuntimeException()` rather than a subclass with a blank message: `Throwable.message` is *null*
|
|
||||||
* here, which is the case the elvis exists for. A blank-but-present message takes the left-hand
|
|
||||||
* side and is a different path — `ConversionStateMappingTest` covers that one, on the read side.
|
|
||||||
*/
|
|
||||||
@UnstableApi
|
|
||||||
private object MessagelessTranscoder : SoftwareTranscoder {
|
|
||||||
override suspend fun run(
|
|
||||||
request: ConversionRequest,
|
|
||||||
inputPath: String,
|
|
||||||
output: File,
|
|
||||||
durationMs: Long,
|
|
||||||
onProgress: (Int) -> Unit,
|
|
||||||
): Unit = throw RuntimeException()
|
|
||||||
}
|
|
||||||
@@ -122,25 +122,24 @@ class OutputPublisherStagingTest {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Makes `cacheDir/conversions` a regular file, which is the whole precondition of the test
|
* 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 once lost a race that CI
|
* above -- and does it in a loop, because a single delete-then-write loses a race that CI
|
||||||
* caught and this machine did not reproduce.
|
* caught and this machine does not reproduce.
|
||||||
*
|
*
|
||||||
* **That race is closed at the source as of #159, and the loop is kept anyway.**
|
* `LibreMediaConverterApp.onCreate` ends with
|
||||||
* `LibreMediaConverterApp.onCreate` launched its staging sweep on `Dispatchers.IO`, and
|
* `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`, and
|
||||||
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric builds an
|
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric instantiates
|
||||||
* application for every test class that asks for one, so that background `mkdirs()` was in
|
* the application for every test that asks for one, so that background `mkdirs()` is in flight
|
||||||
* flight across the whole suite, on a thread the paused main looper does not control. Between
|
* across the whole suite, on a thread the paused main looper does not control. Between deleting
|
||||||
* deleting this path and writing it there is a window where the path does not exist and that
|
* this path and writing it there is a window where the path does not exist and that `mkdirs()`
|
||||||
* `mkdirs()` could win -- `FileNotFoundException: ... (Is a directory)` out of `writeBytes`,
|
* can win, which is `FileNotFoundException: ... (Is a directory)` out of `writeBytes` -- run
|
||||||
* run 33069641674 on #149, once, against 468 tests that passed here. The JVM suite now runs
|
* 33069641674 on #149, once, against 468 tests that pass here.
|
||||||
* `TestLibreMediaConverterApp`, whose sweep finishes before `onCreate()` returns, so nothing is
|
|
||||||
* sweeping while a test body runs.
|
|
||||||
*
|
*
|
||||||
* The loop stays because it is what would catch that substitution being undone. Without it the
|
* Retrying closes it rather than narrowing it, because the race is not symmetric: `mkdirs()`
|
||||||
* regression returns as this one class failing rarely on CI -- the exact shape that took #159
|
* fails on an existing regular file, so the invariant only has to survive being *established*.
|
||||||
* from a single run on #149 to a wave-4 flake before anyone chased it. Retrying closes the
|
* Once a write lands, nothing in the suite can turn this back into a directory.
|
||||||
* 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*.
|
* The wider problem -- application-scope IO work racing every Robolectric test that shares
|
||||||
|
* `cacheDir` -- is #159, and is deliberately not fixed here.
|
||||||
*/
|
*/
|
||||||
private fun stagingPathAsRegularFile(): File {
|
private fun stagingPathAsRegularFile(): File {
|
||||||
val stagingPath = File(cacheDir, "conversions")
|
val stagingPath = File(cacheDir, "conversions")
|
||||||
|
|||||||
@@ -1,136 +0,0 @@
|
|||||||
package org.libremediaconverter.convert
|
|
||||||
|
|
||||||
import android.app.Application
|
|
||||||
import android.content.Intent
|
|
||||||
import android.net.Uri
|
|
||||||
import androidx.activity.ComponentActivity
|
|
||||||
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
|
|
||||||
import androidx.compose.ui.test.onNodeWithTag
|
|
||||||
import androidx.compose.ui.test.performClick
|
|
||||||
import androidx.compose.ui.test.performScrollTo
|
|
||||||
import androidx.media3.common.util.UnstableApi
|
|
||||||
import androidx.work.WorkManager
|
|
||||||
import androidx.work.workDataOf
|
|
||||||
import org.junit.After
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertNotEquals
|
|
||||||
import org.junit.Before
|
|
||||||
import org.junit.Rule
|
|
||||||
import org.junit.Test
|
|
||||||
import org.junit.runner.RunWith
|
|
||||||
import org.libremediaconverter.model.InputProbe
|
|
||||||
import org.libremediaconverter.model.OutputFormat
|
|
||||||
import org.libremediaconverter.ui.TestTags
|
|
||||||
import org.libremediaconverter.work.ConversionWorker
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
import org.robolectric.RuntimeEnvironment
|
|
||||||
import org.robolectric.Shadows.shadowOf
|
|
||||||
import java.io.File
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The save dialog opens with the type the *job* produced, not the type the picker is showing now.
|
|
||||||
*
|
|
||||||
* `ConverterScreen.kt:80` — `state.pendingSave()?.mimeType ?: settings.spec.mimeType` — had never
|
|
||||||
* taken its left-hand side. Its comment records what the line is for:
|
|
||||||
*
|
|
||||||
* > a retry offered after a failed save opens the dialog with the type its first attempt used —
|
|
||||||
* > the cast answered null for a `Failed`, and the fallback below is the current picker, which a
|
|
||||||
* > reattached job never set.
|
|
||||||
*
|
|
||||||
* So the untested half is the fix, and the tested half is the fallback it was added to stop being
|
|
||||||
* used.
|
|
||||||
*
|
|
||||||
* ## This revises a named exemption, deliberately
|
|
||||||
*
|
|
||||||
* `FailedSaveRetryTest`'s KDoc lists this line under "Not asserted here, so each is a decision
|
|
||||||
* rather than an omission":
|
|
||||||
*
|
|
||||||
* > It lives in the entry point, above the `ScreenContent` seam, and reaching it needs a real
|
|
||||||
* > ViewModel inside a composition.
|
|
||||||
*
|
|
||||||
* That was true when written. `AdaptiveShellTest` (#173) then established exactly that capability,
|
|
||||||
* and #200 added the two `ShadowActivity` mechanics that let a test read what a launcher launched.
|
|
||||||
* The reason the exemption gave no longer holds, so the exemption is withdrawn rather than left to
|
|
||||||
* be taken at face value — the same shape as #141 revising #84's boundary. That KDoc is corrected
|
|
||||||
* in this change.
|
|
||||||
*
|
|
||||||
* ## Why the job is reattached rather than run
|
|
||||||
*
|
|
||||||
* The screen composes its own ViewModel through `viewModel()`, so nothing can be injected into it.
|
|
||||||
* A job finished before the composition is the one route to a `Converted` state carrying output
|
|
||||||
* `Data` this test chose — and it is also the case the line exists for, since a reattached job's
|
|
||||||
* spec "was never in these settings at all".
|
|
||||||
*/
|
|
||||||
@UnstableApi
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class RetrySaveMimeTest {
|
|
||||||
|
|
||||||
@get:Rule
|
|
||||||
val composeRule = createAndroidComposeRule<ComponentActivity>()
|
|
||||||
|
|
||||||
private lateinit var app: Application
|
|
||||||
private lateinit var staged: File
|
|
||||||
|
|
||||||
@Before
|
|
||||||
fun setUp() {
|
|
||||||
app = RuntimeEnvironment.getApplication()
|
|
||||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
|
||||||
staged = OutputPublisher(app).createStagingFile("holiday.mkv").apply { writeBytes(ByteArray(4096)) }
|
|
||||||
}
|
|
||||||
|
|
||||||
@After
|
|
||||||
fun tearDown() = ConversionDependencies.reset()
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun `the save dialog offers the type the job produced, not the one the picker is showing`() {
|
|
||||||
finishAJobProducing(JOB_MIME_TYPE)
|
|
||||||
composeRule.setContent { ConverterScreen() }
|
|
||||||
composeRule.waitForIdle()
|
|
||||||
|
|
||||||
composeRule.onNodeWithTag(TestTags.SAVE_FILE).performScrollTo().performClick()
|
|
||||||
composeRule.waitForIdle()
|
|
||||||
|
|
||||||
val intent = requireNotNull(shadowOf(composeRule.activity).nextStartedActivityForResult) {
|
|
||||||
"the save dialog was never launched"
|
|
||||||
}.intent
|
|
||||||
assertEquals(Intent.ACTION_CREATE_DOCUMENT, intent.action)
|
|
||||||
assertEquals(JOB_MIME_TYPE, intent.type)
|
|
||||||
// The fixture is only meaningful while the two differ; without this the assertion above
|
|
||||||
// would pass just as well against the fallback.
|
|
||||||
assertNotEquals(
|
|
||||||
"the picker's own type must differ, or this test proves nothing",
|
|
||||||
JOB_MIME_TYPE,
|
|
||||||
OutputFormat.MP4_H265.spec.mimeType,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* A conversion that finished while nothing was watching, which is what `reattach()` picks up.
|
|
||||||
*
|
|
||||||
* `SucceedingWorkerFactory` reports this output `Data` for whatever is enqueued, so the job
|
|
||||||
* lands `SUCCEEDED` carrying a staged path that exists — the two things `Reattachment.choose`
|
|
||||||
* requires of a finished job.
|
|
||||||
*/
|
|
||||||
private fun finishAJobProducing(mimeType: String) {
|
|
||||||
installTestWorkManager(
|
|
||||||
app,
|
|
||||||
workDataOf(
|
|
||||||
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
|
||||||
ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mkv",
|
|
||||||
ConversionWorker.KEY_MIME_TYPE to mimeType,
|
|
||||||
),
|
|
||||||
)
|
|
||||||
WorkManager.getInstance(app).enqueue(
|
|
||||||
ConversionWorker.request(
|
|
||||||
inputUri = Uri.parse("content://test/holiday.mkv"),
|
|
||||||
displayName = "holiday.mkv",
|
|
||||||
sizeBytes = 4_096L,
|
|
||||||
),
|
|
||||||
).result.get()
|
|
||||||
}
|
|
||||||
|
|
||||||
private companion object {
|
|
||||||
/** Matroska, against the MP4 the picker defaults to. */
|
|
||||||
const val JOB_MIME_TYPE = "video/x-matroska"
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,147 +0,0 @@
|
|||||||
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")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -190,30 +190,6 @@ class FFmpegCommandBuilderTest {
|
|||||||
assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k")
|
assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k")
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Turning audio off, which the Advanced picker offers and nothing had ever built a command for.
|
|
||||||
*
|
|
||||||
* `audioArgs`' `Drop` arm was `ci == 0`. The suite's only `-an` assertion is in
|
|
||||||
* `gif generates a palette to avoid banding and drops audio`, and that one comes from the image
|
|
||||||
* path (`FFmpegCommandBuilder.kt:79`/`:90`), which emits `-an` directly and never reaches
|
|
||||||
* `audioArgs`. Two sites, one string, one tested.
|
|
||||||
*
|
|
||||||
* It is a live path rather than defensive code: `AdvancedPicker` renders all of
|
|
||||||
* `AudioCodec.entries` including `NONE`, `ContainerCapabilities.validate` permits audio-off
|
|
||||||
* whenever the input has video, and MKV routes the job to FFmpeg.
|
|
||||||
*
|
|
||||||
* Both halves are asserted. `-an` alone would still pass if the arm fell through to the `else`
|
|
||||||
* and emitted an AAC encoder beside it -- a file that is silent because the flag won, carrying
|
|
||||||
* an encoder nobody asked for.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `turning audio off drops the track instead of encoding one`() {
|
|
||||||
val args = cmd(OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.NONE))
|
|
||||||
|
|
||||||
assertTrue("audio turned off must emit -an, got $args", args.contains("-an"))
|
|
||||||
assertFalse("a dropped track must not also carry an encoder, got $args", args.contains("-c:a"))
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `audio only formats never carry a video encoder`() {
|
fun `audio only formats never carry a video encoder`() {
|
||||||
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
|
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
|
||||||
|
|||||||
@@ -1,127 +0,0 @@
|
|||||||
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 })
|
|
||||||
}
|
|
||||||
@@ -1,89 +0,0 @@
|
|||||||
package org.libremediaconverter.ui.theme
|
|
||||||
|
|
||||||
import androidx.compose.material3.ColorScheme
|
|
||||||
import androidx.compose.material3.MaterialTheme
|
|
||||||
import androidx.compose.ui.test.junit4.v2.createComposeRule
|
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertNotEquals
|
|
||||||
import org.junit.Rule
|
|
||||||
import org.junit.Test
|
|
||||||
import org.junit.runner.RunWith
|
|
||||||
import org.robolectric.RobolectricTestRunner
|
|
||||||
import org.robolectric.annotation.Config
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The theme called the way the app calls it: with no arguments at all.
|
|
||||||
*
|
|
||||||
* [ThemeColorSchemeTest] resolves every branch of the `when` and always passes `darkTheme`
|
|
||||||
* explicitly, so the `$default` bridge is never entered and **`isSystemInDarkTheme()` is never
|
|
||||||
* called**. `MainActivity.kt:79` is its only default-argument caller and does not execute on the
|
|
||||||
* JVM, which left the app's actual call shape the one nothing exercised —
|
|
||||||
* `LibreMediaConverterTheme` reported `mi=21, mb=6, cb=12` at method level.
|
|
||||||
*
|
|
||||||
* ## Not #68
|
|
||||||
*
|
|
||||||
* #68 is about the two **unreachable** arms, `DarkColorScheme` and `LightColorScheme`, which cannot
|
|
||||||
* run because `dynamicColor` is always `true` and nothing can flip it. That is an open product
|
|
||||||
* decision. This is the reachable half — whether the default follows the system — and closing it
|
|
||||||
* does not close that.
|
|
||||||
*
|
|
||||||
* ## Why the assertion compares schemes rather than reading a number
|
|
||||||
*
|
|
||||||
* A luminance threshold would be a guess about the device palette. What is asserted instead is that
|
|
||||||
* the no-argument call resolves to **the same scheme** an explicit `darkTheme` of the matching
|
|
||||||
* value does, and a different one from its opposite. That holds whatever palette the platform
|
|
||||||
* hands back, and it is exactly the claim: the default reads the system rather than picking a side.
|
|
||||||
*
|
|
||||||
* Both schemes are resolved in one composition because `setContent` may be called once per test.
|
|
||||||
*/
|
|
||||||
@RunWith(RobolectricTestRunner::class)
|
|
||||||
class ThemeFollowsSystemTest {
|
|
||||||
|
|
||||||
@get:Rule
|
|
||||||
val composeRule = createComposeRule()
|
|
||||||
|
|
||||||
@Test
|
|
||||||
@Config(qualifiers = "+night")
|
|
||||||
fun `with no arguments the theme follows a system in dark mode`() {
|
|
||||||
val resolved = resolve()
|
|
||||||
|
|
||||||
assertEquals("the default must resolve what darkTheme = true does", resolved.dark, resolved.bare)
|
|
||||||
assertNotEquals(resolved.light, resolved.bare)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
@Config(qualifiers = "+notnight")
|
|
||||||
fun `with no arguments the theme follows a system in light mode`() {
|
|
||||||
val resolved = resolve()
|
|
||||||
|
|
||||||
assertEquals("the default must resolve what darkTheme = false does", resolved.light, resolved.bare)
|
|
||||||
assertNotEquals(resolved.dark, resolved.bare)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The three colours are read together as one value, because any single one could coincide
|
|
||||||
* between the two schemes on some palette while the schemes themselves differ. Background is
|
|
||||||
* what dark mode is chiefly about; primary and surface are along to make a coincidence
|
|
||||||
* implausible rather than merely unlikely.
|
|
||||||
*/
|
|
||||||
private data class Fingerprint(val background: Long, val primary: Long, val surface: Long)
|
|
||||||
|
|
||||||
private fun ColorScheme.fingerprint() =
|
|
||||||
Fingerprint(background.value.toLong(), primary.value.toLong(), surface.value.toLong())
|
|
||||||
|
|
||||||
private class Resolved(val bare: Fingerprint, val dark: Fingerprint, val light: Fingerprint)
|
|
||||||
|
|
||||||
private fun resolve(): Resolved {
|
|
||||||
lateinit var bare: Fingerprint
|
|
||||||
lateinit var dark: Fingerprint
|
|
||||||
lateinit var light: Fingerprint
|
|
||||||
composeRule.setContent {
|
|
||||||
// No arguments — the call MainActivity makes, and the one nothing exercised.
|
|
||||||
LibreMediaConverterTheme { bare = MaterialTheme.colorScheme.fingerprint() }
|
|
||||||
LibreMediaConverterTheme(darkTheme = true) { dark = MaterialTheme.colorScheme.fingerprint() }
|
|
||||||
LibreMediaConverterTheme(darkTheme = false) { light = MaterialTheme.colorScheme.fingerprint() }
|
|
||||||
}
|
|
||||||
composeRule.waitForIdle()
|
|
||||||
return Resolved(bare, dark, light)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -21,10 +21,8 @@ import org.junit.Before
|
|||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
import org.junit.runner.RunWith
|
import org.junit.runner.RunWith
|
||||||
import org.libremediaconverter.convert.ConversionDependencies
|
import org.libremediaconverter.convert.ConversionDependencies
|
||||||
import org.libremediaconverter.convert.HardwareTranscoder
|
|
||||||
import org.libremediaconverter.convert.SoftwareTranscoder
|
import org.libremediaconverter.convert.SoftwareTranscoder
|
||||||
import org.libremediaconverter.convert.installTestWorkManager
|
import org.libremediaconverter.convert.installTestWorkManager
|
||||||
import org.libremediaconverter.model.Container
|
|
||||||
import org.libremediaconverter.model.ConversionRequest
|
import org.libremediaconverter.model.ConversionRequest
|
||||||
import org.libremediaconverter.model.DeviceCodecs
|
import org.libremediaconverter.model.DeviceCodecs
|
||||||
import org.libremediaconverter.model.EnginePreference
|
import org.libremediaconverter.model.EnginePreference
|
||||||
@@ -131,40 +129,6 @@ class ProgressNotificationTest {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* The same plumbing on the engine most conversions actually use, which had none.
|
|
||||||
*
|
|
||||||
* `ConversionWorker.kt:208-210` is a second `onProgress` lambda at a second call site — the one
|
|
||||||
* handed to `engine.transcode` — and it reported `ci == 0`. Every test above drives the FFmpeg
|
|
||||||
* path; `HardwareFallbackTest` reaches `runMedia3OrFallBack` but its recording transcoder
|
|
||||||
* records the call and never invokes the callback it was given. So the two engines' progress
|
|
||||||
* wiring was one tested and one not, and the untested one is the default: `ConversionRouter`
|
|
||||||
* sends everything it can to Media3.
|
|
||||||
*
|
|
||||||
* `AUTO` with a real H.264 probe, because `FORCE_SOFTWARE` is precisely what keeps the other
|
|
||||||
* tests out of this branch. The probe and the permissive codec profile are what let the router
|
|
||||||
* choose Media3 at all — `InputProbe()` reports `UNPARSEABLE`, which routes straight to FFmpeg.
|
|
||||||
*
|
|
||||||
* Asserted on the *percentage*, not merely on an update having happened: `publishProgress`
|
|
||||||
* takes a display name and a percent, and replacing the percent with a constant compiles.
|
|
||||||
*/
|
|
||||||
@Test
|
|
||||||
fun `progress from the hardware engine reaches WorkManager the same way FFmpeg's does`() {
|
|
||||||
ConversionDependencies.probe = { _, _ -> H264_SOURCE }
|
|
||||||
val reporting = ReportingHardwareTranscoder { onProgress -> onProgress(PERCENT) }
|
|
||||||
ConversionDependencies.hardware = { reporting }
|
|
||||||
|
|
||||||
runBlocking { workerReporting(EnginePreference.AUTO) { }.doWork() }
|
|
||||||
|
|
||||||
assertEquals("the job must have gone to the hardware engine", 1, reporting.attempts)
|
|
||||||
val progressUpdates = updater.infos.drop(1)
|
|
||||||
assertEquals("one throttled progress update expected", 1, progressUpdates.size)
|
|
||||||
assertEquals(
|
|
||||||
PERCENT,
|
|
||||||
progressUpdates.single().notification.extras.getInt(Notification.EXTRA_PROGRESS),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* A worker routed to the software engine, whose engine is [report] and a written output.
|
* A worker routed to the software engine, whose engine is [report] and a written output.
|
||||||
*
|
*
|
||||||
@@ -173,10 +137,7 @@ class ProgressNotificationTest {
|
|||||||
* bridge, which is native. [report] is handed the worker's own progress callback, and runs with
|
* bridge, which is native. [report] is handed the worker's own progress callback, and runs with
|
||||||
* the worker as its receiver so a test can stop it mid-transcode.
|
* the worker as its receiver so a test can stop it mid-transcode.
|
||||||
*/
|
*/
|
||||||
private fun workerReporting(
|
private fun workerReporting(report: ConversionWorker.((Int) -> Unit) -> Unit): ConversionWorker {
|
||||||
enginePreference: EnginePreference = EnginePreference.FORCE_SOFTWARE,
|
|
||||||
report: ConversionWorker.((Int) -> Unit) -> Unit,
|
|
||||||
): ConversionWorker {
|
|
||||||
val worker = TestListenableWorkerBuilder<ConversionWorker>(
|
val worker = TestListenableWorkerBuilder<ConversionWorker>(
|
||||||
context = app,
|
context = app,
|
||||||
inputData = workDataOf(
|
inputData = workDataOf(
|
||||||
@@ -186,7 +147,7 @@ class ProgressNotificationTest {
|
|||||||
ConversionWorker.KEY_CONTAINER to SPEC.container.name,
|
ConversionWorker.KEY_CONTAINER to SPEC.container.name,
|
||||||
ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name,
|
ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name,
|
||||||
ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name,
|
ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name,
|
||||||
ConversionWorker.KEY_ENGINE_PREFERENCE to enginePreference.name,
|
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
|
||||||
),
|
),
|
||||||
runAttemptCount = 0,
|
runAttemptCount = 0,
|
||||||
).setId(JOB_ID)
|
).setId(JOB_ID)
|
||||||
@@ -210,17 +171,6 @@ class ProgressNotificationTest {
|
|||||||
const val TICKS = 50
|
const val TICKS = 50
|
||||||
val SPEC = OutputFormat.MP4_H265.spec
|
val SPEC = OutputFormat.MP4_H265.spec
|
||||||
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
|
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
|
||||||
|
|
||||||
/**
|
|
||||||
* A probe the router can actually route. `InputProbe()` reports `UNPARSEABLE`, which
|
|
||||||
* `PERMISSIVE.canDecode` refuses, so every job would reach FFmpeg with no test saying why.
|
|
||||||
*/
|
|
||||||
val H264_SOURCE = InputProbe(
|
|
||||||
videoCodec = "h264",
|
|
||||||
audioCodec = "aac",
|
|
||||||
container = Container.MP4,
|
|
||||||
durationMs = 1_000,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -261,22 +211,3 @@ private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) :
|
|||||||
const val OUTPUT_BYTES = 512
|
const val OUTPUT_BYTES = 512
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/** A hardware engine that reports whatever [report] wants reported, then writes an output. */
|
|
||||||
@UnstableApi
|
|
||||||
private class ReportingHardwareTranscoder(private val report: ((Int) -> Unit) -> Unit) : HardwareTranscoder {
|
|
||||||
|
|
||||||
var attempts = 0
|
|
||||||
|
|
||||||
override suspend fun transcode(input: Uri, output: File, request: ConversionRequest, onProgress: (Int) -> Unit) {
|
|
||||||
attempts++
|
|
||||||
report(onProgress)
|
|
||||||
output.writeBytes(ByteArray(OUTPUT_BYTES))
|
|
||||||
}
|
|
||||||
|
|
||||||
override fun close() = Unit
|
|
||||||
|
|
||||||
private companion object {
|
|
||||||
const val OUTPUT_BYTES = 16
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -10,9 +10,3 @@
|
|||||||
# Set here rather than in a @Config on each class so a later Robolectric test does not have
|
# 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.
|
# to rediscover it. Remove it once Robolectric ships an android-all jar for 37.
|
||||||
sdk=36
|
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
|
|
||||||
|
|||||||
+17
-233
@@ -1,27 +1,22 @@
|
|||||||
# Coverage-read findings
|
# Coverage-read findings
|
||||||
|
|
||||||
**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added
|
**Status:** five findings, none fixed, none urgent. F5 was added on 2026-08-27, found while decomposing #132 into children — it had been listed there as a test gap, and is not one. Every entry here is a *code* observation —
|
||||||
on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every
|
something a test would document rather than repair. The test gaps found in the same read are
|
||||||
entry here is a *code* observation — something a test would document rather than repair. The test
|
tickets #132 and #133, not entries here; see [Not covered here](#not-covered-here).
|
||||||
gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here).
|
**Scope:** what a JaCoCo read on 2026-08-26 turned up that writing a test would not fix. This is
|
||||||
**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a
|
a survey, not a work order. Acting on any entry is a separate decision and would be its own commit.
|
||||||
work order. Acting on any entry is a separate decision and would be its own commit.
|
**Last verified:** `main` at `dc8b7c3`, 2026-08-26. Coverage re-measured that day with
|
||||||
**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with
|
`./gradlew :app:jacocoTestReport`: **84.9% line (1971/2321), 63.8% branch (900/1410)**, against
|
||||||
`./gradlew :app:jacocoTestReport`: **92.8% line (2183/2352), 81.3% branch (1091/1342)**, against
|
**456 JVM tests in 68 classes**. `CLAUDE.md` quotes 454 in 67 from four hours earlier; the
|
||||||
**584 JVM tests in 87 classes**, matching what `CLAUDE.md` quotes.
|
percentages are unchanged, so no figure there is stale.
|
||||||
|
|
||||||
The wave-4 read that produced F6-F10 also produced twelve test tickets, **#192-#203**, plus **#204**
|
|
||||||
for four candidates whose cost was not obviously worth paying. The split between them is the same one
|
|
||||||
this document has always drawn: a ticket is where a test goes, an entry here is where a test would not
|
|
||||||
help.
|
|
||||||
|
|
||||||
## Why this document is separate from `defect-audit.md`
|
## Why this document is separate from `defect-audit.md`
|
||||||
|
|
||||||
`defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that
|
`defect-audit.md` is the record of the 2026-08-22 defect sweep: sixteen entries, each a thing that
|
||||||
is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be
|
is *wrong at runtime*. Nothing here is wrong at runtime today. These are arms that cannot be
|
||||||
reached, accessors nobody calls, and two KDocs that contradict the code beside them — the category
|
reached, accessors nobody calls, and one KDoc that contradicts the code beside it — the category
|
||||||
`defect-audit.md` calls **latent**, plus several that are not defects at all and are recorded so the
|
`defect-audit.md` calls **latent**, plus one that is not a defect at all and is recorded so the
|
||||||
next coverage read does not re-file them.
|
next coverage read does not re-file it.
|
||||||
|
|
||||||
They are here rather than in that document because folding them in would inflate a sixteen-entry
|
They are here rather than in that document because folding them in would inflate a sixteen-entry
|
||||||
audit whose status metadata has already gone stale once, and because they share a provenance:
|
audit whose status metadata has already gone stale once, and because they share a provenance:
|
||||||
@@ -29,7 +24,7 @@ every one fell out of reading a coverage report, and every one is the kind of th
|
|||||||
report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed
|
report is *good* at surfacing and a test is bad at fixing. F5 is the clearest case — it was filed
|
||||||
as a test gap first, and only stopped being one when someone went looking for its callers.
|
as a test gap first, and only stopped being one when someone went looking for its callers.
|
||||||
|
|
||||||
Entry ids are `F1`–`F10` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
|
Entry ids are `F1`–`F5` so they cannot be confused with `defect-audit.md`'s `D1`–`D16`.
|
||||||
|
|
||||||
## How to read the confidence labels
|
## How to read the confidence labels
|
||||||
|
|
||||||
@@ -267,179 +262,6 @@ no way to make it happen now.
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## F6 — Four more arms that cannot be reached, and one KDoc among them that is false
|
|
||||||
|
|
||||||
**Severity: low · Confirmed by inspection · F4's family, found in the wave-4 read**
|
|
||||||
|
|
||||||
```
|
|
||||||
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:178-179
|
|
||||||
app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt:221
|
|
||||||
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:297
|
|
||||||
app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt:324, :340
|
|
||||||
```
|
|
||||||
|
|
||||||
Four sites that a coverage report flags and that no test can reach. Each is recorded with the
|
|
||||||
upstream guard that makes it unreachable, because that guard is what would have to change first.
|
|
||||||
|
|
||||||
- **`ConversionRouter:178-179`** — the missed branch is `orEmpty()`'s absent-key arm on
|
|
||||||
`MEDIA3_MUXABLE_VIDEO[plan.container]`. `MEDIA3_CONTAINERS` is `setOf(MP4)` and `route()` returns at
|
|
||||||
`:104` for anything else, so `media3CanMux` only ever sees MP4, which both maps key. Same function
|
|
||||||
as F4's second pair, one line below it.
|
|
||||||
- **`ConversionRouter:221`** — `DeviceCodecs.PERMISSIVE.canDecode` returning **false** for
|
|
||||||
`InputProbe.UNPARSEABLE`. `PERMISSIVE` has no production caller at all (tests only), and the
|
|
||||||
router's one `canDecode` call at `:128` is already preceded by `:117` returning FFMPEG for
|
|
||||||
`UNPARSEABLE`. **Its KDoc at `:214-217` is false as written:**
|
|
||||||
|
|
||||||
> That exception matters: a device double that claims it can decode an unparseable file would let
|
|
||||||
> the router send a doomed job to Media3.
|
|
||||||
|
|
||||||
It would not — `:117` already caught it. This is F2's shape: a comment that describes a hazard the
|
|
||||||
code upstream has removed. Correcting it is a one-line change and should not be bundled with
|
|
||||||
anything.
|
|
||||||
- **`ContainerCapabilities:297`** — `if (container == GIF || container == IMAGE_SEQUENCE) return null`
|
|
||||||
in `repair`. `repair`'s only caller is `suggestions` (`:281`); `validate` returns at `:121` for
|
|
||||||
`isImageOutput` (which is exactly GIF ∥ IMAGE_SEQUENCE) before `suggestions` is reached, and
|
|
||||||
`firstContainerHolding` filters on `CARRIES_VIDEO`, which is empty for both.
|
|
||||||
- **`ContainerCapabilities:324` and `:340`** — the `else ->` arms themselves are exercised; what is
|
|
||||||
missed is the elvis tail, `firstOrNull() ?: VideoCodec.NONE` / `?: AudioCodec.NONE`. Reaching it
|
|
||||||
needs a container with no encodable codec on that axis. Audio-only containers return early at
|
|
||||||
`:307`, and the only containers with an empty audio set are GIF and IMAGE_SEQUENCE, excluded at
|
|
||||||
`:297` above.
|
|
||||||
|
|
||||||
**Recorded so the next read does not re-file them.** F4's rule applies unchanged: a second line of
|
|
||||||
defence that can be provoked is not a second line of defence, and widening a private function to make
|
|
||||||
one reachable buys a test that asserts a fallback fires when called in a way production cannot call
|
|
||||||
it.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## F7 — `probeWithExtractor`'s catch is unreachable for the same measured reason `probeForConcat`'s is
|
|
||||||
|
|
||||||
**Severity: n/a · No action · completes a measurement already on record**
|
|
||||||
|
|
||||||
```
|
|
||||||
app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:180-182
|
|
||||||
```
|
|
||||||
|
|
||||||
```kotlin
|
|
||||||
} catch (e: Exception) {
|
|
||||||
Log.i(TAG, "Platform extractor could not read $uri.", e)
|
|
||||||
null
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
`CLAUDE.md` records the measurement for the *other* extractor site: Robolectric's `MediaExtractor`
|
|
||||||
never throws from `setDataSource`, checked across an unregistered `content://` authority, a missing
|
|
||||||
`file://`, a file of garbage bytes and an `http://` URL — all four returned with `trackCount = 0`.
|
|
||||||
|
|
||||||
`probeWithExtractor` calls the same overload, three lines apart in the same file, and the measurement
|
|
||||||
covers it identically. It was simply not written down for this site, so a future read would re-derive
|
|
||||||
it. It stays device-only, alongside `probeForConcat`'s.
|
|
||||||
|
|
||||||
**Two neighbouring line counts are artifacts of this, not separate gaps.** `MediaProbe:184` and
|
|
||||||
`:331` each report 27 missed instructions and are the `finally` block's synthetic exception-path copy
|
|
||||||
— JaCoCo duplicates a `finally` per exit path, and the exceptional one is unreachable for the reason
|
|
||||||
above. Do not read them as a third and fourth site.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## F8 — Three more dead members, and six unused defaults
|
|
||||||
|
|
||||||
**Severity: low · Confirmed by inspection · F3's family**
|
|
||||||
|
|
||||||
```
|
|
||||||
app/src/main/java/org/libremediaconverter/model/CopyPlanner.kt:28 ConversionPlan.hasVideo
|
|
||||||
app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:39 hardwareEncoders()
|
|
||||||
app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:30 Result.output
|
|
||||||
app/src/main/java/org/libremediaconverter/convert/Transcoders.kt:28, :29, :40, :61
|
|
||||||
app/src/main/java/org/libremediaconverter/work/Reattachment.kt:28, :30
|
|
||||||
```
|
|
||||||
|
|
||||||
- **`ConversionPlan.hasVideo`** — zero callers in `main`, `test` or `androidTest`. Every `hasVideo`
|
|
||||||
hit in the tree is `InputProbe.hasVideo`, `OutputSpec.hasVideo` or `Container.extensionFor(hasVideo)`,
|
|
||||||
which are different properties on different types. A test asserting
|
|
||||||
`plan.hasVideo == (plan.video != VideoPlan.Drop)` is vacuous by construction.
|
|
||||||
- **`AndroidDeviceCodecs.hardwareEncoders()`** — its only caller is `RealMediaBenchmark`, in
|
|
||||||
`androidTest`. Production reads capabilities through `DeviceCodecs`, never the raw set.
|
|
||||||
- **`ConcatEngine.Result.output`** — `ConcatWorker` reads `result.strategy` and uses the `staged`
|
|
||||||
file it passed in, never `.output`.
|
|
||||||
- **`Transcoders.kt`'s default arguments** — `request` and `onProgress` on
|
|
||||||
`HardwareTranscoder.transcode` (`:28`, `:29`), `onProgress` on `SoftwareTranscoder.run` (`:40`),
|
|
||||||
and `format` on `ConcatJoiner.join` (`:61`). All three production call sites
|
|
||||||
(`ConversionWorker.kt:208`, `:234`, `ConcatWorker.kt:79`) pass every argument, so the synthesised
|
|
||||||
`$default` bridges and `$DefaultImpls` copies are never entered. The
|
|
||||||
`request: ConversionRequest = ConversionRequest(OutputFormat.MP4_H265.spec)` default is the one
|
|
||||||
worth a second look: nothing anywhere omits it, so an interface silently promises H.265 to a
|
|
||||||
caller that does not exist.
|
|
||||||
- **`JobSnapshot`'s `outputModifiedAt` and `tags` defaults** — `JobSnapshots.kt:32-42` passes all
|
|
||||||
seven fields, so the synthesised `$default` constructor (20 missed instructions at
|
|
||||||
`Reattachment.kt:14`) is never entered.
|
|
||||||
|
|
||||||
**Not a test gap, for F3's reason.** Delete them, or keep them and know they are unused; either is a
|
|
||||||
decision, and a test restating the compiler is not.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## F9 — Both workers' `getForegroundInfo` overrides are dead, and this is why
|
|
||||||
|
|
||||||
**Severity: n/a · No action · sharpens #88 rather than reopening it**
|
|
||||||
|
|
||||||
```
|
|
||||||
app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt:342-346
|
|
||||||
app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt:132-136
|
|
||||||
```
|
|
||||||
|
|
||||||
**#88 already closed on these**, after reading both and finding no decision worth a seam — the
|
|
||||||
correct call, and it stands. What #88 did not name is the reason they are cold in the first place,
|
|
||||||
which is stronger than "the JVM cannot reach them":
|
|
||||||
|
|
||||||
WorkManager calls `getForegroundInfoAsync()` **only for expedited work**. `ConversionWorker`'s own
|
|
||||||
KDoc says expedited is deliberately not used, and `grep -rn 'setExpedited\|OutOfQuotaPolicy' app/src`
|
|
||||||
returns nothing. So both overrides are dead in production today, not merely untested — a test would
|
|
||||||
assert the shape of something nothing invokes.
|
|
||||||
|
|
||||||
They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWorker` contract and
|
|
||||||
`setForeground` is called explicitly elsewhere. **What would reopen this** is the same trigger #88
|
|
||||||
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
|
|
||||||
`setExpedited`.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
|
|
||||||
|
|
||||||
**Severity: n/a · No action · the shape a coverage number cannot distinguish**
|
|
||||||
|
|
||||||
```
|
|
||||||
app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt:550, :553
|
|
||||||
app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt:349, :352, :278
|
|
||||||
```
|
|
||||||
|
|
||||||
F4 and F6 hold arms that cannot be *reached*. These can — and a test written against them would still
|
|
||||||
pass under the mutation that ought to redden it, which is the harder case to spot and the more
|
|
||||||
expensive one to discover halfway through writing the test.
|
|
||||||
|
|
||||||
- **`observer?.cancel()`'s non-null arm** (`ConversionViewModel:550`, `JoinViewModel:349`). Reachable
|
|
||||||
by calling `convert()` twice. But `ScreenOwnership`'s token is what actually blocks the superseded
|
|
||||||
write — the ViewModel's own KDoc at `reset()` says the cancel is "a request honoured at the next
|
|
||||||
suspension point" and "the claim is what actually stops that write". Delete `observer?.cancel()`
|
|
||||||
and the suite stays green, correctly.
|
|
||||||
- **`if (info == null) return@collect`** (`ConversionViewModel:553`, `JoinViewModel:352`). Reachable
|
|
||||||
through `pruneWork()`. But when the null arrives the state is already terminal, so removing the
|
|
||||||
guard crashes the collector and **leaves the state unchanged** — a state assertion is green under
|
|
||||||
the mutation. The only observable is an escaped coroutine exception, which the ViewModel's own KDoc
|
|
||||||
documents as unreliable on the JVM: kotlinx-coroutines-test's process-wide collector hands it to
|
|
||||||
whichever `runTest` starts next.
|
|
||||||
- **`JoinViewModel:278`'s `Ambiguous` arm.** Looks like the twin of `ReattachGuardsTest`'s "a result
|
|
||||||
two jobs both claim", and is not. An `Ambiguous` requires a shared `outputPath`, so it can only be a
|
|
||||||
*finished* job — which maps to `Joined`, a state that reads nothing from `inputs`. **The Convert-side
|
|
||||||
twin does bite**, because `displayNameOf(tags)` reaches the file card; the asymmetry is the point.
|
|
||||||
|
|
||||||
**Recorded because each of these was picked up as a candidate and put down again.** The wave-4 read
|
|
||||||
lost time to all three before the mutation test was run in the head rather than the editor, which is
|
|
||||||
the cheaper order.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## Summary
|
## Summary
|
||||||
|
|
||||||
| ID | Finding | Severity | Evidence | Action |
|
| ID | Finding | Severity | Evidence | Action |
|
||||||
@@ -449,23 +271,12 @@ the cheaper order.
|
|||||||
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
|
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
|
||||||
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
|
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
|
||||||
| F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap |
|
| F5 | `ConversionNotifications.areEnabled()` is never called | low | confirmed by inspection; grep returns the declaration only | **decide**: act on it or delete it — **not** a test gap |
|
||||||
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
|
|
||||||
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
|
|
||||||
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
|
|
||||||
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close |
|
|
||||||
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
|
|
||||||
|
|
||||||
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
|
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
|
||||||
user-visible answer — a format the app can produce and does not offer, and a warning the app
|
user-visible answer — a format the app can produce and does not offer, and a warning the app
|
||||||
documents and does not give — and either answer changes what the tidying should look like. F2, F3 and
|
documents and does not give — and either answer changes what the tidying should look like. F2 and F3
|
||||||
F8 are tidying and belong in one commit with each other, not with F1 or F5. F6's KDoc correction is a
|
are tidying and belong in one commit with each other, not with F1 or F5. F4 is finished by being
|
||||||
third kind: one line, no decision, and it should not wait on the tidying. F4, F7, F9 and F10 are
|
written down.
|
||||||
finished by being written down.
|
|
||||||
|
|
||||||
**Six of the ten are now "no action" or "not a test gap", and that is the useful shape.** By wave 4
|
|
||||||
the report's remaining red is mostly this: arms nothing can reach, members nothing calls, and arms a
|
|
||||||
test can reach but not pin. A coverage number cannot tell any of them from a real gap, which is why
|
|
||||||
this document exists and why it grows faster than the percentage moves.
|
|
||||||
|
|
||||||
**F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the
|
**F1 and F5 share a shape worth naming:** both are places where a comment describes behaviour the
|
||||||
code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would
|
code does not have, and in both the tempting fix (delete the dead arm, test the dead method) would
|
||||||
@@ -476,15 +287,7 @@ freeze the wrong answer in place. The decision comes first.
|
|||||||
**The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions
|
**The test gaps from the same read.** Seven JVM-side gaps (**#132**) and three seam questions
|
||||||
(**#133**) came out of this coverage read and are tracked there, because they are work rather than
|
(**#133**) came out of this coverage read and are tracked there, because they are work rather than
|
||||||
observations. This document holds only what a test would not fix. #133 also records why
|
observations. This document holds only what a test would not fix. #133 also records why
|
||||||
`AndroidDeviceCodecs.probe()` was considered and left out **through `ShadowMediaCodecList`**, so that
|
`AndroidDeviceCodecs.probe()` was considered and left out, so that spike is not run a third time.
|
||||||
spike is not run a third time.
|
|
||||||
|
|
||||||
**Updated 2026-09-02:** #194 proposes reaching the same code through a *pure seam* instead, which is a
|
|
||||||
different mechanism and one #133 did not evaluate — the builder objection it turns on (no
|
|
||||||
`setIsAlias`, no `setCanonicalName`) does not apply to a function taking its own entry type. #133's
|
|
||||||
close stands for the shadow; it is not a close on the seam. #194 also carries the reason the seam is
|
|
||||||
worth cutting at all, which is not coverage: the `runCatching` fallback logs "assuming permissive" and
|
|
||||||
returns empty sets, which makes `canEncode` and `canDecode` answer *no* for everything.
|
|
||||||
|
|
||||||
**`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not.
|
**`ConversionForegroundType.current()`**, which looked like the sharpest gap in the read and is not.
|
||||||
Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is
|
Its API 33 and 34 arms are cold on the JVM, but issue **#88** already established that the class is
|
||||||
@@ -518,25 +321,6 @@ the real ones — **34 of 383** and **20 of 143** missed — and the screens are
|
|||||||
better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the
|
better-covered files in the repo, which is what #52, #57 and #61 were for. **Do not chase the
|
||||||
branch number here.** If a future read wants a screen metric, use lines.
|
branch number here.** If a future read wants a screen metric, use lines.
|
||||||
|
|
||||||
**Updated 2026-09-02: the same codegen inflates the *instruction* count, which wave 3's filter did
|
|
||||||
not allow for.** Wave 3 selected candidates on `mi > 0` — at least one missed instruction — which was
|
|
||||||
right to prefer over a bare branch count and is still wrong on these files. `JoinScreen.kt:222` reads
|
|
||||||
`mi=10` and looks uncovered; it also reads `ci=38`, and `JoinStateAffordancesTest` already clicks that
|
|
||||||
Save button and asserts `save:joined.mp4`. Every `onClick` lambda body flagged this way turned out to
|
|
||||||
be covered at method level, the missed instructions being the recomposition-skip path again.
|
|
||||||
|
|
||||||
Use `ci == 0` — the line never executed, which is JaCoCo's own missed-line definition — and pair it
|
|
||||||
with a method-level `ci > 0 && mb > 0` pass for covered methods with cold arms. Neither filter alone
|
|
||||||
is enough: `ConversionViewModel.cancel()` misses no line at all, yet its non-null arm had never been
|
|
||||||
entered in 584 tests (#192). `CLAUDE.md`'s coverage entry carries the same correction.
|
|
||||||
|
|
||||||
**Also codegen, also not gaps**, recorded once so they are not re-derived: the synthetic
|
|
||||||
`NoWhenBranchMatchedException` closing an exhaustive `when` (`ConverterScreen:399`, `:686`,
|
|
||||||
`JoinScreen:278`, `MainActivity:160`); the inner `is Idle -> Unit` arms at `ConverterScreen:253-254`
|
|
||||||
and `JoinScreen:158-159`, which are structurally unreachable because the outer `when` already routed
|
|
||||||
`Idle`; and the closing brace of a `launch` block whose `collect` never terminates
|
|
||||||
(`ConversionViewModel:578`, `JoinViewModel:371`).
|
|
||||||
|
|
||||||
**Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`)
|
**Anything requiring a device.** `MediaProbe`'s FFprobe half (`MediaProbe.kt:151, 156-158, 173-188`)
|
||||||
and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures
|
and `FFmpegEngine` in full report 0% on the JVM and are covered by `androidTest`. JaCoCo measures
|
||||||
`testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded
|
`testDebugUnitTest` only; their zeroes are a boundary, as #84, #85, #86 and #88 each recorded
|
||||||
|
|||||||
@@ -1,327 +0,0 @@
|
|||||||
# 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,18 +312,6 @@ on sample media that is deliberately not committed. Its third test,
|
|||||||
`reportDeviceEncoderCapabilities`, has no such guard and runs. A level reporting 0 skipped
|
`reportDeviceEncoderCapabilities`, has no such guard and runs. A level reporting 0 skipped
|
||||||
would mean someone had staged sample files, not that something improved.
|
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
|
### What the sweep adds, and what it does not
|
||||||
|
|
||||||
**The renderer rule held four more times.** No boot log contains the string
|
**The renderer rule held four more times.** No boot log contains the string
|
||||||
|
|||||||
Reference in New Issue
Block a user