#142 -- openOutputStream refuses two ways and only one was reachable. A
provider that has gone away throws from inside the call, which
`a destination the provider will not open...` already drives. A provider
that is present and declines returns null, and nothing could produce that
on demand. openDestination is the seam; the test asserts the failure names
the destination, which is what separates the `?: error(...)` from an NPE
inside `use`.
#143 -- the sweep's re-read. **The seam the ticket proposed does not reach
it.** Overriding the listing fires before the entries are snapshotted, so
StagingSweep.collectable is handed the new timestamp, the file is never
proposed for deletion, and the guard is never exercised. Measured: with an
entriesIn seam, deleting the guard outright left the test green.
The race is a file that *was* collectable when the snapshot was taken and
is not by the time the delete comes round, so the seam has to sit at the
snapshot. `snapshot(listing)` does, and deleting the guard now reddens the
test.
Three mutations after the move, three red:
null stream returns silently null-return test
null stream via !! instead null-return test
sweep deletes unconditionally race test
OutputPublisher.kt now has no never-executed lines at all. Two partial
branches are left and both are named exemptions rather than gaps:
L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are
the failure arms of a runCatching whose body cannot be made to throw
through any public entry point -- the same shape as the `size >= 0`
exemption recorded in the previous commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`the sweep tolerates a staging path that is not a directory` failed once on run
33069641674, against 468 tests that pass on this machine including under
`--rerun-tasks`:
java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112
468 tests completed, 1 failed
Line 112 was `writeBytes` immediately after `deleteRecursively()`.
`FileOutputStream` answers `FileNotFoundException` for an existing directory, so
something had recreated the path inside that window. That something is
`LibreMediaConverterApp.onCreate`, which ends with
appScope.launch { OutputPublisher(...).sweepStaging() }
on `Dispatchers.IO`, and `sweepStaging` reads `stagingDir`, whose getter calls
`mkdirs()`. Robolectric builds the application for every test that asks for one,
so that background `mkdirs()` is in flight across the whole suite on a thread the
paused main looper does not control and no test awaits.
Retrying closes the window rather than narrowing it, because the race is not
symmetric: `mkdirs()` fails on an existing regular file, so the invariant only has
to survive being *established*. Once a write lands, nothing in the suite can turn
this path back into a directory -- which is also why the new assertion that the
sweep left a file behind is worth making.
The `check()` matters as much as the loop. The next failure here should say
"something recreated conversions/ as a directory", not `FileNotFoundException at
line 112` -- that is the difference between a flake someone reads and a flake
someone re-runs.
The wider problem is #159 and is deliberately not fixed here: `AppStartSweepTest`,
`JobSnapshotsTest` and `SpaceArithmeticTest` all name the same path, and the real
answer is an injectable scope rather than a retry loop in every staging test.
#159's done-when is that this loop can be deleted.
Mutation: `listFiles() ?: return` -> `listFiles()!!` reddens exactly this test.
Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
destinationIsKnownEmpty's three short-circuits -- no SIZE column, no row,
a null cell -- each had to answer false and none was tested. Its KDoc is
unambiguous about why: "this decides whether a delete is allowed and 'I
could not tell' must never authorise one." The existing tests only ever
drove a provider that answers properly, where the answer is zero and the
delete is correct. Getting the uncertain cases backwards costs the user a
file they already had, on a save that failed.
Also discardStaged's null parentFile, and sweepStaging's null listing --
which is not the case the existing `tolerates a staging directory that
does not exist yet` covers, because stagingDir's own mkdirs() recreates a
missing directory and it then lists as empty. Only a path that cannot be
a directory makes listFiles() answer null.
Five mutations, three bite:
drop !row.isNull(size) short-circuit test red
drop row.moveToFirst() five tests red
parentFile!! instead of ?: return false parentless test red
size >= 0 -> size >= -1 GREEN, does not bite
parentless treated as staged GREEN -- bad mutation, see below
The first green one is recorded in the test as a named exemption.
Measured: getColumnIndex returns -1 for an absent column and isNull(-1)
throws CursorIndexOutOfBoundsException, which the surrounding runCatching
already turns into `?: false`. Same answer, reached by the exception path,
so no behavioural test can pin that conjunct. It stays anyway -- control
flow through an exception is worse than a comparison, and another Cursor
implementation need not throw.
The second was my mistake rather than a finding: substituting stagingDir
for the null parent reaches `return false` by a different route, so it
proves nothing. parentFile!! is the honest mutation and it goes red.
:197, :235 and :258 are now covered. What is left in this file is exactly
what the ticket scoped out: :173-174 (#142) and :267 (#143).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing had ever handed InputQuery a cursor row. UnknownInputSizeTest
drives the no-provider case thoroughly -- query returns null, measure()
answers -- so firstRow's body, displayNameOrNull and sizeOrNull had never
executed at all.
Nine tests over FakeSafProvider's RowShape states. What they pin is not
"reads a cursor" but the rule the class exists for: a size nobody could
determine must arrive as null, never 0. Four separate ways a provider
fails to give one -- a null cell, a missing column, a negative value, an
empty cursor -- plus a provider that throws outright, which is the guard
firstRow's KDoc is written for.
Mutations run, all four bite:
drop `takeIf { it >= 0 }` from sizeOrNull -> negative-size test red
drop `!isNull(it)` from sizeOrNull -> null-size test red
drop the runCatching in firstRow -> throwing-provider test red
drop `!isNull(it)` from displayNameOrNull -> GREEN, does not bite
That last one is recorded in the test's KDoc as a named exemption rather
than papered over. Measured: MatrixCursor.getString on a null cell returns
null while getLong returns 0. So the guard is load-bearing on the size path
-- it is what stops a null becoming a real number -- and unfalsifiable on
the name path, where getString already yields null. It stays regardless:
Cursor.getString's contract makes throwing on null implementation-defined,
and a real provider may do what MatrixCursor does not.
InputQuery.kt now has no never-executed lines. Suite 456 -> 465 tests,
branch coverage 63.8% -> 65.6%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of #132's items are cursor-shaped -- InputQuery's row reads (#137) and
OutputPublisher.destinationIsKnownEmpty's short-circuits (#140) -- and the
provider that could drive them lived inside OutputPublisherPublishTest and
could only answer correctly. Its row was always (file.name, file.length()).
Moved FakeSafProvider, FakePlainProvider and the registration helper to
FakeProviders.kt, same package, following StagingCleanupSupport.kt and
ParkedPickDispatcher.kt. UnreliableOutputStream stays behind: it serves one
test, which is the line WorkerStubs.kt draws.
Added RowShape, seven ways a provider can answer a metadata query. Column
granularity is deliberate -- OutputPublisher reads only SIZE, InputQuery
reads both and reaches different answers depending on which is bad -- and so
is keeping null, missing, negative and no-row distinct rather than folding
them into one "bad" case. That distinction is the whole reason InputQuery
exists: hasSpaceFor(0) is only "is there 128 MB free", so a size nobody
could determine must not arrive as 0.
No production change. OutputPublisherPublishTest, OutputPublisherStagingTest
and UnknownInputSizeTest pass unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A gap audit over the eight branches merged together looked for lines still
never executed and asked, for each, whether something already accounts for it.
Everything mapped except one: `ConcatWorker.kt:42`, the refusal of a join with
fewer than two inputs.
Its neighbour maps. `ConcatWorker.kt:40` -- the missing-URI-array arm, two lines
above -- is covered on the device by
`UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. That is invisible to
JaCoCo, which measures `testDebugUnitTest` only, so the report shows both arms
cold and cannot distinguish the one that is e2e-covered from the one nothing
touches. Only reading the androidTest source separates them.
`grep` says nothing in either source set mentions "Pick at least two files to
join." Two tests here now do:
- `a join of a single file is refused with a message rather than joined` pins
the verdict and the message together, via `Failure.equals`, for the reason the
file's header already gives.
- `a join of two files is not refused for its count` is the control that puts
the assertion on the boundary rather than on the string. It refuses the
*space* rather than letting the job run: the next thing past the count guard
is `ConcatEngine`, which is native, and `NamingPublisher`'s KDoc already
records that no JVM test gets past it. A failure carrying the space message is
proof execution reached line 57, which is proof it cleared line 42, at no
engine cost.
Reachability is the header's argument plus one of its own: `request(...)` takes
a `List<Uri>` and checks nothing about its length, so a one-item join is a
well-formed call rather than a corrupted queue entry.
Mutations, each killing exactly the test it should:
| mutation | red |
|---|---|
| guard deleted outright | `a join of a single file is refused...` |
| `uris.size < 2` -> `< 3` | `a join of two files is not refused for its count` |
Restored, both green. Gate green: assembleDebug, testDebugUnitTest,
compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both exits were cold, and both are reachable for the same reason: a job
does not have to come from the picker. WorkManager keeps work for about a
week, so a downgrade or rollback hands this build a job enqueued by
another one, and request(...) is callable directly.
:62 -- a missing KEY_INPUT_URI -- was untested everywhere, JVM and device.
The nearest e2e test, ForcedFailureTest.aMissingInputFailsRatherThanCrashing,
passes a URI pointing at a file that does not exist, which reaches the
engine and fails much later with a different message.
:124-126 -- the Validation.Invalid refusal -- had no test at all, though
its comment names both arrival paths it exists for.
Five tests, in a new file because both are about the *message*. A refusal
that fails with empty output Data renders the UI's generic "Conversion
failed." with nothing else to say, which is the defect shape
DeniedForegroundStartTest records from the device pass; asserting the
verdict alone would pass against exactly that.
Two of the five are there to stop the others passing for the wrong reason:
`a refused spec never reaches an engine` says it failed *before*
converting rather than during, and `a valid spec is not refused` is the
control -- without it every assertion here would still pass against a
worker that refused everything.
Three mutations, three red:
change the no-input message no-input message test
drop the validation refusal both refusal tests
validate but keep converting both refusal tests
L61-62 and L123-126 are now fully covered, branches included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ConversionWorker has WorkerCancellationTest and DeniedForegroundStartTest.
Its twin had the retry case only -- `a join whose foreground start is
denied` already existed -- so two of ConcatWorker's three failure exits
were cold: the CancellationException arm, and FOREGROUND_DENIED.
Four tests, added to the files that own each rule rather than to a new
ConcatWorker file, which is how this suite is organised: a file per rule,
tested across both workers.
The cancellation seam is worth a look in review. The conversion twin
cancels inside the engine, which is honest there because
ConversionDependencies has a seam for it. ConcatWorker calls ConcatEngine
directly and has none -- it is native and nothing here gets past it -- so
the cancellation is injected at the only other point inside the try,
setForeground. That is a real shape rather than a contrivance: a job
cancelled while WorkManager is promoting it is exactly when that window is
open, and the catch arm cannot tell where in the try it came from.
FailedFuture moved to WorkerStubs.kt on the way. Two tests now inject two
different failures through it, and Kotlin will not take two file-private
top-level classes of one name in one package.
Four mutations, four red, each isolated:
cancellation arm -> Result.failure propagation test only
drop delete on cancellation cancellation-partial test only
FOREGROUND_DENIED -> Result.retry past-the-bound test only
drop delete on the Throwable path give-up-partial test only
ConcatWorker's :92, :95-96 and :105-106 are covered; missed branches 4 -> 3.
What is left is what the ticket scoped out: the two input guards (e2e), the
ConcatEngine success path (native), and getForegroundInfo (#88's named
exemption).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two halves of ContainerCapabilities.validate were written together
and only one of them was ever checked. Six audio outcomes had no test --
every one a string the user reads -- while the video twin of each was
already covered.
Seven tests, deliberately shaped like their twins rather than as a fresh
idea about what to assert:
unidentifiable source audio on a COPY twin of `an unidentifiable
source codec cannot be copied`
container cannot hold the copied source twin of `a codec the container
cannot hold is refused...`
container cannot carry it on encode twin of `H265 in AVI is refused`
this app cannot encode it twin of `copying is offered as
the fix when...`
accepts(_, AudioCodec.NONE, _) -> true twin of the VideoCodec.NONE arm
accepts(_, AudioCodec.COPY, _) throws twin of `resolving COPY before
asking the matrix is required`
The seventh is not the audio axis: validateVideo's copy-into-a-container-
that-cannot-hold-it refusal was the one video outcome with no test, and it
is the same shape and the same file.
Each asserts the message verbatim and re-validates every suggestion the
refusal offers. Validation.Invalid promises its suggestions are themselves
valid and names this class as the proof; the existing property test walks
the presets, and no preset reaches suggestions() through validateAudio.
Seven mutations run, seven red, each isolated to exactly one test:
CARRIES_AUDIO check -> false encode-path test only
drop the COPY error arm resolve-first test only
AudioCodec.NONE -> false no-audio-track test only
drop ENCODABLE_AUDIO check unencodable test only
drop audio copy container check audio-copy test only
drop video copy container check video-copy test only
drop unidentified-audio guard unidentifiable test only
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WorkerEnumFallbackTest already existed for this defect class -- a name
this build does not define, read above the try, throwing out of doWork
entirely: FAILED with reschedule=false, empty output Data so the screen
said "Conversion failed." with nothing else, and the staged file never
deleted. It covered 2 of the 5 above-the-try reads. readSpec's three
were the ones left, and all three were cold.
The baseline is the part worth reviewing. readSpec returns the *entire*
fallback spec the moment any one axis fails to resolve, so a test
starting from MP4_H265 -- which is itself the fallback -- cannot tell a
worker that read the spec correctly from one that gave up on it. These
start from MKV/H.264, which differs on container and video codec at
once, and assert the spec that actually reached the transcoder rather
than only that a Result came back.
Mutations run, four for three tests:
KEY_CONTAINER `?: return fallback` -> `?: error(...)` -> container test red
KEY_VIDEO_CODEC same -> video test red
KEY_AUDIO_CODEC same -> audio test red
fallback = MP4_H264 instead of MP4_H265 -> all three red
The first three confirm the tests are isolated to their own axis; the
fourth confirms they pin *which* spec ran, which is what "a Result at
all" would have missed.
readSpec is now fully covered, branches included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment described adopting a later build's worker as an edge case. It is the
ordinary CI shape: the worker is found by scanning this daemon's descendants for
GradleWorkerMain, which cannot tell one invocation from the next, and the Unit
tests job runs testDebugUnitTest and jacocoTestReport back to back against one
daemon. Still harmless -- the watchdog only reads and writes -- but a reader
should not have to rediscover that.
Refs #125.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JVM suite had no timeout of any kind, so #125's Room/WorkManager lock-order
inversion ran until something outside it gave up: 47 minutes locally, and on CI
it would burn the Unit tests job's 30-minute cap and report as a job timeout
with no cause. The deadlock is monitor contention, which no interrupt breaks, so
nothing inside the JVM could have ended it either.
The obvious fix does not work here. A JUnit `Timeout` -- as a rule or as
`@Test(timeout = ...)` -- runs the test body on a separate thread, and every Compose test in this
source set goes through Robolectric's paused main looper. Both forms fail with
"main looper can only be controlled from main thread"; the same tests with the
timeout removed pass, so it is the mechanism and not the probe.
So the bound comes from outside the test JVM, where it moves no threads:
`timeout` on the Test tasks kills the forked worker, and a watchdog jstacks that
worker two minutes earlier. The jstack is the point. Gradle's timeout on its own
kills silently, a timed-out run writes no XML for the class that hung, and the
JVM's own "Found one Java-level deadlock" section naming both monitors is the
only reason #125 could be described at all -- so it goes to stdout as well as to
a file, because the Unit tests job uploads only reports/tests/.
Ten minutes is against the slowest observed passing run, not the typical one:
eight CI samples of the whole invocation ranged 62-90s, so this is ~6.7x that
and a third of the job cap. A timeout that fires on a healthy slow runner turns
a real signal into noise.
Both numbers live in a build script that nothing compiles, so HangBoundTest
reads them back and the build script joins build.yml as a declared input --
without that the guard would go stale on exactly the edit it exists to catch.
Refs #125.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two accuracy fixes to notes the earlier commits left behind.
The test KDocs carried "roughly 1-in-130" and a 400-leg-attempt denominator.
Both come from earlier comments on #49 that its own census later replaced --
that ticket has three recorded corrections to its rate claims, and a
superseded figure in a permanent comment is the exact thing its author kept
having to fix. What survives the corrections is the count and the spread:
four occurrences, API 33, 35 and 36, every one on attempt 1 and green on
re-run.
The save exemption described a real defect with nowhere to look it up. It is
#123 now, so the KDoc names a number instead of trailing off.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note claimed `save` is left unguarded because nothing can overwrite what
it writes. That half is true -- the only observation that could belongs to a
job already in a terminal state. The other half was missing: a save whose
copy is still in flight when the user taps Start over lands `Saved` on a
screen they have just cleared.
Guarding it would drop that write instead, which reports nothing for a file
that may genuinely have reached the destination. That is a question about
what the screen should offer during a save, and answering it in a race fix
would be deciding it by accident.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`reattach()` read `_state.value`, found it `Idle`, and then handed the job
to `observe()` -- which launches a *separate* coroutine that cannot write
until its `collect` has resumed with a `WorkInfo`. So the check happened at
one moment and the write landed at another, with a whole pick able to fit
in between: the user tapped, their metadata query suspended, the guard saw
an empty screen, and the finished job from an earlier session wrote over
`Ready(picked)` a moment later.
The comment above that guard said "no suspension point between this check
and the assignment below, so nothing can interleave". There is no
assignment below, and the two lines are in different coroutines. That
sentence is why this sat as flaky CI for two days rather than being read as
the product race it is.
`ScreenOwnership` makes the answer the test already encodes -- the user's
pick wins -- true rather than probable. A claim is taken synchronously when
the user acts; every write that lands after a suspension point checks the
claim it was made under and drops itself if that claim has been superseded.
Dropped, not reordered: a write that is dropped cannot come back later.
Cancelling the superseded observer was never enough on its own. `Job.cancel`
is honoured at the next suspension point, and a collector that has already
resumed and is on its way to `_state.value = ...` has none left; the write
lands anyway. It also cannot help at all in the case reported, where nothing
supersedes the observation until after it has been launched.
`JoinViewModel` had the identical shape and nothing watching it, so it gets
the same fix and the counterpart test that was missing. Its pick dispatcher
becomes injectable for the same reason `ConversionViewModel`'s already was:
without that seam there is no way to ask what happens while a pick is still
in flight.
Closes#49
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refusing "copy the video" for a file that has none built its one suggestion by
hand — drop the video track and leave everything else alone. That is valid only
when the audio axis already happened to be fine. For any audio the target cannot
carry (Vorbis or PCM into MP4, MP3 into WebM) the offer is refused in the next
breath, so the Advanced picker showed a one-tap fix leading straight to a second
error. Nothing unsafe shipped — ConversionWorker re-validates — but it is a dead
end, and it contradicted the promise Validation.Invalid makes in its own KDoc.
Route it through the shared repair-and-filter path instead, as every other branch
does. Excluding what the *user* asked for rather than the already-repaired spec
is what keeps the case that worked working: an MP3 into MP4 still gets its copy
offered, because the repair of a copyable track is that same copy.
Only a branch that builds its own list can break that promise at all, since
suggestions() ends by filtering on validate().isValid. The property test now
covers both of them — this one and the image output — rather than reaching them
by luck, which is how a dead-end chip survived two earlier widenings of it. Its
failures name the probe too: three rows share a spec and differ only in the input.
Closes#114
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The KDoc claimed dynamic colour "stays switchable so users can opt back to the
brand palette". Nothing switches it: MainActivity is the only caller and passes
no arguments, so dynamicColor is always true and the two brand-palette branches
are dead. A reader who trusted that sentence would go looking for a setting that
has never existed.
Replace the claim with what is true today and point at #68, which holds the
decision -- add a switch, delete the dead branches along with the template
palette, or replace that palette first. None of the three is taken here.
ThemeKt had no test, so nothing would have caught the branches being swapped
either. Assert what the theme resolves by reading MaterialTheme.colorScheme
inside the content lambda: the two live branches on background luminance, which
is the one thing two schemes off the same device palette do not share, and the
dead pair by passing dynamicColor explicitly. Both KDocs say plainly that the
test is the only thing that passes it, so the coverage is not misread as
evidence a switch exists -- which is the misreading #68 exists to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
save()'s onFailure keeps the staged file on purpose -- it can be the only copy
of an hour of transcoding, and the destination did not receive it -- and then
handed the screen a Failed carrying a message and nothing else. That branch
rendered exactly one control: "Start over", wired to reset(), which discards
precisely the file the comment above it goes out of its way to keep. The intent
was already written down in main; the UI did not honour it, and the only rescue
was process death followed by reattach -- unadvertised, and bounded by a sweep
that collects anything a day old.
Failed now carries a PendingSave, and only where the failure came from save().
A transcode that died staged nothing and must not sprout a save button, so the
handle is nullable and the observe() arm leaves it null; so does a save that
found the file already gone. The branch renders "Try saving again" above "Start
over", opening the same CreateDocument flow with the same name and type the
first attempt used. A retry that fails again lands back on a carrying Failed
rather than a bare one, so the second failure cannot eat what the first kept.
Start over still deletes from there, and that is a decision rather than an
inheritance: deletion is the user's choice only once the alternative has been
offered. pendingStaged remains the single owner of the delete, so the carried
handle is a view of it rather than a second owner and no path out of the state
can drop a file the old shape could not.
pendingSave() exists so save() and each screen's CreateDocument registration
answer "what would a save target" once instead of twice -- the entry points
cast to Converted/Joined, which answered null for a Failed and fell back to the
current pickers, wrong for any spec edited since the job ran and for every
reattached job.
Both tabs, since JoinViewModel and JoinScreen have the same shape.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
transcode() posts its work to a HandlerThread, and everything on that
thread has no caller to throw back to: an escaping exception reaches the
thread's uncaught handler and takes the process down, while the
continuation is never resumed. Both halves of that are bad, and the
second is arguably worse — a worker left suspended forever holds a
foreground service.
The guarding was two narrow runCatching blocks, one around
buildTransformer and one around transformer.start, with the two Media3
builders sitting unguarded between them. That gap was not theoretical.
EditedMediaItem.Builder rejects a composition with both tracks removed,
which is exactly what a plan of (Drop, Drop) asks for, and it does so
with a plain IllegalStateException from the constructor.
Validation now refuses the spec that produces such a plan, so neither
the picker nor ConversionWorker will start one. Routing is a separate
question and still answers Media3 for it — a dropped track makes nothing
un-hardware-able — so a request that skips validation still arrives
here: a job queued before the settings changed, or one made through
ConversionWorker.request directly. CopyPlanner's own KDoc already names
that path as the reason it re-checks what validation has checked; this
is the same belt for the same braces.
One guard around the whole body costs nothing on success and turns any
such refusal into a failed job with a reason attached. The export body
moves into startExport, whose contract is the thing that makes one guard
enough: returning normally means the export is running and the listener
owns the continuation, throwing means it never started and the caller
does. Cancellation is still registered before start.
Covered twice on purpose. Robolectric runs the real HandlerThread and
the real Media3 builders, so the JVM test exercises the whole sequence
and can be run anywhere; the instrumented one repeats it against the
real framework. Neither asserts only that the failure is an
IllegalStateException, because withTimeout raises
TimeoutCancellationException and java.util.concurrent.CancellationException
extends IllegalStateException — so that assertion alone calls an
unresumed continuation a pass. Both were written that way first, and
reverting the guard is what exposed it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Validation already refused two ways of asking for an empty file: None on
both codec axes, and Copy for a video track the input does not have. It
missed the third, because it read only the spec. Name H.265 with the
audio off, hand it an MP3, and the spec looks fine — it names a video
codec — while CopyPlanner drops that track anyway, because the *input*
has no video to encode. The plan is (Drop, Drop), the router still says
Media3, and EditedMediaItem.Builder refuses to build a composition with
both tracks removed. It refuses it on Transformer's own HandlerThread,
where the user sees the app die rather than a reason.
Asking the probe as well as the spec catches all three faces with one
guard, and the equivalence is exact rather than approximate: CopyPlanner
drops video for None or for an input with none, and audio for None, so
"(Drop, Drop)" and this condition are the same set. A sweep over every
non-image container by codec by codec against both probes asserts that,
so a new container or codec cannot reopen the gap on an axis nobody
wrote a case for.
This newly refuses a combination the Advanced picker accepts today, and
that is the point: today it crashes. What it must not do is refuse
without a way out. The Copy face had one only nominally — its single
hand-built suggestion was None + None, which validation rejects in the
next breath, so the one-tap fix fixed nothing. All three faces now go
through the shared repair-and-filter path, which for an MP3 into MP4
offers "copy the audio across" and nothing that has to be re-refused.
Repair is also stopped from naming a video codec for a file with no
video track. It used to fall through to the first codec the container
could encode, so the fix offered for an MP3 was "H.264" — a codec
CopyPlanner then drops, making the offer a fiction that happened to
validate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"A comparison was asked for" and "a number was found to compare against" were one
variable, and collapsing them put the report one refactor away from being the
thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored
at the line start, so indenting the const into an object -- or renaming it, or
moving it -- empties it, and the old code then skipped the whole comparison while
the table kept printing exactly as before. Silent, and indistinguishable from a
run that matched.
Now an unreadable baseline is itself a deviation, with the notice naming the
const so the fix is obvious. Verified against the real captured log of run
32865281555 three ways: baseline file absent, const indented into an object, and
the committed file unchanged -- the first two announce, the third stays silent.
That job is continue-on-error and red on every PR by design, which CLAUDE.md
states plainly -- and that instruction is exactly why nobody reads it. Nothing in
a red X separates "the known three" from "the known three plus yours".
A bare failure count would not have fixed it, and this is measured rather than
assumed. The run is usually truncated: seven of eight advisory runs read on
2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.`
with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run
misleads in both directions -- a fourth marked test can still yield the same
number if the abort lands earlier, and the known set getting worse can lower it.
The test XML does not rescue it either, which was the thing worth checking before
building on it: it IS written for an aborted run, and it reports a tidy
tests="3" failures="3" for a run the runner had just described as truncated. So
the XML is the authority on how many results landed, the runner's own output is
the only authority on whether the run finished, and the report reads both and
says which number came from where.
The baseline is one number beside the marker, because the marker means "cannot
pass on this image": the count is both how many tests the advisory leg runs and
how many should fail. A smaller failure count is the interesting direction -- it
means one now passes, which is the documented trigger for deleting the
annotation.
Nothing about the job's status changes. It stays continue-on-error, stays red,
stays out of the required contexts; a deviation is a ::notice::, never an
::error::. The report is a separate script so it can be run against a real log
saved from a real CI run, which is how the comparison was shown to fire.
The gating legs get the shape without the comparison: they run the whole suite,
so comparing there would announce a deviation five times a run -- but a truncated
run reporting fewer results than it ran is what #108 looks like, and "completed
cleanly" is the field that would show it.
Closes#83
build.yml's `release` job declares `contents: write`, and nothing checked it. Deleting those
lines leaves actionlint clean and CodeQL silent -- a narrower permission is not an alert --
and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no merge
can exercise it. Measured with the declaration removed: every gating check still passed. The
first thing that would notice is a release failing to publish, at the moment someone is
trying to cut one.
The deletion also looks like tidying. #106 has just put a top-level `permissions: contents:
read` directly above it, so a reader could reasonably take the job-level block for a
duplicate. It is an override, and a comment saying so is not a check.
BackupExclusionsTest is the precedent: configuration rather than code, load bearing, and
unguarded because nothing compiles it.
The part worth reading twice is the second commit-worth of work in here. The test passed,
and then the mutation that is supposed to redden it did not:
BUILD SUCCESSFUL in 614ms
Gradle cannot infer that a test depends on a file outside the source set, so the task stayed
UP-TO-DATE and the test never ran. Under --rerun-tasks the same mutation failed it properly,
which is the tell: the assertion was right and the wiring was not. A guard that does not
re-run when its subject changes is not a guard -- it is a test that will be green on the day
it matters, which is worse than no test because it reads as cover.
Fixed by declaring the workflow as a task input. Verified the whole way round afterwards,
without --rerun-tasks: mutate the file and the task re-runs and fails; restore it and the
task re-runs and passes.
What this pins and what it does not: it asserts the declaration exists in the release job's
block. It cannot assert a release actually publishes -- that needs a tag push, which is the
thing no PR can do. A tripwire against silent removal, not proof the path works, and the
KDoc says so.
Closes#107.
RealMediaBenchmark's class KDoc said:
Populate with:
adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says:
Internal storage, not the external files dir. Files placed in the external dir by
`adb push` or `adb shell cp` stay owned by the shell user, and the app then gets
EACCES trying to read them -- which presents as an unparseable input rather than a
permission problem.
Different directories, and the second exists specifically to explain why the first fails.
Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and
reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked
into by the instruction in the same file.
The fix is not a corrected command. Restating the mechanism in a second place is what let
these drift, and a replacement command I have not executed would be the same defect with a
fresher date. The class KDoc now names [samples] as the single place that answers it.
Two things added that are checkable rather than remembered: the exact filenames the tests
look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `<file>.mp4`, so even the
right directory left you guessing -- and a note that the two skips every green E2E leg
reports are these.
Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so;
it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the
guards work, and "harmless in CI" is accurate. The failure that prompted the look is
Media3EngineTest, tracked as #102.
Closes#101.
ConversionViewModelProbeFailureTest's pickedProbe() helper held:
val ready = awaitState(viewModel.state, "Ready with a probe") {
it is ConversionState.Ready && it.input.probe != null
}
assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))
The predicate requires `Ready`. `Ready` and `Failed` are sibling subtypes of one sealed
interface, so `ready as? Failed` is always null and the assertNull could never fire. R26
filed this PLAUSIBLE on types read; it is measured now.
Flipping the line to assertNotNull failed 3 of the 4 tests in the class -- three, because
pickedProbe() has three callers, which is also why a dead line here was worth removing
rather than shrugging at: it read as coverage in a helper the whole class depends on.
Deleted rather than replaced. There is nothing for a live assertion to add: a pick that
ended in Failed never satisfies the predicate, so awaitState fails on its timeout naming
what it was waiting for -- "Ready with a probe" -- which is a better failure message than
the assertion would have produced. The comment now says that, so the next reader does not
re-add the guard the predicate already is.
This is the ninth vacuous assertion this line of work has turned up, and the pattern is
consistent: they hide in helpers, they pass, and they look like care. The suite is green
before and after, which is exactly the point -- deleting a dead assertion cannot change a
result, and if it had, the line was not dead.
Closes#35.
SafPickerRoundTripTest began failing on gating legs at API 33, 34, 35 and 37
ninety minutes after it landed, on diffs that cannot cause it -- two KDoc
comments, a MIME lookup table, a README paragraph. Every failure named the
fixture root, so #93 was filed as a root-discovery race. It was not one, and
finding out what it was took making the test say something else first.
DocumentsUI was fine throughout: its own `ProvidersAccess: Matched roots` names
the fixture authority five times inside the sixty seconds the test spent failing.
What failed was reading any window at all -- 1095 `Retrieving node with selector`
against 1095 `Node not found` on that leg, against 7 and 2 on the green one. So
this now asks whether the app's OWN window is readable before it opens a picker,
and prints the accessibility window list when it is not.
That list named the culprit on the next occurrence:
What it could see: com.android.systemui[type=3], android[type=3]
No TYPE_APPLICATION window at all, on a device that had just logged `Displayed
org.libremediaconverter/.MainActivity`. `android[type=3]` is system_server, and
the same logcat says what it was holding, minutes before this class ran:
ANR in com.google.android.apps.nexuslauncher
Reason: Input dispatching timed out (Application does not have a focused window)
Window{4ed8414 u0 Application Not Responding: com.google.android.apps.nexuslauncher}
The launcher ANRs on a loaded runner emulator and the dialog it leaves behind
never goes away. It is opaque and fullscreen, so AccessibilityWindowManager drops
every application window beneath it -- which is how the app can be Displayed and
unreadable at once, the contradiction that made this look like a SAF bug for six
PRs. Present on both legs examined, API 33 and 34, at the failure timestamp.
So the dialog is dismissed, by resource id rather than by localised button text,
`aerr_wait` first so the app under it is left alone. Waking the device and
rebuilding the UiAutomation connection are kept behind it and are recorded as
measured non-causes rather than as fixes.
A second PickActivity is not a remedy for this either, and that was measured: the
failing leg opened one for the second test, in the same DocumentsUI process, and
read as little from it. The whole pick is still retried, but for a smaller and
separate claim -- a picker whose lists were built before their data arrived, which
#80's node-level re-find cannot reach because it re-acquires a handle inside the
one picker.
One API 37 run failed a step deeper, on the file rather than the root. That shape
has not been reproduced or diagnosed; the reopen covers it because a fresh pick
re-walks from Recent, and the KDoc says that rather than claiming more.
Two things the retry must not become. It must not tolerate an absent root, or
#64's MIME mutation goes vacuous -- so a missing node is reported rather than
retried away, and the mutation was re-run: both tests still fail, still with "the
system picker never showed BySelector [TEXT='\QLMC R38 fixtures\E']", in 126 s and
127 s against the 1200 s wrapper timeout. And it must not decide the picker has
closed by asking the same accessibility window list that is broken -- so the back
presses are counted against Activity.hasWindowFocus, which comes from the
framework.
Each new path was forced on and measured rather than trusted: the injected-failure
run showed the reopen recovering, with four OPEN_DOCUMENT starts for two tests;
the rebuild was forced unconditionally and the suite stayed green, ruling out a
connection that comes back without FLAG_RETRIEVE_INTERACTIVE_WINDOWS; the dialog
dismissal was forced with no dialog present, ruling out a blind click breaking a
healthy run. Dismissing a real ANR dialog has not been observed, because the fault
has never reproduced locally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two reasoned decisions were sitting in comments with nothing under them.
`AndroidDeviceCodecs.mimeFor`'s `COPY, NONE -> null` arm explains itself by
naming a consequence at another seam: returning null is what makes `canEncode`
answer true, because a copied or absent track places no demand on the hardware.
#90 pinned the null; nothing pinned the answer. Put a MIME in that arm and a
device with no matching encoder starts refusing stream copies — jobs that encode
nothing — and the router hands FFmpeg a re-mux Media3 could have done. Asserted
now against `forTesting(encoders = emptySet())`, with an H.264 refusal alongside
so a `canEncode` that simply said yes could not satisfy it.
The second is a whole table. `Media3Engine.videoMimeTypeFor` is `VideoCodec ->
MIME` on the same axis as `mimeFor`, and until #85 and #87 widened both to
`internal` no test could see them together. Each had per-arm coverage pinning its
own answers, which is exactly the shape that cannot notice the two tables
describing different codecs: change one arm and its own expectation together and
both suites stay green while the device is asked about H.265 and Transformer is
told to produce H.264.
They do not agree everywhere, and forcing them to would be a regression, so the
test sorts every codec into the three buckets that exist and asserts the fourth
is empty. H.264 and H.265 must match. VP8, VP9 and AV1 are named by the device
table and not by Transformer's, deliberately: `setVideoMimeType` rejects them so
the router never asks Media3, while the device may genuinely own a VP9 encoder
and `canEncode` has to answer about it truthfully. COPY and NONE are named by
neither. Sorting rather than filtering means a convergence fails too, so moving
the line requires saying so in the file.
Audio has no partner — `AndroidDeviceCodecs` enumerates video MIME types only,
so `audioMimeTypeFor` has nothing to cross-check against and a missing audio
encoder is still discovered by failing rather than up front. Named in the KDoc
as unfinished rather than left as an unexplained asymmetry.
Closes#86
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JMR-dev
