Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
7d1d3191a9 | ||
|
|
68b863fbdb | ||
|
|
f4174e5b06 | ||
|
|
dd01f9f27c | ||
|
|
dd76229e90 | ||
|
|
8105291f6a | ||
|
|
68bd24e54a | ||
|
|
19e35394e1 | ||
|
|
cbbaf74285 | ||
|
|
fac8e67db2 | ||
|
|
4de169c99b | ||
|
|
e27d7601b1 | ||
|
|
e81403c5f3 | ||
|
|
54167c052f | ||
|
|
1f21557b8d | ||
|
|
3b0c262030 | ||
|
|
18aff51c98 |
@@ -272,13 +272,15 @@ jobs:
|
||||
# has to begin after them, not between them. The name is stale and kept:
|
||||
# read .github/scripts/e2e-run.sh's header, which carries the measurements.
|
||||
#
|
||||
# notAnnotation below keeps five tests off this row, and one of
|
||||
# them is new. SafPickerRoundTripTest's PICKER test was measured on
|
||||
# 2026-08-24 as passing here and was left on the leg; four gating logcats
|
||||
# read on 2026-09-05 show it aborting system_server from the task-snapshot
|
||||
# path on every single run, pass or fail, which is what had been failing
|
||||
# unrelated PRs (#108). Both of that class's tests now carry the marker.
|
||||
# docs/api-37-emulator-crash.md has the timings and the correction.
|
||||
# notAnnotation below keeps seven tests off this row. SafPickerRoundTripTest's
|
||||
# PICKER test was measured on 2026-08-24 as passing here and was left on the
|
||||
# leg; four gating logcats read on 2026-09-05 show it aborting system_server
|
||||
# from the task-snapshot path on every single run, pass or fail, which is what
|
||||
# had been failing unrelated PRs (#108). All FOUR of that class's tests now
|
||||
# carry the marker -- the two saves through the picker (#226, #250) joined on
|
||||
# 2026-09-06 by inheritance rather than measurement, since they open the same picker.
|
||||
# docs/api-37-emulator-crash.md has the timings and the correction, and
|
||||
# FailsOnEmulatorApi37.kt has why the third one cannot be measured here.
|
||||
#
|
||||
# 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
|
||||
@@ -288,9 +290,12 @@ jobs:
|
||||
# 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
|
||||
# notAnnotation removes the seven tests that cannot be RUN on this image; they
|
||||
# run in the advisory job below, off the same marker so they cannot end up
|
||||
# in both or neither. docs/api-37-emulator-crash.md has the measurements.
|
||||
# in both or neither. "Cannot be run" rather than "do not pass" is deliberate:
|
||||
# four fail outright, one of those aborts the framework on its way down, and on
|
||||
# the advisory leg the three picker tests behind it never report at all.
|
||||
# docs/api-37-emulator-crash.md has the measurements.
|
||||
- label: "37"
|
||||
api-level: "37.0"
|
||||
disable-system-ui: "1"
|
||||
|
||||
@@ -76,12 +76,21 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
|
||||
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
|
||||
table.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Five** of the 69 instrumented
|
||||
tests cannot be *run* on that image, for three unrelated reasons: three Media3 tests fail inside
|
||||
the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down when it
|
||||
rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` from
|
||||
the task-snapshot path whether it passes or not. All five carry `@FailsOnEmulatorApi37` and run
|
||||
in a separate `continue-on-error` job; the gating leg runs the other 64.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Seven** of the 71 instrumented
|
||||
tests cannot be *run* on that image, for three measured reasons and two inherited: three Media3
|
||||
tests fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework
|
||||
down when it rotates the display, and its sibling — the SAF picker round trip — aborts
|
||||
`system_server` from the task-snapshot path whether it passes or not. The sixth, that class's
|
||||
two saves through the picker (#226 and #250), carry the marker because they open the same picker
|
||||
and a second DocumentsUI dialog on top of it — **not** because either has ever been observed here. It cannot be:
|
||||
the rotation test runs first and takes the framework down, so all five advisory runs at the
|
||||
previous baseline reported `expected: 6, received: 4, failed: 4`, and the four were the three
|
||||
Media3 tests plus the rotation — runs 34041156680, 34041593697, 34042397320, 34043502322 and
|
||||
34045105857. **No picker test has ever reported on the advisory leg**, which is a correction to
|
||||
what the marker's own KDoc used to say. All seven carry `@FailsOnEmulatorApi37` and run in a
|
||||
separate `continue-on-error` job; the gating leg runs the other 64 — **the same 64 for the third
|
||||
time running**, which is exactly how this paragraph goes stale unnoticed: 69−5, 70−6 and 71−7
|
||||
are all 64.
|
||||
|
||||
**These two numbers move with the suite and are derived, not remembered.** `grep -cE
|
||||
'^\s*@Test' ` over `app/src/androidTest` is the first; the second is that minus the marker
|
||||
@@ -114,7 +123,7 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
describes everything in it. The name is kept deliberately — it is not a required context and
|
||||
people have learned to look for it — so **read the marker, not the name**, for what it holds.
|
||||
**It is red on every PR, by design**: do not read it as your change breaking something, and do
|
||||
not read a green run as evidence those five tests pass.
|
||||
not read a green run as evidence those seven tests pass.
|
||||
`docs/api-37-emulator-crash.md` has the measurements.
|
||||
|
||||
**That instruction is also why nobody looks, so the job now reports its own shape** — expected,
|
||||
@@ -134,7 +143,7 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
is gradle never returning, so the log it left says nothing about it.
|
||||
|
||||
Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on
|
||||
the Pixel 10 Pro XL before each release.** Those five tests are the one thing CI cannot answer
|
||||
the Pixel 10 Pro XL before each release.** Those seven tests are the one thing CI cannot answer
|
||||
for.
|
||||
|
||||
On a device or emulator, build only the ABI it can execute:
|
||||
@@ -339,7 +348,7 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
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
|
||||
first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E7**, 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.
|
||||
|
||||
@@ -378,6 +387,55 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
|
||||
and is how #238 surfaced.
|
||||
|
||||
**The 2026-09-06 re-check found that the read's own last PR had re-introduced the drift the read
|
||||
was about**, and that is the entry worth carrying forward. #226 moved the suite 69 -> 70 and the
|
||||
markers 5 -> 6 and changed neither the count in this file, the marker's KDoc, nor the two
|
||||
comments in `status_check.yml`. **The gating figure is what hid it**: 69 - 5 and 70 - 6 are both
|
||||
64, so the one number a reader checks against a run had not moved — which is precisely why the
|
||||
paragraph above says to derive these rather than remember them. Worse, two KDoc claims in the new
|
||||
test described a draft rather than the code: it says MP3 was chosen so the setup could not depend
|
||||
on the device's codecs, while the code converts at the default `MP4_H265`/`FAST` and therefore
|
||||
routes on `canEncode(H265)` — the *negation* of the stated reason. **That is E1 and E3's failure
|
||||
mode, committed by the wave that found it.** All of it is fixed; the standing item is **#250**,
|
||||
because #226 proved D4's premise and never drove its delete arm.
|
||||
|
||||
- **Nothing is committed or pushed until the local gate is green, at every supported API level.**
|
||||
Source work (`app/src/main`) needs the unit tests **and** the instrumented tests passing on every
|
||||
level; test work (`app/src/test`, `app/src/androidTest`) needs the whole suite passing on every
|
||||
level. `tools/git-hooks/local-gate.sh` enforces it as `pre-commit` and `pre-push`; wire it up once
|
||||
with `git config core.hooksPath tools/git-hooks`.
|
||||
|
||||
33-36 run the whole suite on emulators. **API 37 cannot be run on an emulator on this host at
|
||||
all** — not "is red", *cannot run*: measured 2026-09-06, the image logs `3 new surfaceflinger
|
||||
aborts in 45 s (want 0)` and then the APK install itself fails with `Can't find service:
|
||||
package`, because the framework is gone before Gradle installs anything. `Starting 0 tests`. So
|
||||
the hook runs API 37 on the **attached Pixel 10 Pro XL** when it is there, and says plainly that
|
||||
the level is uncovered when it is not — CI's gating leg being what answers for it then. It never
|
||||
claims five levels having run four.
|
||||
|
||||
**It runs shellcheck and actionlint too, at CI's exact pins** — shellcheck over
|
||||
`git ls-files '*.sh'`, actionlint over the workflows, the same digests and the same file sets
|
||||
that leg uses. actionlint is not an afterthought to shellcheck but the other half of the same
|
||||
hole: much of this repo's bash lives in workflow `run:` blocks, which `'*.sh'` does not match at
|
||||
all. That gap was found the hard way: the gate checked ktlint,
|
||||
detekt and Android lint, so a new `.sh` file was precisely the case where it passed and CI still
|
||||
went red, and the first file it could not check was itself. **The digest is read out of
|
||||
`status_check.yml` rather than copied** — two copies drift, and the symptom of that drift is the
|
||||
gate passing while CI fails, which is the one thing this check exists to prevent.
|
||||
|
||||
The sweep is cached under the hash of the **`app/src` subtree**, not the whole repo tree. Keying
|
||||
it on the whole tree was the first cut and it was wrong: editing a comment in `CLAUDE.md` threw
|
||||
away a sweep of byte-identical application code and re-ran forty minutes of emulators to prove
|
||||
nothing, which is how a gate teaches people to resent it. Any change under `app/src` still
|
||||
invalidates it, and the JVM gate runs unconditionally. **There is deliberately no skip
|
||||
variable**, and `--no-verify` needs the repo owner's say-so each time rather than being reached
|
||||
for when the gate is inconvenient.
|
||||
|
||||
Why it is worth tens of minutes a commit: the alternative was measured on 2026-09-06, when one PR
|
||||
spent several gating legs learning one leg at a time what a sweep answers in one pass — and the
|
||||
failing leg **moved** between runs (API 35 red then green, API 34 green then red). One leg at a
|
||||
time that reads as someone else's flake; as a sweep it is one signal.
|
||||
|
||||
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
|
||||
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
|
||||
a change that is both needs both.
|
||||
|
||||
@@ -9,14 +9,23 @@ package org.libremediaconverter
|
||||
* That is the whole reason there is one annotation rather than a pair of test lists: two lists
|
||||
* drift, and the drift is silent in both directions (a test that runs nowhere reads as green).
|
||||
*
|
||||
* **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Four of the
|
||||
* five carriers simply fail: three Media3 tests die in the image's own `c2.goldfish.h264.decoder`,
|
||||
* and the SAF rotation test takes the framework down with it. The fifth —
|
||||
* `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` — **passes about
|
||||
* half the time and aborts `system_server` every time**, which is worse for a gating leg than an
|
||||
* honest failure: it fails the leg from the teardown, with no failing test to point at (#108).
|
||||
* The wording was widened rather than the test excused; that test's own KDoc has the four-run
|
||||
* measurement.
|
||||
* **"Cannot be run" covers three things now, and it covered only the first until 2026-09-05.**
|
||||
* Four of the seven carriers simply fail: three Media3 tests die in the image's own
|
||||
* `c2.goldfish.h264.decoder`, and the SAF rotation test takes the framework down with it. The
|
||||
* fifth — `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` —
|
||||
* **passes about half the time and aborts `system_server` every time**, which is worse for a
|
||||
* gating leg than an honest failure: it fails the leg from the teardown, with no failing test to
|
||||
* point at (#108). The wording was widened rather than the test excused; that test's own KDoc has
|
||||
* the four-run measurement.
|
||||
*
|
||||
* **The sixth and seventh are the new third thing: they are marked by inheritance, not by
|
||||
* measurement.** `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226)
|
||||
* and `.aFailedSaveDeletesTheDocumentItCouldNotWrite` (#250) each open the same picker and then a
|
||||
* second DocumentsUI dialog on top of it, so they sit on the same task-snapshot path their sibling
|
||||
* was marked for. Neither has ever been observed at API 37 either way — see the measurement under
|
||||
* [FAILS_ON_EMULATOR_API37_BASELINE], which is why they cannot be. Marking them was the
|
||||
* conservative choice, and **the trigger for revisiting it is the rotation test, not themselves**:
|
||||
* while that one truncates the advisory run, nothing downstream of it can report.
|
||||
*
|
||||
* It says only what has been measured: **on the emulator, at API 37.** The same tests pass on a
|
||||
* physical Pixel 10 Pro XL at API 37 and at API 33–36 on the same runner under the same renderer,
|
||||
@@ -46,19 +55,21 @@ annotation class FailsOnEmulatorApi37
|
||||
* keep printing with nothing to compare to, so it announces that it could not read the baseline
|
||||
* rather than falling quiet. If you see that notice, this line is what it means.
|
||||
*
|
||||
* **One number, both checks, and that is what the marker means.** A test carrying it cannot be run
|
||||
* on this image, so the count is simultaneously how many the advisory leg runs and how many fail.
|
||||
* A *smaller* failure count is the interesting direction: it means one of them now passes, which
|
||||
* is the trigger the KDoc above names for deleting the annotation.
|
||||
* **One number, both checks, and that is what the marker was meant to mean.** A test carrying it
|
||||
* cannot be run on this image, so the count is meant to be simultaneously how many the advisory
|
||||
* leg runs and how many fail. A *smaller* failure count is the interesting direction: it means one
|
||||
* of them now passes, which is the trigger the KDoc above names for deleting the annotation.
|
||||
* **Since 2026-09-06 the second half no longer holds in practice** — the run truncates before
|
||||
* three of the seven start, which the last paragraph below measures. `expected` still holds, and it is the
|
||||
* field that catches a marker added without changing this number.
|
||||
*
|
||||
* **The picker test is the one to read that sentence carefully for.**
|
||||
* `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting
|
||||
* `system_server` rather than for failing (#108), and on the gating leg it passed two runs of
|
||||
* four. It fails on the advisory leg because the rotation test runs before it and takes the
|
||||
* framework down first — measured, `api37-debug.yml` run 34008889182, which reports
|
||||
* `expected: 4, received: 4, failed: 4` with the four in the order Media3, Media3, rotation,
|
||||
* picker. (Those dispatches predate the third Media3 marker landing on `main`, so their totals
|
||||
* are four rather than five; the ordering they establish is what matters here.)
|
||||
* **The picker tests are the ones to read that sentence carefully for, and the reason changed
|
||||
* on 2026-09-06.** `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on
|
||||
* 2026-09-05 for aborting `system_server` rather than for failing (#108), and on the gating leg
|
||||
* it passed two runs of four. It was recorded here as *failing* on the advisory leg, behind the
|
||||
* rotation test — measured, `api37-debug.yml` run 34008889182, `expected: 4, received: 4,
|
||||
* failed: 4`, in the order Media3, Media3, rotation, picker. (Those dispatches predate the third
|
||||
* Media3 marker, so their totals are four rather than six.)
|
||||
*
|
||||
* **But a second dispatch of the identical configuration reported 4/3/3**, having lost the last
|
||||
* test to the abort rather than to anything about the test list, and that is why
|
||||
@@ -68,6 +79,15 @@ annotation class FailsOnEmulatorApi37
|
||||
* reporting fewer failures than this as one of them now passing; read a truncated one as the
|
||||
* framework having died, which is this job's normal.
|
||||
*
|
||||
* **That is no longer what happens, and the difference is that neither picker test reports at
|
||||
* all.** The rotation test truncates the run before them: **all five** advisory runs at the
|
||||
* previous baseline of six — 34041156680, 34041593697, 34042397320, 34043502322 and 34045105857 —
|
||||
* report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
|
||||
* rotation. #250 adds a third picker test behind the same wall, so expect `expected: 7,
|
||||
* received: 4`. So the advisory leg currently answers for
|
||||
* four of its six, and the comparison below is unaffected only because `failed` is not compared
|
||||
* on a truncated run. Read it as **unmeasured**, not as passing or failing.
|
||||
*
|
||||
* So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in
|
||||
* the same diff. The report says so on the run itself if you forget — it prints the tree's own
|
||||
* `grep` count beside this one.
|
||||
@@ -78,4 +98,4 @@ annotation class FailsOnEmulatorApi37
|
||||
* `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report
|
||||
* records the truncation next to the counts for that reason.
|
||||
*/
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = 6
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = 7
|
||||
|
||||
@@ -14,8 +14,6 @@ import java.io.FileOutputStream;
|
||||
import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.io.OutputStream;
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
|
||||
/**
|
||||
* One file, offered to the system file picker, so that picking one can be tested at all.
|
||||
@@ -116,7 +114,6 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
|
||||
public static final String DESTINATION_PREFIX = "dest/";
|
||||
|
||||
/** Document ids {@link #deleteDocument} was called with, newest last. Cleared by {@link #reset}. */
|
||||
private static final List<String> DELETED = new ArrayList<>();
|
||||
|
||||
/** Already in this source set, and already a real H.264 MP4 the engines can open. */
|
||||
private static final String FIXTURE_ASSET = "sample_h264.mp4";
|
||||
@@ -239,24 +236,11 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
|
||||
if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) {
|
||||
throw new FileNotFoundException("refusing to delete: " + documentId);
|
||||
}
|
||||
synchronized (DELETED) {
|
||||
DELETED.add(documentId);
|
||||
}
|
||||
destinationFile(documentId).delete();
|
||||
}
|
||||
|
||||
/** Document ids {@link #deleteDocument} was called with, newest last. */
|
||||
public static List<String> deletedDocumentIds() {
|
||||
synchronized (DELETED) {
|
||||
return new ArrayList<>(DELETED);
|
||||
}
|
||||
}
|
||||
|
||||
/** Forgets recorded deletes and removes created destinations. The process outlives one class. */
|
||||
/** Removes created destinations. The process outlives one class. */
|
||||
public static void reset(File filesDir) {
|
||||
synchronized (DELETED) {
|
||||
DELETED.clear();
|
||||
}
|
||||
File dir = new File(filesDir, "destinations");
|
||||
File[] children = dir.listFiles();
|
||||
if (children != null) {
|
||||
|
||||
@@ -25,9 +25,11 @@ import androidx.test.uiautomator.Configurator
|
||||
import androidx.test.uiautomator.StaleObjectException
|
||||
import androidx.test.uiautomator.UiDevice
|
||||
import androidx.test.uiautomator.Until
|
||||
import androidx.work.WorkManager
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertArrayEquals
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNotEquals
|
||||
import org.junit.Assert.assertNotNull
|
||||
import org.junit.Assert.assertTrue
|
||||
@@ -40,6 +42,7 @@ import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.OutputPublisher
|
||||
import org.libremediaconverter.ui.TestTags
|
||||
import java.io.File
|
||||
import java.io.OutputStream
|
||||
import java.util.concurrent.atomic.AtomicInteger
|
||||
import java.util.regex.Pattern
|
||||
|
||||
@@ -280,17 +283,41 @@ private class RecordingPublisher(private val app: Context) : OutputPublisher(app
|
||||
super.publish(staged, destination)
|
||||
}
|
||||
|
||||
/**
|
||||
* Refuses the write when [failOpen] is set, which is the forcing condition for #250.
|
||||
*
|
||||
* Returning null rather than throwing is deliberate: it is the arm `publish`'s
|
||||
* `?: error("Could not open destination for writing")` exists for, and `openDestination`'s
|
||||
* own KDoc says a provider that is present and declines is the half no fake can produce on
|
||||
* demand. The size probe in `publish` has already run by the time this is reached, so
|
||||
* `destinationWasEmpty` is true and `deletePartialOutput` is reached with the document
|
||||
* genuinely empty — which is the whole point.
|
||||
*/
|
||||
override fun openDestination(destination: Uri): OutputStream? =
|
||||
if (failOpen) null else super.openDestination(destination)
|
||||
|
||||
companion object {
|
||||
var savedBytes: ByteArray = ByteArray(0)
|
||||
var seenDestination: Uri? = null
|
||||
var seenIsDocumentUri: Boolean? = null
|
||||
var seenSizeBefore: Long? = null
|
||||
|
||||
/**
|
||||
* Makes the next `publish` refuse to open its destination.
|
||||
*
|
||||
* A flag rather than a second publisher because `ConversionDependencies.publisher` is one
|
||||
* seam and there is no orchestrator: every test in this process shares the instance the
|
||||
* `init` block installed. [reset] clears it in teardown, so a test that sets it cannot
|
||||
* leak a refusing publisher into the next class.
|
||||
*/
|
||||
var failOpen: Boolean = false
|
||||
|
||||
fun reset() {
|
||||
savedBytes = ByteArray(0)
|
||||
seenDestination = null
|
||||
seenIsDocumentUri = null
|
||||
seenSizeBefore = null
|
||||
failOpen = false
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -367,6 +394,8 @@ class SafPickerRoundTripTest {
|
||||
fun restoreOrientation() {
|
||||
// The suite runs without Android Test Orchestrator, so a swapped seam outlives the class.
|
||||
ConversionDependencies.reset()
|
||||
RecordingPublisher.reset()
|
||||
clearFinishedWork()
|
||||
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
|
||||
if (!rotated) return
|
||||
device.setOrientationNatural()
|
||||
@@ -515,9 +544,28 @@ class SafPickerRoundTripTest {
|
||||
*
|
||||
* ## The conversion is setup, not subject
|
||||
*
|
||||
* Save is only offered on `Converted`, so the test converts first. MP3 is chosen because the
|
||||
* router sends it to FFmpeg unconditionally at every API level, so the setup cannot depend on
|
||||
* the device's codecs — #223 is what that costs.
|
||||
* Save is only offered on `Converted`, so the test converts first, at the screen's default
|
||||
* `MP4_H265` / `FAST`. That is **not** codec-independent, and this KDoc claimed the opposite
|
||||
* until 2026-09-06: an earlier draft used MP3 for exactly that reason, and the format had to
|
||||
* move for a different constraint the picker imposes — [convertToTheDefaultFormat] has it.
|
||||
* `MP4_H265` at `FAST` reaches `ConversionRouter`'s `canEncode(H265)` gate, so it runs on
|
||||
* FFmpeg on the emulators (no hardware H265) and on Media3 on the Pixel.
|
||||
*
|
||||
* **That is tolerable here, and #223 is the reason it needs saying.** There, the routing
|
||||
* decided whether the *subject* was reached, so a route to FFmpeg made the test pass while
|
||||
* proving nothing. Here the conversion is setup: if it goes the other way and fails, this test
|
||||
* fails loudly on the setup rather than quietly on the assertion. The subject is what `publish`
|
||||
* was handed, which the engine that produced the file does not touch.
|
||||
*
|
||||
* ## Why it carries [FailsOnEmulatorApi37]
|
||||
*
|
||||
* By inheritance, not measurement. It opens the same picker as
|
||||
* [pickingAFileThroughTheSystemPickerFillsInTheFileCard], which was marked for aborting
|
||||
* `system_server` from the task-snapshot path (#108), and then a second DocumentsUI dialog on
|
||||
* top of it. It has never been observed at API 37 either way: the rotation test truncates the
|
||||
* advisory run first, so all four advisory runs at this baseline report
|
||||
* `expected: 6, received: 4` without reaching either picker test. Marking it was the conservative choice and it is
|
||||
* recorded as unmeasured in `FailsOnEmulatorApi37.kt` rather than dressed up as a measurement.
|
||||
*/
|
||||
@Test
|
||||
@FailsOnEmulatorApi37
|
||||
@@ -551,6 +599,116 @@ class SafPickerRoundTripTest {
|
||||
assertTrue("staging should be empty after a successful save", staged.listFiles().isNullOrEmpty())
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of D4 (#250): a save that fails deletes the document it could not write.
|
||||
*
|
||||
* ## Why this is separate from the test above
|
||||
*
|
||||
* #226 proved the *premise* — SAF hands back a document reporting exactly zero bytes, so
|
||||
* `destinationIsKnownEmpty` can answer true — and then drove the success path, where the
|
||||
* `catch` is never entered. So `deletePartialOutput` had still never run against a real
|
||||
* `DocumentsProvider`; its only assertions were `OutputPublisherPublishTest`'s, against
|
||||
* `FakeSafProvider` under Robolectric. That is the same "asserted only against a fake built to
|
||||
* match it" shape #226 was filed to break, one layer down.
|
||||
*
|
||||
* ## The forcing condition, and why it is a returned null
|
||||
*
|
||||
* [RecordingPublisher.failOpen] makes `openDestination` return null. `publish` turns that into
|
||||
* `error("Could not open destination for writing")` **after** its size probe has already run,
|
||||
* so the `catch` is reached with `destinationWasEmpty == true` on a document DocumentsUI
|
||||
* created seconds earlier. Nothing is simulated: the URI, the grant, the provider and the
|
||||
* delete are all real.
|
||||
*
|
||||
* Null rather than a throw because `openDestination`'s KDoc says a provider that is present
|
||||
* and declines is the half no fake can produce on demand — so this is also the first time that
|
||||
* arm has been taken against a live provider rather than a stub.
|
||||
*
|
||||
* ## The oracle, and why it is not a recorder inside the provider
|
||||
*
|
||||
* The obvious assertion — have the provider record what `deleteDocument` was called with, and
|
||||
* read it back — **cannot work here, and finding that out is half of what this test cost.**
|
||||
* `FixtureDocumentsProvider` is declared by the test APK and runs in
|
||||
* `org.libremediaconverter.test`; instrumentation runs in the app's process. A `static` in the
|
||||
* provider is therefore a different object from the one a test can see, and the accessor #226
|
||||
* left behind read empty on every run. That is E7's process wall from a third side, after
|
||||
* `ACTION_OPEN_DOCUMENT` and `ActivityScenario`.
|
||||
*
|
||||
* So the oracle is the document, which does cross the boundary because the app holds a URI
|
||||
* grant for it. **This is still the path rather than the artefact**, because the two
|
||||
* assertions are read together: the size query above proves the document *existed and was
|
||||
* empty* moments earlier, and a `content://` document that no longer answers a query is one
|
||||
* something deleted. Nothing else in the app deletes SAF documents.
|
||||
*
|
||||
* The staged file is asserted to **survive**, which is the deliberate other half of that
|
||||
* `catch`: a failed save may leave the staged copy as the only copy of an hour of transcoding,
|
||||
* so `ConversionViewModel` keeps it and puts "Try saving again" on screen.
|
||||
*/
|
||||
@Test
|
||||
@FailsOnEmulatorApi37
|
||||
fun aFailedSaveDeletesTheDocumentItCouldNotWrite() {
|
||||
pickTheFixture()
|
||||
convertToTheDefaultFormat()
|
||||
|
||||
RecordingPublisher.failOpen = true
|
||||
saveThroughTheSystemPicker(settlesOn = TestTags.RETRY_SAVE)
|
||||
|
||||
val destination = RecordingPublisher.seenDestination
|
||||
assertNotNull("publish was never reached, so the delete arm was not exercised", destination)
|
||||
assertEquals(
|
||||
"the document was not positively empty, so publish would refuse to delete it",
|
||||
0L,
|
||||
RecordingPublisher.seenSizeBefore,
|
||||
)
|
||||
assertFalse(
|
||||
"publish did not delete the document it could not write: $destination",
|
||||
documentStillExists(destination!!),
|
||||
)
|
||||
|
||||
// The staged copy is kept on purpose -- see ConversionViewModel.save's onFailure.
|
||||
val staged = File(context.cacheDir, "conversions")
|
||||
assertTrue(
|
||||
"a failed save must not delete the staged file; it may be the only copy",
|
||||
staged.listFiles()?.isNotEmpty() == true,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Leaves nothing for the next test's launch to reattach to.
|
||||
*
|
||||
* **In teardown rather than at the end of a test, and that placement is the point.**
|
||||
* `aFailedSaveDeletesTheDocumentItCouldNotWrite` proves that a failed save *keeps* its staged
|
||||
* file — deliberately, since it may be the only copy — so it ends with a finished job and a
|
||||
* live staged file, which is exactly what the app reattaches to on the next launch. Its
|
||||
* sibling then opened on `Converted` with no "Choose file" to tap: measured, as a 30 s timeout
|
||||
* on `converter.chooseFile` in a test that had nothing wrong with it.
|
||||
*
|
||||
* The first fix tapped "Start over" at the end of the test body. That works until the test
|
||||
* fails, and then it does not run at all — measured too, on the mutation run that proved this
|
||||
* suite bites: one real failure became two, and the second looked like an unrelated flake.
|
||||
* **One cause must produce one red test**, so the cleanup belongs where it runs either way.
|
||||
*
|
||||
* **`pruneWork` and not `cancelAllWork`, on design grounds and not on a measurement.** Only
|
||||
* finished work records need to go — that is all the next launch reattaches to — and
|
||||
* `cancelAllWork` additionally cancels live work, which is a wider blast radius than teardown
|
||||
* in a shared process needs. `pruneWork` cannot touch a job that has not run yet.
|
||||
*
|
||||
* `cancelAllWork` was **suspected** of causing an API 35 red here and did not cause it; see
|
||||
* [CONVERSION_TIMEOUT_MS], which did. A local API 35 run with `cancelAllWork` passed, and the
|
||||
* logcat showed the conversion encoding rather than cancelled. The narrower call is kept
|
||||
* because it is the right one, not because it fixed anything.
|
||||
*/
|
||||
private fun clearFinishedWork() {
|
||||
WorkManager.getInstance(context).pruneWork()
|
||||
File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() }
|
||||
}
|
||||
|
||||
/** Whether [destination] still answers a metadata query. A deleted document does not. */
|
||||
private fun documentStillExists(destination: Uri): Boolean = runCatching {
|
||||
context.contentResolver
|
||||
.query(destination, arrayOf(OpenableColumns.SIZE), null, null, null)
|
||||
?.use { it.moveToFirst() } ?: false
|
||||
}.getOrDefault(false)
|
||||
|
||||
/**
|
||||
* Runs the conversion, leaving the screen on `Converted`.
|
||||
*
|
||||
@@ -617,7 +775,7 @@ class SafPickerRoundTripTest {
|
||||
* Retried whole, for the reason [pickTheFixture] documents: a dialog that came up unreadable
|
||||
* cannot be recovered from inside, and a fresh one is the only answer.
|
||||
*/
|
||||
private fun saveThroughTheSystemPicker() {
|
||||
private fun saveThroughTheSystemPicker(settlesOn: String = TestTags.Converter.CONVERT_ANOTHER) {
|
||||
var missing: BySelector? = null
|
||||
repeat(PICK_ATTEMPTS) { attempt ->
|
||||
requireAReadableScreen()
|
||||
@@ -626,7 +784,9 @@ class SafPickerRoundTripTest {
|
||||
if (attempt == 0) PICKER_TIMEOUT_MS else REOPENED_TIMEOUT_MS,
|
||||
)
|
||||
if (missing == null) {
|
||||
awaitNode(TestTags.Converter.CONVERT_ANOTHER, SAVE_TIMEOUT_MS)
|
||||
// The node that says the save has *finished*, either way. Waiting on the success
|
||||
// one when the save is meant to fail would time out on a test that is working.
|
||||
awaitNode(settlesOn, SAVE_TIMEOUT_MS)
|
||||
return
|
||||
}
|
||||
dismissThePicker()
|
||||
@@ -1038,9 +1198,35 @@ class SafPickerRoundTripTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Waits for [tag], treating "the app has no composition right now" as *not yet* rather than
|
||||
* as a failure.
|
||||
*
|
||||
* `fetchSemanticsNodes` **throws** `IllegalStateException: No compose hierarchies found in the
|
||||
* app` when nothing is attached at that instant, and `waitUntil` propagates it on the first
|
||||
* poll instead of waiting out the deadline. This class spends much of its time with another
|
||||
* app in front — the picker, the create-document dialog, the permission dialog — so there is
|
||||
* always a window where the app is coming back and has no composition yet. Before this, that
|
||||
* window was a hard failure: measured on the API 34 leg of run 34057196628, where **both** SAF
|
||||
* tests died that way while the same commit passed API 33, 35, 36 and 37, and the previous
|
||||
* commit passed API 34 and failed 35. A failing leg that moves between runs is #190's
|
||||
* emulator flake, and this is the one place in the class that turned it into a red test.
|
||||
*
|
||||
* **The cost is honest and bounded**: an app that is genuinely gone now fails at the deadline
|
||||
* rather than immediately, so the last composition error is carried into the message to keep
|
||||
* that case diagnosable.
|
||||
*/
|
||||
private fun awaitNode(tag: String, timeoutMs: Long = APP_TIMEOUT_MS) {
|
||||
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
|
||||
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
|
||||
var lastError: Throwable? = null
|
||||
try {
|
||||
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
|
||||
runCatching { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty() }
|
||||
.onFailure { lastError = it }
|
||||
.getOrDefault(false)
|
||||
}
|
||||
} catch (timeout: ComposeTimeoutException) {
|
||||
val note = lastError?.let { "; last composition error: ${it.message}" } ?: ""
|
||||
throw AssertionError("waited ${timeoutMs}ms for a node tagged $tag$note", timeout)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1060,8 +1246,23 @@ class SafPickerRoundTripTest {
|
||||
/** Short: either the dialog is up almost immediately, or the permission was already held. */
|
||||
const val PERMISSION_DIALOG_MS = 5_000L
|
||||
|
||||
/** A 3 s clip to MP3 on an emulator is about a second; this only bounds a hang. */
|
||||
const val CONVERSION_TIMEOUT_MS = 120_000L
|
||||
/**
|
||||
* Bounds a hang, and **the first number here was measured on one API level and wrong on
|
||||
* another.** It read 120 s, on the strength of the whole test taking 11.8 s on the API 34
|
||||
* CI leg (run 34043502322). API 35 is a different machine: on run 34056545386 the fixture's
|
||||
* `libx265 -crf 24 -preset veryfast` encode started at `20:05:26.897` and the next job in
|
||||
* the suite did not appear until `20:07:41.693` — **134.8 s**, so the encode was still
|
||||
* running when the 120 s bound expired and the test failed with the conversion healthy.
|
||||
*
|
||||
* The logcat is what settles it: `ConversionWorker` logs the route and `FFmpegEngine` the
|
||||
* command, and there is no cancel between them. A timeout that fires on a working
|
||||
* conversion is worse than no bound, because it reads as a product failure.
|
||||
*
|
||||
* 300 s is chosen against that 134.8 s, not against API 34's 11.8 s. **Do not re-tighten
|
||||
* it from a fast leg's timing** — the encode is software on every emulator here, and the
|
||||
* spread between images is larger than any margin a single measurement would suggest.
|
||||
*/
|
||||
const val CONVERSION_TIMEOUT_MS = 300_000L
|
||||
|
||||
/** The copy is a few kilobytes, but it crosses a provider. */
|
||||
const val SAVE_TIMEOUT_MS = 30_000L
|
||||
|
||||
+137
-2
@@ -1,6 +1,6 @@
|
||||
# E2E-read findings
|
||||
|
||||
**Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a
|
||||
**Status:** seven findings; E4 fixed, E7 extended and its ticket closed, the rest standing — **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
|
||||
@@ -274,6 +274,27 @@ ticket is about.
|
||||
**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays
|
||||
#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so.
|
||||
|
||||
**Updated 2026-09-06, doing it: there is a second constraint underneath, and it has the same
|
||||
cause.** The obvious way to avoid driving the app was a host Activity in `androidTest` owning its
|
||||
own `CreateDocument` launcher. It cannot be started at all:
|
||||
|
||||
```
|
||||
java.lang.RuntimeException: Intent in process org.libremediaconverter resolved to different
|
||||
process org.libremediaconverter.test
|
||||
at android.app.Instrumentation.startActivitySync
|
||||
```
|
||||
|
||||
Instrumentation runs in the target app's process, so a component declared in the instrumentation
|
||||
APK is in the wrong one — the same fact that sinks approach 2 above, arriving from the other side.
|
||||
**The app's own Save button is the only launcher available to drive**, which is also the more
|
||||
faithful thing to drive. `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` is
|
||||
what came of it.
|
||||
|
||||
**And the premise turned out to be true**, which is the answer #226 was filed for: on API 34,
|
||||
stock DocumentsUI hands back a document URI reporting a size of exactly zero. `deletePartialOutput`
|
||||
can fire, and D4's fix is live rather than inert. A "no defect found" — and not one that could have
|
||||
been reached by reading.
|
||||
|
||||
### What this does *not* block, which is the useful half
|
||||
|
||||
`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no
|
||||
@@ -361,7 +382,7 @@ decision, not a detail — see **E6** for why no third option exists — and **#
|
||||
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
|
||||
| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
|
||||
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | closed — the *premise* holds; see E7. The delete **arm** is still unrun: **#250** |
|
||||
| **#227** | The notification's Cancel action has never been fired | closed |
|
||||
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
|
||||
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
|
||||
@@ -380,3 +401,117 @@ unasserted value, it was **a combination of two covered things that no test put
|
||||
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.
|
||||
|
||||
## The 2026-09-06 re-check
|
||||
|
||||
Run after the last ticket landed, to ask whether the suite's self-description had drifted again. It
|
||||
had, and **every drifted line came from #226 — the last PR of this read's own wave.**
|
||||
|
||||
The suite is 70 tests in 14 classes, 6 carrying `@FailsOnEmulatorApi37`, gating leg 64; the
|
||||
committed baseline says 6 and the advisory job agrees (`baseline: matches`). Every gating leg is
|
||||
green on `main`.
|
||||
|
||||
- **The counts had gone stale in four places** — `CLAUDE.md` (three sites),
|
||||
`FailsOnEmulatorApi37.kt`'s KDoc, and two comments in `status_check.yml` — all still saying five
|
||||
carriers of 69. **The gating figure is what hid it**: 69 − 5 and 70 − 6 are both 64, so the one
|
||||
number a reader would check against a run had not moved. CLAUDE.md's own instruction to derive
|
||||
these rather than remember them is what caught it.
|
||||
- **Two KDoc claims in `SafPickerRoundTripTest` described a draft rather than the code.** The save
|
||||
test says MP3 was chosen so the setup could not depend on device codecs; the code converts at the
|
||||
default `MP4_H265` / `FAST`, which routes by `canEncode(H265)`. The *negation* of the stated
|
||||
reason was true. This is **E1 and E3's failure mode landing in a test written by the read that
|
||||
found it** — a passing test with a wrong explanation.
|
||||
- **Neither picker test has ever reported on the advisory leg.** The marker's KDoc said the picker
|
||||
test *fails* there behind the rotation test; with six carriers the rotation test truncates the run
|
||||
first, and all four advisory runs at this baseline (`34041156680`, `34041593697`,
|
||||
`34042397320`, `34043502322`) report `expected: 6, received: 4, failed: 4` — the three Media3
|
||||
tests plus the rotation. The save test is therefore
|
||||
marked by **inheritance, not measurement**, which is now what both KDocs say.
|
||||
- **One substantive gap, filed as #250.** `FixtureDocumentsProvider.deletedDocumentIds()` has no
|
||||
callers. #226 proved D4's *premise* — SAF hands back a document of exactly zero bytes — but drove
|
||||
only the success path, so `deletePartialOutput` against a real `DocumentsProvider` is still
|
||||
asserted nowhere. `openDestination` is `protected open` precisely to force the failure, so the
|
||||
test is cheap; it costs another marked picker test and a baseline of 7.
|
||||
|
||||
**The reusable part is the second bullet.** A read that fixes documentation drift can introduce it in
|
||||
the same wave, and the tests it writes are no more self-describing than the ones it audited. The
|
||||
check that found it is the one this document already recommends: **read the KDoc against the code,
|
||||
not against the ticket.**
|
||||
|
||||
## E8 — the instrumented suite's coverage, measured for the first time
|
||||
|
||||
**Severity: n/a · Measured 2026-09-06 on API 34 · the number had never existed**
|
||||
|
||||
Four coverage waves were steered by a figure that cannot see `app/src/androidTest`. Nothing had
|
||||
ever produced the other half, because `enableAndroidTestCoverage` was unset, so a connected run
|
||||
emitted no `.ec` at all and `jacocoTestReport`'s execution data names only `testDebugUnitTest`.
|
||||
|
||||
Measured by setting that flag temporarily, running `run-e2e.sh 34` (70/70, 0 failed, 3m21s — the
|
||||
instrumentation destabilised nothing), and reporting the resulting `.ec` against the **same** class
|
||||
directories and exclusions the committed task uses:
|
||||
|
||||
| suite | line | branch |
|
||||
|---|---|---|
|
||||
| JVM `testDebugUnitTest` | 2236/2374 — **94.2%** | 1171/1338 — **87.5%** |
|
||||
| Instrumented, 70 tests | 1711/2374 — **72.1%** | 669/1354 — **49.4%** |
|
||||
| **Union** | 2342/2374 — **98.7%** | 1212/1354 — **89.5%** |
|
||||
|
||||
The JVM row reproduced the committed figure exactly, which is the control: both exec sets match the
|
||||
current class files, so the union is trustworthy.
|
||||
|
||||
**Two caveats before anyone quotes these.** Branch denominators differ by 16 — 1338 against 1354 —
|
||||
entirely inside `MediaProbe`, an artefact of offline versus on-the-fly instrumentation; line
|
||||
denominators are identical at 2374, so only the line figures compare exactly. And **72.1% is not a
|
||||
grade for the instrumented suite.** Seventy end-to-end tests reach code broadly and choose arms
|
||||
rarely; a branch figure of 49.4% is what that shape looks like. This whole document exists because
|
||||
the gaps that mattered — #223's vacuous assertions, #238's two covered things nobody combined —
|
||||
are invisible to any percentage.
|
||||
|
||||
### What the device suite is for, in numbers
|
||||
|
||||
It closes **106 lines** the JVM suite misses, and they are precisely the ones wave 4 wrote off:
|
||||
|
||||
| file | JVM missed | union missed |
|
||||
|---|---|---|
|
||||
| `FFmpegEngine.kt` | 32 | **0** |
|
||||
| `Media3Engine.kt` | 24 | **0** |
|
||||
| `ConcatEngine.kt` | 15 | **0** |
|
||||
| `MediaProbe.kt` | 13 | **3** |
|
||||
| `MainActivity.kt` | 10 | **1** |
|
||||
| `AndroidDeviceCodecs.kt` | 8 | **0** |
|
||||
| `Transcoders.kt` | 10 | 3 |
|
||||
|
||||
CLAUDE.md's wave-4 read called 81 lines "native or device edges" — `FFmpegEngine` 33,
|
||||
`Media3Engine` 24, `ConcatEngine` 14, `MainActivity.onCreate` 10. The first four rows above total
|
||||
**81**, and the union leaves **1**. That **confirms** the read's own hypothesis rather than
|
||||
overturning it: it always said those zeroes were "the `testDebugUnitTest`-only measurement
|
||||
boundary". Nobody had measured past the boundary. `AndroidDeviceCodecs` is the pointed one — #194
|
||||
was filed to cut a seam because `probe()` could not be reached, and on a device it is fully covered.
|
||||
|
||||
### The 32 lines neither suite reaches, classified
|
||||
|
||||
Every one was read. **None of them is an e2e test gap**, which is the result:
|
||||
|
||||
| lines | where | classification |
|
||||
|---|---|---|
|
||||
| 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` |
|
||||
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252** |
|
||||
| 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record |
|
||||
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
|
||||
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
|
||||
| 2 | `FFmpegCommandBuilder:167-168` | `COPY`/`NONE -> error(...)` — F4-shaped, deliberately exempt |
|
||||
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produces it, but `ContainerCapabilities` lists it for WEBM and OGG. **#254** |
|
||||
| 1 | `ConversionWorker:231` | `?: error("Could not open the input file.")`. `UnopenableUriTest` fails the job *downstream* of it, so the elvis is unprovoked — F4-shaped, same as the two above |
|
||||
|
||||
**`MediaProbe:210-212` is the one that gained a measurement.** F7 ruled `probeWithExtractor`'s catch
|
||||
unreachable because Robolectric's `MediaExtractor` never throws. That reasoning does not transfer:
|
||||
`probeWithFFprobe` calls `readMediaInformation` in native ffmpeg-kit, which the JVM never loads.
|
||||
But `MediaProbe.probe` calls **both** probes on one line, and
|
||||
`RemuxTest.probeDistinguishesAudioFromImagesFromRubbish` drives it on a device with 4096 bytes of
|
||||
garbage — so the ffprobe path *has* been given malformed input on real hardware and **did not
|
||||
throw**. Same conclusion as F7, reached by a different mechanism, and now on record rather than
|
||||
assumed.
|
||||
|
||||
**The reusable part**: a union report is what separates "no test calls this" from "only a device
|
||||
calls it", and neither report alone can. Six of the eight rows above were indistinguishable from
|
||||
real gaps in the JVM-only number.
|
||||
|
||||
Executable
+249
@@ -0,0 +1,249 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# The local gate: what has to be green before a commit is made or a branch is pushed.
|
||||
#
|
||||
# THE RULE THIS ENFORCES (2026-09-06). Source changes must have the unit tests AND the
|
||||
# instrumented tests passing at every supported API level before they are committed or
|
||||
# pushed; test changes must have the whole suite passing at every API level. CI is not the
|
||||
# place to find out. Four legs of this repo's history were spent discovering on CI what a
|
||||
# local sweep would have said in twenty minutes -- and worse, the failing leg MOVED between
|
||||
# runs (API 35 red then green, API 34 green then red), which is exactly the signal that gets
|
||||
# misread as "someone else's flake" when it is read one leg at a time.
|
||||
#
|
||||
# WHY BOTH HOOKS RUN THE SAME GATE. A pre-commit-only gate is bypassed by amending; a
|
||||
# pre-push-only gate lets a broken commit exist locally and get rebased into something else.
|
||||
# Running both is not redundant in practice because of the cache below.
|
||||
#
|
||||
# THE CACHE IS KEYED ON CONTENT, NOT ON TIME, AND ON THE RIGHT CONTENT. The sweep is recorded
|
||||
# under the hash of the `app/src` SUBTREE it verified, not the whole repo tree. Keying it on the
|
||||
# whole tree was the first cut and it was wrong in a way that would have trained people to hate
|
||||
# this hook: editing a comment in CLAUDE.md, or in this script, invalidated a sweep of identical
|
||||
# application code and re-ran forty minutes of emulators to prove nothing. What the sweep is
|
||||
# evidence about is `app/src`; that is what it is filed under. Any change to a single byte under
|
||||
# `app/src` still invalidates it. The JVM gate is cheap and runs unconditionally.
|
||||
#
|
||||
# WHAT COUNTS AS "EVERY SUPPORTED API LEVEL", AND WHY 37 IS NOT AN EMULATOR HERE. 33, 34, 35
|
||||
# and 36 run the whole suite on emulators. **API 37 cannot be run on an emulator on this host at
|
||||
# all** -- not "is red", cannot run: measured 2026-09-06, the image logs
|
||||
# `3 new surfaceflinger aborts in 45 s (want 0)` and then the APK install itself fails with
|
||||
# `Can't find service: package`, because the framework is already gone before Gradle gets to
|
||||
# install anything. `Starting 0 tests`. That is the same gralloc abort docs/api-37-emulator-crash.md
|
||||
# measures, hit earlier in the sequence than the suite.
|
||||
#
|
||||
# So API 37 is covered here by the physical Pixel 10 Pro XL when it is attached, and by CI's
|
||||
# gating leg otherwise. The hook says loudly which of the two happened rather than quietly
|
||||
# claiming five levels when it ran four.
|
||||
#
|
||||
# THERE IS DELIBERATELY NO SKIP VARIABLE. An `LMC_SKIP_E2E=1` would be `--no-verify` wearing
|
||||
# a different hat, and `--no-verify` needs the repo owner's say-so each time. If this gate is
|
||||
# wrong, fix the gate.
|
||||
set -uo pipefail
|
||||
|
||||
REPO_ROOT="$(git rev-parse --show-toplevel)"
|
||||
cd "$REPO_ROOT" || exit 1
|
||||
|
||||
MODE="$(basename "$0")"
|
||||
ZERO="0000000000000000000000000000000000000000"
|
||||
# `git rev-parse --git-common-dir`, not a literal ".git" (#258). In a linked worktree `.git` is a
|
||||
# FILE containing `gitdir: ...`, so `mkdir -p .git/lmc-verify` fails with "Not a directory" -- and
|
||||
# because the write is the last thing this script does, it failed while the gate still printed
|
||||
# green and exited 0. Every push from a worktree then re-swept 33-36 for nothing, silently, which
|
||||
# is the worst shape a cache can fail in: invisible and expensive.
|
||||
#
|
||||
# --git-common-dir rather than --git-dir so the cache is SHARED across worktrees. The key is the
|
||||
# app/src tree hash, and identical content is identical content whichever worktree produced it.
|
||||
CACHE_DIR="$(git rev-parse --git-common-dir)/lmc-verify"
|
||||
GRADLE_GATE=(:app:assembleDebug :app:testDebugUnitTest :app:compileDebugAndroidTestKotlin
|
||||
:app:ktlintCheck :app:detekt :app:lintDebug)
|
||||
|
||||
# Says so when it cannot record, rather than leaving a cache that silently never fills (#258).
|
||||
record_sweep() {
|
||||
[ -n "$tree" ] || return 0
|
||||
if mkdir -p "$CACHE_DIR" 2>/dev/null && : > "$CACHE_DIR/$tree" 2>/dev/null; then
|
||||
return 0
|
||||
fi
|
||||
printf '\n\033[1m[local-gate]\033[0m could not record the sweep under %s -- it will re-run next
|
||||
time. Not fatal, but it means every commit and push pays for it again.\n' "$CACHE_DIR"
|
||||
}
|
||||
|
||||
say() { printf '\n\033[1m[local-gate]\033[0m %s\n' "$*"; }
|
||||
die() {
|
||||
printf '\n\033[1;31m[local-gate] BLOCKED\033[0m %s\n' "$*"
|
||||
printf ' The rule: source work needs unit + e2e green at every API level before commit/push;\n'
|
||||
printf ' test work needs the whole suite green at every level. Fix it, or ask before using\n'
|
||||
printf ' --no-verify -- that flag is not yours to reach for unprompted.\n\n'
|
||||
exit 1
|
||||
}
|
||||
|
||||
# --- what changed, and what tree is being verified ---------------------------------------
|
||||
|
||||
changed_files=""
|
||||
tree=""
|
||||
case "$MODE" in
|
||||
pre-commit)
|
||||
changed_files="$(git diff --cached --name-only --diff-filter=ACMR)"
|
||||
tree="$(git rev-parse "$(git write-tree):app/src" 2>/dev/null || echo "")"
|
||||
;;
|
||||
pre-push)
|
||||
# stdin is `<local ref> <local sha> <remote ref> <remote sha>`, one line per ref pushed.
|
||||
while read -r _ local_sha _ remote_sha; do
|
||||
[ "$local_sha" = "$ZERO" ] && continue # branch deletion carries no content
|
||||
base="$remote_sha"
|
||||
if [ "$remote_sha" = "$ZERO" ]; then
|
||||
# A new branch: compare against main rather than against every commit ever made.
|
||||
base="$(git merge-base origin/main "$local_sha" 2>/dev/null || echo "")"
|
||||
fi
|
||||
if [ -n "$base" ]; then
|
||||
changed_files="$changed_files$(git diff --name-only --diff-filter=ACMR "$base" "$local_sha")"$'\n'
|
||||
else
|
||||
changed_files="$changed_files$(git show --pretty=format: --name-only "$local_sha")"$'\n'
|
||||
fi
|
||||
tree="$(git rev-parse "$local_sha:app/src" 2>/dev/null || echo "")"
|
||||
done
|
||||
;;
|
||||
*)
|
||||
say "unknown hook name '$MODE'; nothing to do"
|
||||
exit 0
|
||||
;;
|
||||
esac
|
||||
|
||||
if [ -z "${changed_files//[[:space:]]/}" ]; then
|
||||
say "no added/modified files; nothing to verify"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
touches_source=0
|
||||
touches_tests=0
|
||||
while IFS= read -r f; do
|
||||
case "$f" in
|
||||
app/src/main/*) touches_source=1 ;;
|
||||
app/src/test/*|app/src/androidTest/*) touches_tests=1 ;;
|
||||
esac
|
||||
done <<< "$changed_files"
|
||||
|
||||
# --- the cheap gate always runs -----------------------------------------------------------
|
||||
|
||||
# --- shellcheck, at CI's exact pin ---------------------------------------------------------
|
||||
# WHY THIS IS HERE. The gate ran ktlint, detekt and Android lint but not shellcheck, so a new or
|
||||
# edited `.sh` file was precisely the case where this hook passed and CI's Static analysis leg
|
||||
# still went red. That is not hypothetical: this script is itself a new `.sh` file, and the first
|
||||
# thing it could not check was itself. It was caught by hand twice before it was caught here.
|
||||
#
|
||||
# THE DIGEST IS READ OUT OF status_check.yml, NOT COPIED INTO THIS FILE. shellcheck 0.9.0 and
|
||||
# 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body lines versus SC2329
|
||||
# once on the declaration, same script, same directive, one red and one green. That disagreement
|
||||
# is why CI pins by digest, and a second copy of the digest here would drift from it silently.
|
||||
# When it drifts, the symptom is this gate passing and CI failing: the exact thing this section
|
||||
# exists to prevent. So there is one digest in the repo and this reads it.
|
||||
#
|
||||
# ALL TRACKED FILES, not just changed ones, because that is what CI does -- `git ls-files '*.sh'`.
|
||||
# The point is to predict that leg, not to audit the diff.
|
||||
shellcheck_pin="$(grep -oE 'koalaman/shellcheck@sha256:[0-9a-f]{64}' \
|
||||
.github/workflows/status_check.yml | head -1)"
|
||||
# :z is podman's SELinux relabel and is what this host needs; docker on CI does without it.
|
||||
runtime=""
|
||||
mount=":z"
|
||||
for candidate in podman docker; do
|
||||
if command -v "$candidate" >/dev/null 2>&1; then
|
||||
runtime="$candidate"
|
||||
[ "$candidate" = "docker" ] && mount=""
|
||||
break
|
||||
fi
|
||||
done
|
||||
|
||||
if [ -z "$shellcheck_pin" ]; then
|
||||
say "NOT COVERED: shellcheck. Could not read the pinned digest out of
|
||||
.github/workflows/status_check.yml -- if that pin moved or was reformatted, fix this grep
|
||||
rather than leaving the check silently absent."
|
||||
elif [ -z "$runtime" ]; then
|
||||
say "NOT COVERED: shellcheck. Neither podman nor docker is on PATH, and there is no shellcheck
|
||||
system package on this host. CI's Static analysis leg is what answers for .sh files then."
|
||||
else
|
||||
say "shellcheck ($runtime, $shellcheck_pin)"
|
||||
if ! git ls-files -z '*.sh' |
|
||||
xargs -0 -r "$runtime" run --rm -v "$PWD:/mnt$mount" "docker.io/$shellcheck_pin"; then
|
||||
die "shellcheck failed. CI runs the same digest over the same files, so this is a red
|
||||
Static analysis leg waiting to happen."
|
||||
fi
|
||||
fi
|
||||
|
||||
# --- actionlint, the half shellcheck cannot see ---------------------------------------------
|
||||
# A good deal of this repo's bash lives in workflow `run:` blocks, which `git ls-files '*.sh'`
|
||||
# does not match at all -- so without this a workflow edit is the same hole the section above
|
||||
# just closed: green here, red on Static analysis. Pinned by digest for the reason in that
|
||||
# section, and for actionlint's own: its documented install is `curl | bash` off a moving branch,
|
||||
# which does not belong in a repo that pins every action by SHA.
|
||||
actionlint_pin="$(grep -oE 'rhysd/actionlint@sha256:[0-9a-f]{64}' \
|
||||
.github/workflows/status_check.yml | head -1)"
|
||||
|
||||
if [ -z "$actionlint_pin" ]; then
|
||||
say "NOT COVERED: actionlint. Could not read the pinned digest out of
|
||||
.github/workflows/status_check.yml -- fix this grep rather than leaving the check absent."
|
||||
elif [ -z "$runtime" ]; then
|
||||
say "NOT COVERED: actionlint. Neither podman nor docker is on PATH; CI's Static analysis leg
|
||||
is what answers for the workflows then."
|
||||
else
|
||||
say "actionlint ($runtime, $actionlint_pin)"
|
||||
if ! "$runtime" run --rm -v "$PWD:/repo$mount" -w /repo "docker.io/$actionlint_pin" -color; then
|
||||
die "actionlint failed. CI runs the same digest over the same workflows."
|
||||
fi
|
||||
fi
|
||||
|
||||
say "$MODE: running the JVM gate"
|
||||
if ! ./gradlew "${GRADLE_GATE[@]}" --continue; then
|
||||
die "the JVM gate failed (assemble, unit tests, androidTest compile, ktlint, detekt, lint)."
|
||||
fi
|
||||
|
||||
# --- the sweep, when code is involved ------------------------------------------------------
|
||||
|
||||
if [ "$touches_source" -eq 0 ] && [ "$touches_tests" -eq 0 ]; then
|
||||
say "no app/src changes; the instrumented sweep is not required for this one"
|
||||
record_sweep
|
||||
exit 0
|
||||
fi
|
||||
|
||||
if [ -n "$tree" ] && [ -f "$CACHE_DIR/$tree" ]; then
|
||||
say "app/src ($tree) already swept and green; nothing under app/src has changed since"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
say "app/src changed -- sweeping API 33, 34, 35, 36 (this takes tens of minutes, by design)"
|
||||
if ! tools/local-emulator/run-e2e.sh 33 34 35 36; then
|
||||
die "the instrumented suite is not green on 33-36."
|
||||
fi
|
||||
|
||||
# API 37: the physical device if it is here, and an honest statement if it is not. run-e2e.sh is
|
||||
# emulator-only (and overwrites E2E_EXTRA_GRADLE_ARGS with --rerun, so extra args cannot be passed
|
||||
# through it), so this drives Gradle directly with the serial pinned -- the phone must never be
|
||||
# picked up by accident, which is the hazard run-e2e.sh's header calls out.
|
||||
export ANDROID_HOME="${ANDROID_HOME:-$HOME/Android/Sdk}"
|
||||
export PATH="$ANDROID_HOME/platform-tools:$PATH"
|
||||
|
||||
device=""
|
||||
while read -r serial state; do
|
||||
[ "$state" = "device" ] || continue
|
||||
case "$serial" in emulator-*) continue ;; esac
|
||||
[ "$(adb -s "$serial" shell getprop ro.build.version.sdk 2>/dev/null | tr -d '\r')" = "37" ] || continue
|
||||
device="$serial"
|
||||
break
|
||||
done < <(adb devices 2>/dev/null | tail -n +2)
|
||||
|
||||
levels="33, 34, 35, 36"
|
||||
if [ -n "$device" ]; then
|
||||
say "API 37 on the attached device $device"
|
||||
if ! ANDROID_SERIAL="$device" ./gradlew :app:connectedDebugAndroidTest -PabiFilters=arm64-v8a; then
|
||||
die "the instrumented suite is not green on API 37 (device $device)."
|
||||
fi
|
||||
levels="$levels, 37"
|
||||
else
|
||||
say "NOT COVERED LOCALLY: API 37. No API 37 device is attached, and the API 37 emulator cannot
|
||||
install the APK on this host (see this script's header). CI's gating leg is what answers for it;
|
||||
attach the Pixel 10 Pro XL to have this hook cover it too."
|
||||
fi
|
||||
|
||||
record_sweep
|
||||
# Name the levels rather than claiming "every supported level". The first cut said the latter on
|
||||
# both paths, including the one that had just printed NOT COVERED two lines above -- a false claim
|
||||
# printed by the tool whose whole job is to stop false claims reaching CI.
|
||||
say "green on API $levels; $MODE allowed"
|
||||
exit 0
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
local-gate.sh
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
local-gate.sh
|
||||
Reference in New Issue
Block a user