Compare commits
13
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4d21996735 | ||
|
|
264b8027e4 | ||
|
|
7d1d3191a9 | ||
|
|
6a8cc01862 | ||
|
|
68b863fbdb | ||
|
|
f4174e5b06 | ||
|
|
dd01f9f27c | ||
|
|
dd76229e90 | ||
|
|
8105291f6a | ||
|
|
68bd24e54a | ||
|
|
19e35394e1 | ||
|
|
cbbaf74285 | ||
|
|
fac8e67db2 |
@@ -272,13 +272,13 @@ 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 six tests off this row. SafPickerRoundTripTest's
|
||||
# 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 THREE of that class's tests now
|
||||
# carry the marker -- the save through the picker (#226) joined on 2026-09-06
|
||||
# by inheritance rather than measurement, since it opens the same picker.
|
||||
# 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.
|
||||
#
|
||||
@@ -290,11 +290,11 @@ 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 six tests that cannot be RUN 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. "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 two picker tests behind it never report at all.
|
||||
# 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"
|
||||
|
||||
@@ -76,20 +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. **Six** of the 70 instrumented
|
||||
tests cannot be *run* on that image, for three measured reasons and one inherited: three Media3
|
||||
- **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
|
||||
save through the picker (#226), carries the marker because it opens the same picker and a second
|
||||
DocumentsUI dialog on top of it — **not** because it has ever been observed here. It cannot be:
|
||||
the rotation test runs first and takes the framework down, so **all four** advisory runs at this
|
||||
baseline report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests
|
||||
plus the rotation — runs 34041156680, 34041593697, 34042397320 and 34043502322. **Neither picker
|
||||
test has ever reported on the advisory leg**, which is a correction to what the marker's own KDoc
|
||||
says. All six carry
|
||||
`@FailsOnEmulatorApi37` and run in a separate `continue-on-error` job; the gating leg runs the
|
||||
other 64 — **the same 64 as before**, which is exactly how this paragraph went stale unnoticed.
|
||||
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
|
||||
@@ -122,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 six 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,
|
||||
@@ -142,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 six 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:
|
||||
@@ -398,6 +399,43 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
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.
|
||||
|
||||
@@ -10,7 +10,7 @@ package org.libremediaconverter
|
||||
* drift, and the drift is silent in both directions (a test that runs nowhere reads as green).
|
||||
*
|
||||
* **"Cannot be run" covers three things now, and it covered only the first until 2026-09-05.**
|
||||
* Four of the six carriers simply fail: three Media3 tests die in the image's own
|
||||
* 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
|
||||
@@ -18,12 +18,13 @@ package org.libremediaconverter
|
||||
* point at (#108). The wording was widened rather than the test excused; that test's own KDoc has
|
||||
* the four-run measurement.
|
||||
*
|
||||
* **The sixth is the new third thing: it is marked by inheritance, not by measurement.**
|
||||
* `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226) opens the same
|
||||
* picker and then a second DocumentsUI dialog on top of it, so it sits on the same task-snapshot
|
||||
* path its sibling was marked for. It has never been observed at API 37 either way — see the
|
||||
* measurement under [FAILS_ON_EMULATOR_API37_BASELINE], which is why it cannot be. Marking it was
|
||||
* the conservative choice, and **the trigger for revisiting it is the rotation test, not itself**:
|
||||
* **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
|
||||
@@ -58,8 +59,8 @@ annotation class FailsOnEmulatorApi37
|
||||
* 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 two
|
||||
* of the six start, which the last paragraph below measures. `expected` still holds, and it is the
|
||||
* **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 tests are the ones to read that sentence carefully for, and the reason changed
|
||||
@@ -79,10 +80,11 @@ annotation class FailsOnEmulatorApi37
|
||||
* 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.** With six carriers the rotation test truncates the run before them: **all four** advisory
|
||||
* runs at this baseline — 34041156680, 34041593697, 34042397320 and 34043502322 — report
|
||||
* `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
|
||||
* rotation. So the advisory leg currently answers for
|
||||
* 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.
|
||||
*
|
||||
@@ -96,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,34 +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.
|
||||
*
|
||||
* <p><b>Nothing reads this yet, and that is recorded rather than hidden (#250).</b> It was
|
||||
* added with #226 to assert {@code OutputPublisher.deletePartialOutput} — D4's cleanup — against
|
||||
* a real {@code DocumentsProvider}. #226 only reached the <i>success</i> path, so the
|
||||
* {@code catch} that calls it is still asserted only against {@code FakeSafProvider} under
|
||||
* Robolectric. It is kept because the forcing condition is one {@code openDestination} override
|
||||
* away and #250 says exactly what to add; if that ticket is closed any other way, delete this
|
||||
* and {@link #DELETED} with it rather than leaving an accessor implying coverage.
|
||||
*/
|
||||
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()
|
||||
@@ -570,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`.
|
||||
*
|
||||
@@ -636,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()
|
||||
@@ -645,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()
|
||||
@@ -1057,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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1080,13 +1247,22 @@ class SafPickerRoundTripTest {
|
||||
const val PERMISSION_DIALOG_MS = 5_000L
|
||||
|
||||
/**
|
||||
* Only bounds a hang, and it is an order of magnitude clear of the real cost: the whole
|
||||
* test — pick, convert, save — takes **11.8 s** on the API 34 CI leg (run 34043502322).
|
||||
* Deliberately generous because the engine is not fixed: the default `MP4_H265` at `FAST`
|
||||
* lands on FFmpeg on an emulator and on Media3 on real hardware, which is faster rather
|
||||
* than slower — see [convertToTheDefaultFormat].
|
||||
* 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 = 120_000L
|
||||
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
|
||||
|
||||
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
|
||||
import androidx.work.Data
|
||||
import androidx.work.ForegroundInfo
|
||||
import androidx.work.OneTimeWorkRequestBuilder
|
||||
import androidx.work.OutOfQuotaPolicy
|
||||
import androidx.work.WorkerParameters
|
||||
import androidx.work.hasKeyWithValueOfType
|
||||
import androidx.work.workDataOf
|
||||
@@ -27,6 +28,10 @@ import org.libremediaconverter.model.OutputFormat
|
||||
* Progress is not reported. FFmpeg's statistics callback gives a timestamp against a
|
||||
* single input's duration, which is meaningless once several files are being
|
||||
* concatenated; showing a fabricated percentage would be worse than showing none.
|
||||
*
|
||||
* Enqueued as **expedited** work for the same reasons, and with the same caveats, as
|
||||
* [ConversionWorker] — its class KDoc carries both, and a join is user-initiated in exactly the
|
||||
* way a conversion is.
|
||||
*/
|
||||
@UnstableApi
|
||||
class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) {
|
||||
@@ -68,13 +73,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
||||
// which is where a WorkManager restart after process death always begins -- used to
|
||||
// throw straight past this catch, taking the retry, the error message and the delete
|
||||
// with it. See ConversionWorker.doWork and FailureOutcome.
|
||||
setForeground(
|
||||
ForegroundInfo(
|
||||
NOTIFICATION_ID,
|
||||
notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true),
|
||||
ConversionForegroundType.current(),
|
||||
),
|
||||
)
|
||||
//
|
||||
// Posted through getForegroundInfo() rather than built here a second time -- see that
|
||||
// override, and its twin in ConversionWorker.
|
||||
setForeground(getForegroundInfo())
|
||||
|
||||
val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format)
|
||||
Result.success(
|
||||
@@ -129,11 +131,29 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
||||
return publisher.hasSpaceFor(bytes)
|
||||
}
|
||||
|
||||
override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo(
|
||||
NOTIFICATION_ID,
|
||||
notifications.build(id, "Joining files", 0, indeterminate = true),
|
||||
ConversionForegroundType.current(),
|
||||
)
|
||||
/**
|
||||
* The notification a starting join posts, and now the only definition of it.
|
||||
*
|
||||
* WorkManager's hook for expedited work, which **will not call this on any device this app
|
||||
* supports** — see [ConversionWorker.getForegroundInfo] for the measurement and for why
|
||||
* `setExpedited` alone would have left these lines exactly as cold as they were. What makes
|
||||
* them live is [doWork] posting this instead of building its own copy.
|
||||
*
|
||||
* It counts the inputs itself rather than being handed the number, so that it is still answerable
|
||||
* before [doWork] has parsed anything — which is the contract WorkManager's own caller wants.
|
||||
* The count is read from the same key, so the two cannot disagree. The `?: 0` arm is
|
||||
* unreachable and named rather than covered: [doWork] refuses a job with no URI array several
|
||||
* lines above this call, and nothing else calls it. It is the shape `docs/coverage-read-findings.md`
|
||||
* calls F4 — a second line of defence that cannot be provoked.
|
||||
*/
|
||||
override suspend fun getForegroundInfo(): ForegroundInfo {
|
||||
val inputCount = inputData.getStringArray(KEY_INPUT_URIS)?.size ?: 0
|
||||
return ForegroundInfo(
|
||||
NOTIFICATION_ID,
|
||||
notifications.build(id, joiningTitle(inputCount), 0, indeterminate = true),
|
||||
ConversionForegroundType.current(),
|
||||
)
|
||||
}
|
||||
|
||||
companion object {
|
||||
/**
|
||||
@@ -204,6 +224,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
||||
*/
|
||||
fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}"
|
||||
|
||||
/**
|
||||
* What the progress notification says while a join runs.
|
||||
*
|
||||
* Named once, for the convention #158 established about strings the user can see. It was
|
||||
* two strings until 2026-09-06 — `"Joining N files"` built inline in [doWork] and a
|
||||
* countless `"Joining files"` in [getForegroundInfo] — for one notification that only ever
|
||||
* had one job, and the copy nothing executed was free to drift from the one that did.
|
||||
*/
|
||||
fun joiningTitle(inputCount: Int): String = "Joining $inputCount files"
|
||||
|
||||
private const val NOTIFICATION_ID = 1002
|
||||
private const val TAG = "ConcatWorker"
|
||||
|
||||
@@ -215,6 +245,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
||||
*/
|
||||
fun request(inputs: List<Uri>, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) =
|
||||
OneTimeWorkRequestBuilder<ConcatWorker>()
|
||||
// Expedited, exactly as ConversionWorker.request is and for the same reasons; that
|
||||
// one's comment and class KDoc carry them. Nothing here sets an initial delay or a
|
||||
// constraint, which is what makes it legal for `build()` to accept.
|
||||
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
|
||||
.addTag(JobTags.inputCount(inputs.size))
|
||||
.setInputData(
|
||||
Data.Builder()
|
||||
|
||||
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
|
||||
import androidx.work.Data
|
||||
import androidx.work.ForegroundInfo
|
||||
import androidx.work.OneTimeWorkRequestBuilder
|
||||
import androidx.work.OutOfQuotaPolicy
|
||||
import androidx.work.WorkerParameters
|
||||
import androidx.work.hasKeyWithValueOfType
|
||||
import androidx.work.workDataOf
|
||||
@@ -40,8 +41,28 @@ import java.io.File
|
||||
* observe. That durability is what makes the six-hour foreground-service timeout
|
||||
* recoverable instead of fatal.
|
||||
*
|
||||
* Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs
|
||||
* with a short quota, which is the wrong shape for a multi-minute transcode.
|
||||
* Enqueued as **expedited** work, with `RUN_AS_NON_EXPEDITED_WORK_REQUEST`. This paragraph said
|
||||
* the opposite until 2026-09-06 — "deliberately *not* used… the wrong shape for a multi-minute
|
||||
* transcode" — and the quota it named does not reach a transcode the way it reads:
|
||||
*
|
||||
* - The quota belongs to the *JobScheduler* job, and `SystemJobInfoConverter:135` in
|
||||
* work-runtime 2.11.2 sets `JobInfo.setExpedited(true)` only when `!isRetry && !isDelayed`.
|
||||
* A retry is therefore scheduled exactly as every job is scheduled today.
|
||||
* - A job the system stops mid-run does not get its answer from [FailureOutcome].
|
||||
* `WorkerWrapper.interrupt` cancels the worker's coroutine with a `WorkerStoppedException`,
|
||||
* which its `launch` resolves as `ResetWorkerStatus` — the worker's own `Result` is discarded
|
||||
* and the work re-enqueued with backoff, whatever it returned. So a quota stop is a retry, and
|
||||
* the `CancellationException` arm in [doWork] is what deletes the partial on the way through.
|
||||
*
|
||||
* What it buys is narrower than "conversions start sooner", and the narrowness is the honest part:
|
||||
* `GreedyScheduler` starts unconstrained, undelayed work in-process the moment it is enqueued and
|
||||
* carries no `expedited` branch at all, so a conversion begun from the open app runs exactly when
|
||||
* it ran before — the common case does not move. The flag is for the job that has to go *through*
|
||||
* JobScheduler because no process is left to start it: one still enqueued when the app died.
|
||||
* `SystemJobScheduler.schedule` re-converts the spec every time it schedules, so such a job is
|
||||
* expedited on the way back in, and a retried one is not. `RUN_AS_NON_EXPEDITED_WORK_REQUEST`
|
||||
* rather than `DROP_WORK_REQUEST`: an invisible quota is no reason to throw a user's conversion
|
||||
* away, and `SystemJobScheduler:198` degrades it to an ordinary job instead.
|
||||
*
|
||||
* That durability is not free, and the queue surviving is not the same as the job surviving.
|
||||
* When WorkManager recovers a job after process death the app is by definition in the background,
|
||||
@@ -60,7 +81,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
override suspend fun doWork(): Result {
|
||||
val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
|
||||
?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))
|
||||
val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input"
|
||||
val displayName = displayName()
|
||||
// Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what
|
||||
// made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free".
|
||||
val declaredSize = inputData
|
||||
@@ -100,7 +121,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
// process death is. With it above the try that throw escaped doWork() entirely: no
|
||||
// retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below
|
||||
// was outside for the same reason and had the same problem.
|
||||
setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true))
|
||||
//
|
||||
// Posted through getForegroundInfo() rather than built here a second time -- see that
|
||||
// override for what the duplicate cost.
|
||||
setForeground(getForegroundInfo())
|
||||
|
||||
// Through the seam rather than MediaProbe directly. The seam already existed for the
|
||||
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
|
||||
@@ -339,8 +363,32 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
return OutputSpec(container, video, audio)
|
||||
}
|
||||
|
||||
/**
|
||||
* What this job's input is called, or [InputQuery.FALLBACK_DISPLAY_NAME] when nothing named it.
|
||||
*
|
||||
* One read rather than the two copies of `?: "input"` that [doWork] and [getForegroundInfo]
|
||||
* each carried, and against `InputQuery`'s constant rather than a third literal of the same
|
||||
* string: it is the same fallback the picker uses, and it reaches the save dialog as
|
||||
* `input_converted.mp4`.
|
||||
*/
|
||||
private fun displayName(): String = inputData.getString(KEY_DISPLAY_NAME) ?: InputQuery.FALLBACK_DISPLAY_NAME
|
||||
|
||||
/**
|
||||
* The notification a starting conversion posts, and now the only definition of it.
|
||||
*
|
||||
* This is WorkManager's hook for expedited work, and **it will not be called on any device
|
||||
* this app supports.** `WorkForeground.kt:38` in work-runtime 2.11.2 opens with
|
||||
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's
|
||||
* only caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So #252's premise — that
|
||||
* enqueueing expedited work would make these lines live — is false, and `setExpedited` alone
|
||||
* would have left them exactly as cold as the first instrumented coverage read found them.
|
||||
*
|
||||
* What makes them live is [doWork] posting *this* instead of building its own copy. The two
|
||||
* were identical — same title, `percent = 0`, `indeterminate = true` — so one was a duplicate
|
||||
* that could drift, and the one nothing executed is the one that would have drifted silently.
|
||||
*/
|
||||
override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
|
||||
inputData.getString(KEY_DISPLAY_NAME) ?: "input",
|
||||
displayName(),
|
||||
percent = 0,
|
||||
indeterminate = true,
|
||||
)
|
||||
@@ -416,6 +464,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
quality: QualityTier = QualityTier.FAST,
|
||||
enginePreference: EnginePreference = EnginePreference.AUTO,
|
||||
) = OneTimeWorkRequestBuilder<ConversionWorker>()
|
||||
// Expedited, so the jobs that do go through JobScheduler are treated as the
|
||||
// user-initiated work they are -- see the class KDoc for what that is and is not worth.
|
||||
// Safe to set here and only because of what this builder does not do: `build()` refuses
|
||||
// an expedited request carrying an initial delay or any constraint but network and
|
||||
// storage, and none of the three is set below.
|
||||
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
|
||||
.addTag(JobTags.displayName(displayName))
|
||||
// Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has
|
||||
// no null, so the absence of the key *is* the unknown — and a tag reading
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
package org.libremediaconverter.work
|
||||
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.Constraints
|
||||
import androidx.work.OutOfQuotaPolicy
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
|
||||
/**
|
||||
* Both workers enqueue **expedited** work, and stay legal doing it.
|
||||
*
|
||||
* The two questions are separate and only one of them is about the flag.
|
||||
*
|
||||
* - **Is it set.** `expedited` is `false` by default, so `assertTrue` here is what a deleted
|
||||
* `setExpedited(...)` reddens. That mutation was run.
|
||||
* - **Is it legal.** `WorkRequest.Builder.build()` refuses an expedited request that carries an
|
||||
* initial delay or any constraint but network and storage — `require(workSpec.initialDelay <= 0)
|
||||
* { "Expedited jobs cannot be delayed" }` in work-runtime 2.11.2. Neither `request` sets either
|
||||
* today, so both `build()` calls pass and the `IllegalArgumentException` is a *future* hazard
|
||||
* rather than a current one. The delay and constraints assertions below are what name it: add a
|
||||
* delay to either builder and this class fails on the throw, in the same second, instead of the
|
||||
* app failing to enqueue a conversion on a device.
|
||||
*
|
||||
* **The policy assertion bites less than it reads, and that is worth writing down rather than
|
||||
* leaving to be rediscovered.** `WorkSpec.outOfQuotaPolicy` *defaults* to
|
||||
* `RUN_AS_NON_EXPEDITED_WORK_REQUEST`, so it is already this value on a request that was never
|
||||
* expedited at all — deleting `setExpedited` does not redden it. What it does pin is the one
|
||||
* alternative: `DROP_WORK_REQUEST` throws a user's conversion away because an invisible quota ran
|
||||
* out, and that mutation *is* red here.
|
||||
*
|
||||
* The delay is not hypothetical either. Three tests deliberately build a delayed request to hold a
|
||||
* job in `ENQUEUED` — `NotificationCancelActionTest`, `ReattachOnLaunchTest` and
|
||||
* `CancelReachesWorkManagerTest` — and every one of them builds its own
|
||||
* `OneTimeWorkRequestBuilder` rather than adding a delay to what `request` returns. That is why
|
||||
* making these expedited broke none of them; the one that starts from `request` takes only
|
||||
* `base.workSpec.input` from it.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class ExpeditedRequestTest {
|
||||
|
||||
@Test
|
||||
fun `a conversion is enqueued as expedited work`() {
|
||||
val spec = ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec
|
||||
|
||||
assertTrue("a conversion the user asked for has to be expedited work", spec.expedited)
|
||||
assertEquals(
|
||||
"a quota nobody can see is no reason to drop a conversion",
|
||||
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
|
||||
spec.outOfQuotaPolicy,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join is enqueued as expedited work`() {
|
||||
val spec = ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec
|
||||
|
||||
assertTrue("a join the user asked for has to be expedited work", spec.expedited)
|
||||
assertEquals(
|
||||
"a quota nobody can see is no reason to drop a join",
|
||||
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
|
||||
spec.outOfQuotaPolicy,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The two properties that keep `build()` from throwing, asserted on both requests at once
|
||||
* because the rule is WorkManager's rather than either worker's.
|
||||
*/
|
||||
@Test
|
||||
fun `neither expedited request carries what would make it illegal`() {
|
||||
val requests = listOf(
|
||||
ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec,
|
||||
ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec,
|
||||
)
|
||||
|
||||
requests.forEach { spec ->
|
||||
assertEquals(
|
||||
"expedited work cannot be delayed: ${spec.workerClassName}",
|
||||
0L,
|
||||
spec.initialDelay,
|
||||
)
|
||||
assertEquals(
|
||||
"expedited work takes only network and storage constraints: ${spec.workerClassName}",
|
||||
Constraints.NONE,
|
||||
spec.constraints,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
|
||||
val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday2.mp4")
|
||||
const val DISPLAY_NAME = "holiday.mp4"
|
||||
const val INPUT_BYTES = 1_024L
|
||||
const val TOTAL_BYTES = 2_048L
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,203 @@
|
||||
package org.libremediaconverter.work
|
||||
|
||||
import android.app.Application
|
||||
import android.app.Notification
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.Data
|
||||
import androidx.work.testing.TestListenableWorkerBuilder
|
||||
import androidx.work.workDataOf
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.ConcatJoiner
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
import org.libremediaconverter.ffmpeg.ConcatEngine
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.model.DeviceCodecs
|
||||
import org.libremediaconverter.model.EnginePreference
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
import java.util.UUID
|
||||
|
||||
/**
|
||||
* The first thing either worker posts is what its own `getForegroundInfo()` builds.
|
||||
*
|
||||
* **Both overrides were dead code until 2026-09-06, and #252 is where that was found** — the first
|
||||
* instrumented coverage read reported `ConversionWorker:342-346` and `ConcatWorker:132-136` among
|
||||
* the 32 lines *neither* suite reaches. The ticket's premise was that enqueueing expedited work
|
||||
* would make them live, since `getForegroundInfo()` is WorkManager's expedited-work hook.
|
||||
*
|
||||
* **That premise is false at this `minSdk`, which is the finding underneath the fix.**
|
||||
* `WorkForeground.kt:38` in work-runtime 2.11.2 opens `workForeground` with
|
||||
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's only
|
||||
* caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So `setExpedited` alone would have left
|
||||
* both overrides exactly as cold as the read found them, and a test written to drive them through
|
||||
* WorkManager would be testing a code path no device this app supports can take — E1's failure
|
||||
* mode, where a test asserts and never reaches.
|
||||
*
|
||||
* What makes them live is a single-definition change instead. Each worker had **two** definitions
|
||||
* of one notification: the override, and an identical `ForegroundInfo` built inline in `doWork`.
|
||||
* `doWork` now posts the override's, so the copy nothing executed is gone and the one that remains
|
||||
* runs on every job.
|
||||
*
|
||||
* These tests are what hold that wiring. Each asserts the notification's *contents* against
|
||||
* constants rather than against `worker.getForegroundInfo()` — comparing the two would move
|
||||
* together under every mutation and stay green — and the mutations that redden them are named on
|
||||
* each test.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class ForegroundNotificationTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var updater: RecordingForegroundUpdater
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
updater = RecordingForegroundUpdater()
|
||||
ConversionDependencies.publisher = { AlwaysRoomPublisher(app) }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
|
||||
ConversionDependencies.software = { WritingTranscoder }
|
||||
ConversionDependencies.concat = { WritingJoiner }
|
||||
// The notification carries a WorkManager cancel PendingIntent, so without this the worker
|
||||
// fails building the notification rather than on anything these tests are about.
|
||||
installTestWorkManager(app, Data.EMPTY)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* Mutation that must go red, and did: inside `ConversionWorker.getForegroundInfo`, replace
|
||||
* `displayName()` with a literal, or `percent = 0` with anything else. Both are in the override's
|
||||
* own body, so a red here is proof `doWork` executes it rather than a copy of it.
|
||||
*/
|
||||
@Test
|
||||
fun `a conversion's first foreground post is the one getForegroundInfo builds`() {
|
||||
runBlocking { conversionWorker().doWork() }
|
||||
|
||||
val first = updater.infos.first()
|
||||
val extras = first.notification.extras
|
||||
assertEquals(
|
||||
"the notification has to name the file the user picked",
|
||||
DISPLAY_NAME,
|
||||
extras.getString(Notification.EXTRA_TITLE),
|
||||
)
|
||||
assertEquals("a conversion starts at zero", 0, extras.getInt(Notification.EXTRA_PROGRESS))
|
||||
assertTrue(
|
||||
"nothing is known about the length of the job yet, so the bar is indeterminate",
|
||||
extras.getBoolean(Notification.EXTRA_PROGRESS_INDETERMINATE),
|
||||
)
|
||||
assertEquals(
|
||||
"the foreground service type is the regime's, not zero",
|
||||
ConversionForegroundType.current(),
|
||||
first.foregroundServiceType,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The count is the point.
|
||||
*
|
||||
* `getForegroundInfo` said `"Joining files"` and `doWork` said `"Joining N files"` — one
|
||||
* notification with two texts, and the one nothing ran was free to drift. Now there is one,
|
||||
* and it reads the input array itself so it can still answer before `doWork` has parsed
|
||||
* anything.
|
||||
*
|
||||
* Mutation that must go red, and did: replace the array read in `ConcatWorker.getForegroundInfo`
|
||||
* with a constant `0`, which yields `"Joining 0 files"`. Asserting merely that the title starts
|
||||
* with "Joining" would survive that, which is why the whole string is pinned.
|
||||
*/
|
||||
@Test
|
||||
fun `a join's first foreground post counts the files it was given`() {
|
||||
runBlocking { joinWorker().doWork() }
|
||||
|
||||
val first = updater.infos.first()
|
||||
assertEquals(
|
||||
"the notification has to say how many files are being joined",
|
||||
ConcatWorker.joiningTitle(INPUTS.size),
|
||||
first.notification.extras.getString(Notification.EXTRA_TITLE),
|
||||
)
|
||||
assertEquals(
|
||||
"the foreground service type is the regime's, not zero",
|
||||
ConversionForegroundType.current(),
|
||||
first.foregroundServiceType,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* And the title is really the file's name rather than any string at all.
|
||||
*
|
||||
* [ConcatWorker.joiningTitle] is asserted above through the constant the worker itself uses, so
|
||||
* that assertion cannot catch the sentence being reworded — deliberately, since the wording is
|
||||
* not what the test is about. This one can: two files, two names, one worker each.
|
||||
*/
|
||||
@Test
|
||||
fun `two conversions of differently named files post differently named notifications`() {
|
||||
runBlocking { conversionWorker(displayName = OTHER_NAME).doWork() }
|
||||
|
||||
assertEquals(
|
||||
OTHER_NAME,
|
||||
updater.infos.first().notification.extras.getString(Notification.EXTRA_TITLE),
|
||||
)
|
||||
}
|
||||
|
||||
private fun conversionWorker(displayName: String = DISPLAY_NAME) = TestListenableWorkerBuilder<ConversionWorker>(
|
||||
context = app,
|
||||
inputData = workDataOf(
|
||||
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
|
||||
ConversionWorker.KEY_DISPLAY_NAME to displayName,
|
||||
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
|
||||
// FORCE_SOFTWARE is the one preference that decides without consulting the input,
|
||||
// and a file:// URI keeps the worker out of FFmpegKit's native SAF bridge.
|
||||
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
|
||||
),
|
||||
runAttemptCount = 0,
|
||||
).setId(JOB_ID)
|
||||
.setForegroundUpdater(updater)
|
||||
.build()
|
||||
|
||||
private fun joinWorker() = TestListenableWorkerBuilder<ConcatWorker>(
|
||||
context = app,
|
||||
inputData = workDataOf(
|
||||
ConcatWorker.KEY_INPUT_URIS to INPUTS.map(Uri::toString).toTypedArray(),
|
||||
ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES,
|
||||
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name,
|
||||
),
|
||||
runAttemptCount = 0,
|
||||
).setId(JOB_ID)
|
||||
.setForegroundUpdater(updater)
|
||||
.build()
|
||||
|
||||
private companion object {
|
||||
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
|
||||
val INPUTS: List<Uri> = listOf(INPUT, Uri.parse("file:///tmp/holiday2.mp4"))
|
||||
const val DISPLAY_NAME = "holiday.mp4"
|
||||
const val OTHER_NAME = "birthday.mkv"
|
||||
const val INPUT_BYTES = 1_024L
|
||||
const val TOTAL_BYTES = 2_048L
|
||||
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000252")
|
||||
}
|
||||
}
|
||||
|
||||
/** A joiner that writes an output and reports a strategy; nothing here is about the engine. */
|
||||
private object WritingJoiner : ConcatJoiner {
|
||||
override suspend fun join(inputs: List<Uri>, output: File, format: OutputFormat): ConcatEngine.Result {
|
||||
output.writeBytes(ByteArray(OUTPUT_BYTES))
|
||||
return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output)
|
||||
}
|
||||
|
||||
private const val OUTPUT_BYTES = 512
|
||||
}
|
||||
@@ -3,16 +3,12 @@ package org.libremediaconverter.work
|
||||
import android.app.Application
|
||||
import android.app.Notification
|
||||
import android.app.NotificationManager
|
||||
import android.content.Context
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.Data
|
||||
import androidx.work.ForegroundInfo
|
||||
import androidx.work.WorkInfo
|
||||
import androidx.work.testing.TestForegroundUpdater
|
||||
import androidx.work.testing.TestListenableWorkerBuilder
|
||||
import androidx.work.workDataOf
|
||||
import com.google.common.util.concurrent.ListenableFuture
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
@@ -224,26 +220,6 @@ class ProgressNotificationTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
|
||||
*
|
||||
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
|
||||
* worker awaits what this returns, so a future that never completes would hang the initial
|
||||
* `setForeground` rather than test anything.
|
||||
*/
|
||||
private class RecordingForegroundUpdater : TestForegroundUpdater() {
|
||||
val infos = mutableListOf<ForegroundInfo>()
|
||||
|
||||
override fun setForegroundAsync(
|
||||
context: Context,
|
||||
id: UUID,
|
||||
foregroundInfo: ForegroundInfo,
|
||||
): ListenableFuture<Void> {
|
||||
infos += foregroundInfo
|
||||
return super.setForegroundAsync(context, id, foregroundInfo)
|
||||
}
|
||||
}
|
||||
|
||||
/** An engine that reports whatever [report] wants reported, then writes an output. */
|
||||
private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder {
|
||||
override suspend fun run(
|
||||
|
||||
@@ -1,11 +1,14 @@
|
||||
package org.libremediaconverter.work
|
||||
|
||||
import android.content.Context
|
||||
import androidx.work.ForegroundInfo
|
||||
import androidx.work.testing.TestForegroundUpdater
|
||||
import com.google.common.util.concurrent.ListenableFuture
|
||||
import org.libremediaconverter.convert.OutputPublisher
|
||||
import org.libremediaconverter.convert.SoftwareTranscoder
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
import java.io.File
|
||||
import java.util.UUID
|
||||
import java.util.concurrent.ExecutionException
|
||||
import java.util.concurrent.Executor
|
||||
import java.util.concurrent.TimeUnit
|
||||
@@ -94,3 +97,27 @@ internal class FailedFuture(private val failure: Throwable) : ListenableFuture<V
|
||||
override fun get(): Void = throw ExecutionException(failure)
|
||||
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
|
||||
}
|
||||
|
||||
/**
|
||||
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
|
||||
*
|
||||
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
|
||||
* worker awaits what this returns, so a future that never completes would hang the initial
|
||||
* `setForeground` rather than test anything.
|
||||
*
|
||||
* Shared scaffolding since #252 moved it here out of `ProgressNotificationTest`, which asks what a
|
||||
* *running* worker publishes; `ForegroundNotificationTest` asks what its *first* post is, and both
|
||||
* questions need the same recorder. `infos.first()` is that first post in either.
|
||||
*/
|
||||
internal class RecordingForegroundUpdater : TestForegroundUpdater() {
|
||||
val infos = mutableListOf<ForegroundInfo>()
|
||||
|
||||
override fun setForegroundAsync(
|
||||
context: Context,
|
||||
id: UUID,
|
||||
foregroundInfo: ForegroundInfo,
|
||||
): ListenableFuture<Void> {
|
||||
infos += foregroundInfo
|
||||
return super.setForegroundAsync(context, id, foregroundInfo)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -403,6 +403,27 @@ They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWo
|
||||
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
|
||||
`setExpedited`.
|
||||
|
||||
**Updated 2026-09-06 (#252, and the sentence above is half wrong).** "WorkManager calls
|
||||
`getForegroundInfoAsync()` only for expedited work" is true and *not sufficient*, and the missing
|
||||
half is what made the reopening trigger wrong. `WorkForeground.kt:38` in work-runtime 2.11.2 opens
|
||||
the library's only caller with
|
||||
|
||||
```kotlin
|
||||
if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return
|
||||
```
|
||||
|
||||
and `minSdk` is 33. So calling `setExpedited` reopens nothing: on **every** device this app
|
||||
supports, WorkManager does not consult `getForegroundInfo()` whether the work is expedited or not.
|
||||
#252 was filed on the trigger as this entry stated it, and its acceptance criterion — "a request
|
||||
now carries `setExpedited` and the existing worker tests drive them" — cannot be met that way.
|
||||
|
||||
What made the lines live instead was that each worker held **two** definitions of one notification:
|
||||
the override, and an identical `ForegroundInfo` built inline in `doWork`. `doWork` now posts the
|
||||
override's, so the duplicate is gone and what remains runs on every job. The general lesson is the
|
||||
one E1 states from the other side: *check that the mechanism you are relying on actually fires on
|
||||
the machine that runs it* — here the mechanism was a library early-return two source lines long,
|
||||
and four waves of reading had taken the API summary's word for it.
|
||||
|
||||
---
|
||||
|
||||
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
|
||||
@@ -452,7 +473,7 @@ the cheaper order.
|
||||
| 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 |
|
||||
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **closed 2026-09-06 by #252** — and its stated reopening trigger was wrong; see the update on the entry |
|
||||
| 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
|
||||
|
||||
@@ -495,7 +495,7 @@ 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** |
|
||||
| 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 — closed 2026-09-06, and not the way this row expects.** Expedited work is now enqueued, but that is *not* what covers these lines: `WorkForeground.kt:38` returns before the hook whenever `SDK_INT >= 31`, and `minSdk` is 33. What covers them is `doWork` posting the override instead of a second copy of the same notification. See `coverage-read-findings.md` F9's update |
|
||||
| 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 |
|
||||
|
||||
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