From 95902a788969b515c322fe62fe6ada1820721384 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 09:36:36 -0500 Subject: [PATCH 1/3] Delete an assertion that could never fail, and say what guards instead ConversionViewModelProbeFailureTest's pickedProbe() helper held: val ready = awaitState(viewModel.state, "Ready with a probe") { it is ConversionState.Ready && it.input.probe != null } assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed)) The predicate requires `Ready`. `Ready` and `Failed` are sibling subtypes of one sealed interface, so `ready as? Failed` is always null and the assertNull could never fire. R26 filed this PLAUSIBLE on types read; it is measured now. Flipping the line to assertNotNull failed 3 of the 4 tests in the class -- three, because pickedProbe() has three callers, which is also why a dead line here was worth removing rather than shrugging at: it read as coverage in a helper the whole class depends on. Deleted rather than replaced. There is nothing for a live assertion to add: a pick that ended in Failed never satisfies the predicate, so awaitState fails on its timeout naming what it was waiting for -- "Ready with a probe" -- which is a better failure message than the assertion would have produced. The comment now says that, so the next reader does not re-add the guard the predicate already is. This is the ninth vacuous assertion this line of work has turned up, and the pattern is consistent: they hide in helpers, they pass, and they look like care. The suite is green before and after, which is exactly the point -- deleting a dead assertion cannot change a result, and if it had, the line was not dead. Closes #35. --- .../convert/ConversionViewModelProbeFailureTest.kt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt index 9bfe5b9..2aa69ac 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -140,10 +140,16 @@ class ConversionViewModelProbeFailureTest { private fun pickedProbe(): InputProbe? { val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) viewModel.onInputPicked(INPUT) + // The predicate is the guard, and it is the only one needed. It requires `Ready`, so a + // pick that ended in `Failed` never satisfies it and `awaitState` fails on its timeout + // naming what it was waiting for -- "Ready with a probe" -- which says more than a + // separate assertion could. A `ready as? ConversionState.Failed` check used to sit here + // and was dead: `Ready` and `Failed` are sibling subtypes of one sealed interface, so + // the cast was always null and the assertNull could never fire. Measured, not assumed -- + // flipping it to assertNotNull failed all three callers of this helper. val ready = awaitState(viewModel.state, "Ready with a probe") { it is ConversionState.Ready && it.input.probe != null } - assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed)) return (ready as ConversionState.Ready).input.probe } From d37c391c60fd54e3c740261dd77b76a4625f8a91 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 09:45:00 -0500 Subject: [PATCH 2/3] Stop telling people to stage the benchmark the one way it cannot be staged RealMediaBenchmark's class KDoc said: Populate with: adb push .mp4 /sdcard/Android/data/org.libremediaconverter/files/ Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says: Internal storage, not the external files dir. Files placed in the external dir by `adb push` or `adb shell cp` stay owned by the shell user, and the app then gets EACCES trying to read them -- which presents as an unparseable input rather than a permission problem. Different directories, and the second exists specifically to explain why the first fails. Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked into by the instruction in the same file. The fix is not a corrected command. Restating the mechanism in a second place is what let these drift, and a replacement command I have not executed would be the same defect with a fresher date. The class KDoc now names [samples] as the single place that answers it. Two things added that are checkable rather than remembered: the exact filenames the tests look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `.mp4`, so even the right directory left you guessing -- and a note that the two skips every green E2E leg reports are these. Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so; it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the guards work, and "harmless in CI" is accurate. The failure that prompted the look is Media3EngineTest, tracked as #102. Closes #101. --- .../bench/RealMediaBenchmark.kt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt b/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt index 3787388..d5f97ea 100644 --- a/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt +++ b/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt @@ -36,8 +36,20 @@ import java.io.File * 1. that the hardware path is worth having a second engine for at all, and * 2. that x264's CRF is worth the GPL licence the app carries for it. * - * Skips itself when the sample files are absent, so it is harmless in CI. Populate with: - * adb push .mp4 /sdcard/Android/data/org.libremediaconverter/files/ + * Skips itself when the sample files are absent, so it is harmless in CI — every green E2E + * leg reports two skips, and these are they. + * + * The two files it looks for, by exact name: + * + * - [H264_SAMPLE] for [hardwareVersusSoftwareOnRealVideo] + * - [AV1_SAMPLE] for [av1InputRoutesAccordingToDeviceDecodeSupport] + * + * **Where they go, and how, is on [samples] — read it before staging anything.** This used to + * carry an `adb push` line naming the external files dir, which [samples] then explains cannot + * work: a pushed file stays owned by the shell user and the app reads EACCES, surfacing as an + * unparseable input rather than a permission error. The instruction and its own refutation sat + * twelve lines apart. It is named in one place now rather than restated here, because restating + * it is what let the two drift. */ @UnstableApi @RunWith(AndroidJUnit4::class) From 1b220856ab1d4c148060fecd1c9a5aafe93b5896 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 10:14:34 -0500 Subject: [PATCH 3/3] Say 37.0 is the choice, not the only api-level that exists R19 raised two things about this comment. One resolved itself: it used to explain why the matrix had no API 37 row at all, and #56 added the gating row, so that half is gone. The other survived, and this is it. The comment read api-level must be "37.0". A bare 37 is not an SDK package and fails during setup The second sentence is true and was measured -- it cost a run to find. The first overstates it. What must be true is that the api-level is a POINT release; 37.0 is one of several. api37-debug.yml's own input descriptions already say so: API level, as the SDK spells it. 37.0, 37.1, 37.2-beta3, 36 ... System image target. android-37.1 and 37.2-beta* ship ONLY as google_apis_ps16k and docs/api-37-emulator-crash.md measures android-37.0 rev 6 and android-37.1 rev 8 side by side, both aborting. So the repo already knows 37.1 exists and behaves the same; only this comment implied otherwise. That matters for the reader it is written for. Someone debugging this row and wondering whether a newer image helps reads "must be 37.0" as a constraint and stops. The measured answer is that it does not help, which is a better thing to learn than a rule that is not one -- and the ps16k-only wrinkle above 37.0 is the detail that would actually bite them. Comment only. No job, matrix, filter or gating behaviour changes. actionlint clean at the pinned digest. Closes #28. --- .github/workflows/status_check.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index f3b3355..7b890fc 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -280,8 +280,13 @@ jobs: # docs/api-37-emulator-crash.md has the per-method measurements, and the # correction that produced them. # - # api-level must be "37.0". A bare 37 is not an SDK package and fails - # during setup, which cost a run to discover. + # api-level must be a POINT release. A bare 37 is not an SDK package and + # fails during setup, which cost a run to discover. `37.0` is the choice + # here rather than the only option: `37.1` and `37.2-beta*` exist and + # abort the same way, and api37-debug.yml's inputs document both, with + # the wrinkle that above 37.0 they ship only as google_apis_ps16k. + # docs/api-37-emulator-crash.md measures 37.0 rev 6 and 37.1 rev 8 side + # by side, so pinning 37.0 is a decision, not a constraint. # # notAnnotation removes the three tests that do not pass on this image; they # run in the advisory job below, off the same marker so they cannot end up