docs/e7-second-constraint
185
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jason Ross
|
f98e49942f |
Merge pull request #96 from JMR-dev/fix/saf-picker-root-discovery
Close the ANR dialog that was hiding every window from UiAutomator |
||
 JMR-devandClaude Opus 5
|
25f162923c |
Close the ANR dialog that was hiding every window from UiAutomator
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> |
||
|
JMR-devandClaude Opus 5
|
b3208ef8c7 |
Hold the two claims the codec MIME tables only asserted in prose
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
|
a0b6a3dde8 | Merge branch 'main' into fix/probe-dispatcher-seam | ||
|
JMR-dev
|
bda5abea6c | Merge branch 'main' into test/media3engine-mime-tables | ||
|
JMR-dev
|
21eeb6f3f8 | Merge branch 'main' into test/mediaprobe-pure-helpers | ||
|
JMR-devandClaude Opus 5
|
2063fe06aa |
Point the coroutines-test comments at the file that still uses it
Both the dependency declaration and its catalog entry named EscapedCoroutineErrors.kt as the sole reason kotlinx-coroutines-test is on the test classpath. That file is gone, and nothing in the gate -- not ktlint, not detekt, not lint -- fails on prose naming a deleted file, so this would have survived as a reference a reader could only resolve through git history. The dependency itself stays, and for a reason worth restating where it is declared: `runTest` is what registers the collector callback, so the one test that deliberately lets an error escape is the scope that receives it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
dab28d5f44 | Merge branch 'main' into fix/codec-vocabulary-drift | ||
|
JMR-devandClaude Opus 5
|
4aba3bbd2e |
Stop swallowing coroutine errors nobody asserted on
`drainEscapedCoroutineErrors()` cleared the collector at rule-construction time
with `runCatching { runTest {} }`, and discarding what it found was the whole
mechanism: it could not tell the one known deposit from an escaped error nobody
had asserted on. That traded a loud, misleading failure for a silent one, which
was acceptable only while exactly one depositor existed and the seam to remove it
did not.
The seam exists now, so the depositor is gone: the OOM is consumed by the test
that raises it. Every Compose class takes the v2 `createComposeRule()` directly,
and a future escaped error fails a test again instead of disappearing.
The two findings the drain's KDoc carried that outlive it: the v2 rule and the
non-v2 `StateRestorationTester` do interoperate -- the note now sits at the two
declarations that pair them -- and a drain could never have been a `@Before`
(the rule's `runTest` wraps it) or a `@BeforeClass` (Robolectric runs that
outside the sandbox classloader, where the collector is a different object).
Full JVM suite run twice in a row with the drain deleted: 373 tests, 0 failures
both times.
Closes #66
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
dbba213c51 |
Give the pick a dispatcher, so an escaped error fails the test that caused it
`onInputPicked` hops to a hard-coded `Dispatchers.IO` inside a `launch` with no exception handler -- deliberate, because a real OutOfMemoryError should reach the thread's default handler and take the process down. On the JVM there is no such handler: kotlinx-coroutines-test installs a process-wide collector, once per classloader and never removed, which keeps the error and rethrows it at whichever `runTest` starts next. Every Compose rule is a `runTest`, so the OOM raised by `ConversionViewModelProbeFailureTest` failed some *other* Compose class, and which one moved between runs of identical, green code. Naming the dispatcher gives the throw somewhere to land. With the pick inline inside a `runTest`, the collector's callback belongs to the test that caused the error, so it is handed over and consumed rather than stored for a stranger. Both hops of a pick rather than only the probe, which is where this differs from the seam issue #66 sketched: leaving the metadata query on a real IO thread makes the coroutine resume on a main looper Robolectric leaves paused, and that bounce is exactly the asynchrony that made delivery unpredictable. That buys the assertion the test could not make before -- the real OutOfMemoryError instance, not an inference from a card that never filled in, which is also what a probe returning null looks like. Reverting the hop to `Dispatchers.IO` turns it red: "expected java.lang.OutOfMemoryError to be thrown, but nothing was thrown". Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8ac6e2b1c2 |
Name the format in the image-demuxer failures
Bare assertTrue/assertFalse report java.lang.AssertionError and nothing else, so the mutation that proves this test bites -- relaxing the _pipe suffix to a substring -- went red saying only that a line failed. The format name is the one thing a reader needs, exactly as the MIME is in the sibling test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
4ff44be1d7 |
Give the Robolectric choice a reason that is still true
Two test classes justified using Robolectric by asserting that the alternative does not
exist:
AppRootRestorationTest "The instrumented tests cannot run on the development
host at all (see CLAUDE.md)"
OutputPublisherStagingTest "The instrumented suite cannot run on the development
host, so this is the only place [it] can be caught"
Both were true when written and stopped being true on 2026-08-22, when the segfault was
traced to SwiftShader's Reactor JIT against SELinux execheap rather than to the machine.
tools/local-emulator/run-e2e.sh has run API 33-36 here since.
The first one cites CLAUDE.md as its authority, and PR #73 corrected CLAUDE.md to say the
opposite. So it was no longer merely stale: a reader who followed the reference found the
contradiction, with the citation making the wrong half look verified. That is the worst
version of this -- R14, R15, R20 and R25 were all the same defect, and this is the fifth.
The choice itself was never wrong, which is why the fix is not to move these tests. Both
belong on the JVM, and the honest reason is cost rather than impossibility: neither needs
anything a device supplies, and both run inside the same ./gradlew invocation as every
other unit test instead of booting an emulator. That argument survives the correction; the
premise did not.
The old line also has a second failure mode worth naming. "Nobody can execute this" invites
a reader to skip the local run and let CI decide, which is the opposite of what the
definition-of-done in #51 asks for.
Verified: the string appears nowhere in app/src now, and testDebugUnitTest, ktlintCheck and
detekt are green.
Closes #46.
|
||
|
JMR-devandClaude Opus 5
|
7f951baf8f |
Make the two codec tables answer for each other, and stop describeAudio printing a NUL
The FFprobe codec vocabulary is written out in at least four places and none of them had a test. Two had already drifted apart. `x264`, `hev1`, `x265` and `vp09` resolved in `CodecNames.videoFromName` and returned null from `AndroidDeviceCodecs.mimeForCodecName`, so the app identified the codec for the source card and for routing and then ran the device capability check blind on the same string; `mpeg4` ran the other way and rendered as a raw name. Nothing could notice, and the reason is structural: a `when` cannot be enumerated, so no test can ask one table what the other one knows. Both are maps now, for that reason alone, and `CodecVocabularyTest` walks the two key sets. A name added to -- or removed from -- one side alone fails the build. The one legitimate asymmetry is listed rather than implied: `mpeg4` is decodable input with no `VideoCodec` to name it, so `CodecNames` is right not to carry it. That list is itself checked, because otherwise it is an escape hatch -- any future divergence could be waved through by adding the name to it, and adding `x265` to it now fails. THIS CHANGES BEHAVIOUR for `x264`, `hev1`, `x265` and `vp09`. A null from `mimeForCodecName` means "unknown to us: assume the platform can handle it and let a failed export trigger the FFmpeg fallback", which is the right policy for a name nobody recognises and the wrong one for a name recognised one file over. A device without the matching decoder now sends those four to FFmpeg up front instead of spending a doomed hardware attempt to discover it. No input loses hardware it could have used: each alias resolves to the MIME its canonical spelling already resolved to, so a device that has the decoder still answers true. `ConversionRouterTest` still passes and that is not evidence either way -- every `canDecode` in it is a hand-written stub that never reaches this table. #74 is the same family one level down. `describeVideo` answered "Unrecognised" for `InputProbe.UNPARSEABLE` and `describeAudio` had no such arm, so an unparseable audio codec would have fallen through to `?: name` -- and the sentinel opens with a NUL, so the source-info card would have rendered a `Text` beginning with U+0000. The two now share one body, which is what stops the next arm being added to one side only. Two corrections to that ticket, taken from the file rather than from the ticket, since it warns about exactly this: - It quotes `audioFromName` as opening with `null, InputProbe.UNPARSEABLE -> null`. It did not; it opened with `null -> null` and the sentinel reached `else`. Naming the sentinel in the shared lookup therefore changes no answer and is documentation, not the fix. - It says `describeVideo`'s arm has no test of its own. It did -- `descriptions stay readable for unknown and missing codecs` asserts it -- so deleting the shared arm now reddens three tests across both sides, not one. Mutations run, each on the full 386-test suite: add "avc3" to CodecNames only -> CodecVocabularyTest red on two counts, CodecNamesTest green: 8 tests, 0 failures, which is the ticket's point about per-table arm tests delete the UNPARSEABLE arm -> CodecNamesTest red on three, one of them quoting the NUL back add "x265" to DECODE_ONLY_NAMES -> CodecVocabularyTest red on the escape hatch delete "vp09" from the MIME map -> CodecVocabularyTest red on three, which is the state this commit is fixing Audio is not cross-checked, and that is a gap rather than a decision: the device capability check is video-only, so this module has no second audio table to compare `AUDIO_ALIASES` against. `Media3Engine.audioMimeTypeFor` is the other half and belongs to #85. `MediaProbe.shortName` (#84) is the fourth table and is untouched here for the same reason. Closes #87. Closes #74. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5ec2bba64b |
Check the MIME types Media3Engine hands Transformer, and the claim above them
Both tables decide what codec ends up in the user's file, and neither was exercised. Point H265 at VIDEO_H264 and every hardware HEVC export writes H.264 into a file the user asked to be H.265: Transformer does as told, the export succeeds, and the only symptom is a codec nobody chose. One arm carried an assertion rather than a value -- "Never reached: only an Encode plan consults this, and COPY/NONE are not Encode" -- which is a claim about callers parked in a branch of a callee. It is true, and nothing checked it, so it would have gone on reading as true after it stopped being. Proved instead: CopyPlanner answers both codecs before the Encode branch and its fallback draws from ContainerCapabilities.encodableVideo, which contains neither, so a sweep over every spec the planner can be handed asserts no Encode plan carries COPY or NONE. Counters guard the sweep, because `as? Encode ?: let` asserts nothing at all for a Drop or Copy plan. The audio sibling claim did not survive intact. "MP3 and FLAC have no Android encoder; the router routes them to FFmpeg" is true and incomplete: one rule, `audioEncode !in MEDIA3_AUDIO`, diverts Vorbis by identical logic, so three of the six encodable codecs never reach the table. VORBIS -> AUDIO_VORBIS is a correct mapping for a request Transformer is never given. The arm stays -- a right answer in unreachable code costs nothing -- and the comment now says so. The tables are asked of the router's decisions rather than of its codec sets, because the comments claim behaviour and a set can be right while the rule reading it is wrong. Both move to an internal companion object so a JVM test can reach them without constructing an engine, which would start a real HandlerThread to answer an enum lookup; #57's precedent, and the JVM test source set is a friend of main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd2bb1d889 |
Test the three MediaProbe helpers nothing else would catch
MediaProbe's MIME table, its image-demuxer rule and its Int reader are pure
functions with no test at all, and each fails silently rather than loudly.
shortName falls through to substringAfter('/') and reports a plausible-looking
string that CodecNames may or may not still recognise, so a dropped arm turns a
stream-copyable file into a re-encode. isImageFormat is checked before anything
else in classify, so a wrong answer overrides both probes. intOr's runCatching
is the only thing standing between a Float frame rate and losing every other
track property the loop had read.
Widen the three to internal, as #57 did, and say in each KDoc why the shape is
what it is -- the _pipe suffix is not a substring test because yuv4mpegpipe is
raw video, and getInteger casts rather than coerces.
Every format name asserted came from ffprobe rather than from memory: a picked
.png reports png_pipe, a .jpg reports jpeg_pipe, a .y4m reports yuv4mpegpipe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
3925f1aa9f |
Re-find the picker node when it goes stale, and re-measure API 37
CI found a flake this workstation could not, and fixing it overturned half of what
the previous commit recorded about API 37.
THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and
DocumentsUI is still settling when a node first appears -- its list rebinds, the
roots strip lays out, a window animates. If the node is replaced in that gap,
click() throws against the handle rather than missing the target:
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042)
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223)
It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every
one of them, on the first run. It never appeared here because the local emulator had
been warm for an hour. tapPickerNode now re-finds the node and taps again, three
attempts. That retries acquiring a handle to a node that has to be there anyway:
every attempt still goes through awaitPickerNode, which fails outright if it is
absent, so the MIME mutation's bite is untouched. Verified with `pm clear
com.google.android.documentsui` between runs, five for five green on API 34.
AND THE CORRECTION IT FORCED. The previous commit marked the whole class
@FailsOnEmulatorApi37 on the strength of two measured failures. One of them was
this bug. Re-measured with the fix, one method per fresh android-37.0 emulator:
thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED:
System has crashed.
pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED
So a rotation, which rebuilds every surface at once, is what the gralloc mapper does
not survive; starting another app's activity is not. The marker moves to the one
method that earned it, and the picker test runs on the gating API 37 leg like
anything else. The workflow comment, run-e2e.sh and the doc all say that now.
The lesson is worth more than the measurement, and the doc keeps it: an annotation
is a claim about an IMAGE, and a broken test makes every image look broken. Both a
framework abort and a stale node read as "the run fell over". Re-measure after
fixing a test before deciding what the platform did.
Also measured rather than assumed, since it is what keeps the gating leg green: the
runner's annotation filter honours a class-level marker, expanding it to every
method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2
here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating
API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker
to a single method is a narrowing rather than a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
a3c835b7c9 |
Keep the picker test off the API 37 gating leg, having measured why
The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper,
init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh
and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE
one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating
the display are not idle. They are the first things in this suite that generate
surface traffic of their own.
Both tests were measured on android-37.0 under swangle_indirect with SystemUI
disabled and verified quiet, and measured SEPARATELY -- inferring the second from
the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They
fail in the two shapes a framework restart produces:
thePickedInputSurvivesARealRotation
INSTRUMENTATION_ABORTED: System has crashed.
Expected 59 tests, received 50
(5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six
later tests never run and the XML carries a failure with no text at all)
pickingAFileThroughTheSystemPickerFillsInTheFileCard
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
(3 aborts; the picker's root node was rebuilt between finding it and tapping it)
Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is
the same evidence pattern that made the Media3EngineTest pair the image rather than
the app.
So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg.
THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes
the SystemUI-disable deviation defensible. It is no longer true of the suite, and all
three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's
header, and the doc. The rule they state is being APPLIED, not broken: the thing that
depends on system UI is excluded from the leg that cannot be trusted for it.
Two consequences stated rather than left to be discovered:
- run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37`
reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come
back short and which later tests ran is arbitrary. The summary row now says so;
it previously promised "exactly two failures", which would have read as a
regression in someone else's diff.
- The advisory job is still named "E2E API 37 Media3 hardware transcode", and half
of what it now runs is neither. Renaming a check touches branch protection, so it
is deliberately not done here; the doc records the staleness and the revisit
trigger now says the marker covers two unrelated bugs that can go green apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
650ca8fca3 |
Pick a file the way a user does, then rotate the phone
Two things nothing in this repo asserted, and they are one test class because
separately the second one asserts nothing new.
THE PICKER. ConverterScreen opens SAF with a MIME filter, and a filter is a thing
that can hide the user's file. Narrow it and the app still builds, still renders,
and still passes every JVM test -- the user taps "Choose file" and gets an empty
picker. The round trip now runs for real: DocumentsUI is driven with UiAutomator to
a fixture root, and the app is asserted to come back with the file.
The file card's name is not the only assertion, because a name proves less than it
looks: it comes from a metadata query, which a URI with no read grant answers just
as well. The "Container: MP4" detail row only appears once something has opened the
file and read its header, so it is what says the picker handed back a URI the app
can USE.
THE ROTATION. MainActivity declares no configChanges, and ConversionViewModel holds
the picked file in a plain MutableStateFlow with NO SavedStateHandle behind it.
Nothing persists it. The only thing that carries it across a rotation is the
ViewModelStore the Activity retains -- which no test anywhere asserted.
Two guards run before that assertion, because both ways it could pass while proving
nothing are silent: the display rotation really changed, and MainActivity really was
a different instance afterwards. Without the second one this is a recomposition test
wearing a rotation's name.
MUTATIONS, RUN RATHER THAN ASSERTED, on a local API 34 emulator.
Narrowing the filter to arrayOf("application/x-lmc-no-such-type") takes the fixture
root out of the picker entirely -- DocumentsUI matches the request against
Root.COLUMN_MIME_TYPES and drops roots that cannot answer -- and both tests fail:
java.lang.IllegalArgumentException: the system picker never showed
BySelector [TEXT='\QLMC R38 fixtures\E']
Making the ViewModel composition-scoped fails ONLY the rotation test:
androidx.compose.ui.test.ComposeTimeoutException: Condition (a node tagged
converter.fileCard.name exists) still not satisfied after 30000 ms
and :app:testDebugUnitTest stays BUILD SUCCESSFUL under it. That divergence is what
#64 exists to establish and what its own comment doubted; the PR body has the
verdict and why the doubt was reasonable.
THE PROVIDER HAD TO BE JAVA. It is the only Java file in the module. A
manifest-declared provider is a component of the instrumentation PACKAGE, so the
system starts a plain org.libremediaconverter.test process for it with only the test
APK on its dex path -- and the test APK is built without the Kotlin stdlib, because
the app APK has it and duplicating it is what checkDebugAndroidTestDuplicateClasses
prevents. The Kotlin draft died on its first query:
java.lang.NoClassDefFoundError: Failed resolution of: Lkotlin/jvm/internal/Intrinsics;
at org.libremediaconverter.saf.FixtureDocumentsProvider.queryDocument
The compiler emits that reference for the null checks on nearly every function, so
no Kotlin dialect avoids it. Same reason nothing in that file imports androidx.
No new test tags: CHOOSE_FILE, FILE_CARD_NAME and detailRow already named both ends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b18f45def7 |
Give the system file picker something to pick
Nothing in either source set drives SAF as a picker. The only SAF coverage is the publish side, in OutputPublisherPublishTest, against hand-written ContentProvider fakes -- so the launcher wiring in ConverterScreen, the MIME filter it passes, and the grant that comes back have never been executed by a test. Driving the real picker needs three things this repo did not have. UiAutomator, because DocumentsUI is another process. Compose's matchers stop at this process's composition and Espresso's stop at its view hierarchy; neither can see or tap a window belonging to another package. It FLOATS, at "2.+", which is the same argument the catalog already makes for work and lifecycle rather than a new one: androidx.test.uiautomator is inside floatedGroupPrefixes, so the componentSelection guard makes "+" mean "newest RELEASED", and that is load-bearing here -- this library publishes 2.4.0-alphas above its stable, so without the guard the float would be a pin to a prerelease. Resolved to 2.4.0 (released) on debugAndroidTestRuntimeClasspath, checked rather than assumed. It is deliberately NOT pinned alongside ktlint/detekt/JaCoCo/Robolectric: those are pinned because a new rule or a new runtime changes the verdict on files nobody touched. UiAutomator has no verdict -- it taps what a selector names, and a selector that stops matching is this repo's test to fix, in a diff that explains itself. The "2." rather than a bare "+" is the one thing held back: a major is where the selector API would be free to change under exactly that assumption. A DocumentsProvider, because DocumentsUI does not browse a filesystem -- it lists what providers offer it. Writing a file into Downloads would have worked and tested less: the fixture root declares Root.COLUMN_MIME_TYPES, and DocumentsUI filters the drawer by it, which is what gives the MIME filter a mutation with a shape rather than "one file among the hundreds in Downloads was not listed". Its contents are also exactly one file, where a shared directory accumulates whatever earlier runs left behind. And the first AndroidManifest.xml this source set has ever had, to declare it -- a ContentProvider is instantiated by the system and cannot be registered from test code. In androidTest rather than src/debug so it is installed by the instrumentation APK only, and never appears in a developer's own file picker. Two things worth knowing before editing either file. XML comments cannot contain "--", which the manifest's first draft failed the build on; and "*/" inside a KDoc closes the comment, which the provider's did. Both are silent in review and loud in the build. No test yet, and no new test tag: TestTags.Converter.CHOOSE_FILE and FILE_CARD_NAME already name both ends of the round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
02555ceb91 |
Ask each join state what it lets the user do next
`JoinScreenContent` decides the whole join UI in one `when`, and until R38.5 gave it a state
parameter nothing could ask it anything: `Waiting` follows a denied foreground start and `Joined`
follows a finished `ConcatWorker` run, so neither is reachable by driving a real `JoinViewModel`.
`JoinScreenContentTest` used that seam to prove it exists, on one state. This is the matrix behind
it -- seven states, each pinned to the affordance it offers and the callback that affordance is
wired to, asserting on the value handed back rather than on something merely having fired.
Two of the thirteen assert things nothing else in the suite has ever asked.
The rows are read back sorted by their position on screen and compared as an ordered list. A join
is the one flow where the order of the inputs is the content of the output -- the empty state
promises "in the order you want them" -- and `JoinLeafTagsTest` proves only that a row tags itself
with the file it shows, which a reversed list would satisfy just as well.
The progress bar is asserted to be indeterminate, not merely present. It carries no percentage on
purpose, because FFmpeg reports progress against one input's duration and that means nothing across
a concatenation; the converter screen's bar is determinate, so "there is a bar" is exactly the
assertion that would let a fabricated percentage land here unnoticed.
Three mutations, each reverted after:
- `Text(s.message)` -> `Text("")` in `Failed`: "a failed join renders the message it carries" fails
with `could not find any node that satisfies: (Text + InputText + EditableText contains 'The
second file has no audio track, so joining stopped.')`.
- `when (s.strategy)` -> `when (ConcatStrategy.STREAM_COPY)` in `Joined`: "a re-encoded join says
the files differed" fails on the copy for the branch that no longer runs.
- `s.inputs.forEach` -> `s.inputs.reversed().forEach` in `Ready`: the ordering test fails
`expected:<[join.fileRow:intro.mp4, join.fileRow:middle.mp4, join.fileRow:outro.mp4]> but
was:<[join.fileRow:outro.mp4, join.fileRow:middle.mp4, join.fileRow:intro.mp4]>`.
Test-only: no file under `app/src/main` changes, and no tag is added to `TestTags`, because every
string these states render is either already tagged or unambiguous as text. The typographic
characters in the asserted copy -- U+2026 in "Joining N files...", U+2014 in the `Joined` and
Paused lines -- were checked byte-for-byte against `JoinScreen.kt` rather than retyped; an ASCII
lookalike compiles and then quietly matches nothing.
Closes #63.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2f3f461cc1 |
Say what each conversion state puts on screen, and what it withholds
The screen's state machine had a seam and no matrix behind it. Every arm of the `when` returns `Unit`, so an arm can render anything at all and still compile -- a button offered where it cannot work, a state's own data never reaching the node meant to show it, an affordance wired to the wrong callback. The leaf tests cannot see any of that: they compose `FileCard`, `AdvancedPicker` and the three pickers directly and never hold a `ConversionState`. The arm worth guarding most is `Ready`'s `enabled = validation.isValid`. The Advanced picker deliberately lets an impossible container / codec pair be selected, so that one expression is all that stands between an invalid spec and a job that cannot succeed. `enabled = true` compiles, renders an identical screen apart from one colour, and passed the whole suite before this. Callbacks are asserted over the complete log rather than one at a time, so a case reads "this one fired and nothing else". A bare "the callback ran" check stays green on an arm that fires the right callback for the wrong reason. The routing chip needed a tag to be locatable at all: its text comes from the finished job, so a text matcher would have to name a routing explanation the screen does not own. That is the only production change here. Not asserted, deliberately: `Failed`'s error colour, which Compose publishes nowhere in the semantics tree; and the three absent `FileCard`s, which are compile-guarded -- `Idle`, `Saved` and `Failed` carry no `input` -- so those lines state the intent without being what enforces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
46ad95350b |
Give both screens somewhere for a state to come from
`ConverterScreen` and `JoinScreen` each inlined their whole `when (state)` inside the public entry point, and state arrived only as `viewModel.state`. That left four of the twelve state branches across the two screens with no test that could ever reach them: driving a real ViewModel needs a WorkManager and a media probe in the constructor, and even then `Waiting` follows a denied foreground start and `Converted`/`Joined` follow a worker run that has already succeeded. So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`, which take the state, the settings, the validation and an actions holder. The entry points keep the three launchers and `collectAsStateWithLifecycle` and nothing else. The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose parameters because detekt's `LongParameterList` sits at its default threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` -- `AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn a clean detekt run red; data classes are exempt from the rule. Nothing else changed. The body was cut and pasted rather than retyped, so the U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable, and deliberately kept -- survives the move. The diff stops above `FormatPicker` in one file and above `FileRow` in the other, which is why the leaf suites #57-#60 landed pass unedited: every one of them composes a leaf directly and none references either entry point. The two new tests are the bite, one per screen and one per direction of the seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back the name the finished job chose. The state matrix itself is #62 and #63. |
||
|
JMR-dev
|
3d55004286 |
Count the Robolectric tests, which JaCoCo has never counted
The three #52 test PRs landed 56 new tests and the coverage figure moved 29.8% -> 29.7%. That looked like the tests being worthless. It was the measurement. Robolectric loads every class it touches through its own sandbox classloader, and those classes arrive with no source location. JaCoCo skips no-location classes unless told otherwise, and nothing here told it. So not one Robolectric test has ever contributed coverage in this repo -- and Robolectric is what exercises the framework edge: both workers, the publisher, both ViewModels, every Compose screen. Same commit, same 335 tests, same 0 failures, only the block below added: LINE 652/2194 29.7% -> 1519/2194 69.2% BRANCH 425/1424 29.8% -> 758/1424 53.2% OutputPublisher 0.0% -> 97.5% MainActivityKt 6.8% -> 86.4% ConversionViewModel 0.0% -> 85.4% ConverterScreenKt 6.6% -> 62.8% The discriminator, so this is not cargo cult: inside ConverterScreenKt, `describe` is the one non-Composable and is exercised by a plain JVM test. It reported 8/8 covered while every @Composable in the same class reported 0 -- including ones whose mutations demonstrably failed the build when reverted. Across files the split is exactly Robolectric-vs-not: StagingSweep, tested purely, 100%; OutputPublisher, ConversionViewModel and FailureOutcome, tested under Robolectric, 0%. `excludes = listOf("jdk.internal.*")` is not decoration. Without it JaCoCo walks JDK-internal classes Robolectric has no location for either and the test JVM dies rather than reporting a number. CLAUDE.md's coverage bullet is rewritten, because it was wrong twice over. The figure was an artifact, and the explanation attached to it -- that coverage fell as the suite grew from 11 test files to 43 because the denominator outran the numerator on framework-edge code "the JVM cannot reach" -- described a cause that does not exist. The JVM reaches that code fine. The new tests were disproportionately Robolectric, so each one added denominator and no numerator: the measurement was punishing precisely the tests that were hardest to write, and the conclusion drawn from it was that writing them had not helped. Mutation, run both ways on this branch: remove the block and jacocoTestReport collapses back to 29.7% / 29.8%; restore it and it returns to 69.2% / 53.2%. Two things that were true stay true. There is still no coverage gate, and a floor still needs a settled baseline -- this one just moved 39 points in one build change. And "re-measure before quoting it" was already written down; following it is the only reason this was found. |
||
|
JMR-dev
|
3c5a37fd3c | Merge branch 'main' into test/r38-2-filecard | ||
|
JMR-dev
|
4ea5afefe1 | Merge branch 'main' into test/r38-4-advanced-picker | ||
|
JMR-devandClaude Opus 5
|
fea88a281f |
Say in tests what the file card says when it does not know
"Size unknown" is the line a stream fixing D5 reported as untestable. It is two assertTextEquals calls, and it needed two rather than one: the size line renders independently of the probe, so it is asserted with a probe and without one. That independence is the contract, and a test of the probed case alone would leave the branch a user hits first -- the card is on screen before the probe finishes -- unguarded. The rest of the card degrades in words the same way, and none of it was covered: the four InputKind branches, "No video track", "No audio track", describeVideo's "Unknown", and the two `> 0` guards that drop the dimension and length rows rather than printing 0 and 0:00. Each guard gets a case on both sides, because the present side alone stays green when the guard is deleted -- what deleting it produces is "Size: 0x0" and "Length: 0:00", the same invented-measurement defect as "0 B". The four pure helpers go in a plain JVM class beside it, with formatBytes pinned at each threshold and one byte below it. A `>=` quietly becoming a `>` is only visible from a value sitting exactly on the boundary. Two things the issue could not have known: - Its second acceptance criterion, "delete the return@Column and watch the Reading... test go red", cannot happen -- it does not compile. The early return is what smart-casts `probe` non-null, so ten uses below it fail with "Only safe (?.) or non-null asserted (!!.) calls are allowed on a nullable receiver". The exit is enforced by the compiler, not by a test. Both compilable regressions someone would land instead are covered and were run red. - CodecNames.describeAudio has no UNPARSEABLE arm, unlike describeVideo, so it answers the raw sentinel rather than "Unrecognised". Unreachable today, because the UNPARSEABLE kind renders the explanatory line instead of rows. Left alone; recorded on the PR for R38.5. The divider's absence is not asserted and cannot be: Material 3 renders it as a Box with no semantics modifier, so it contributes no node. What is asserted is everything it precedes, plus the card's child count. The class KDoc says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7c69d0699a |
Hold the Advanced panel's gate, and the error card outside it
`AdvancedPicker` is the one leaf on the converter screen that carries its own state, and `ValidationError` is deliberately invoked after the `AnimatedVisibility` that gates the chip rows -- so an invalid spec explains itself and offers one-tap fixes while the section is collapsed. That is the only route out of an invalid spec for a user who never opened Advanced, it was completely untested, and folding the two `if` blocks into one is a plausible tidy-up that compiles. `AdvancedPickerTest` covers the gate in both directions, clicks each of the four colliding chip labels through its own row tag, and does every assertion about the error card with the toggle untouched. `AdvancedPanelSavedStateTest` is the `DestinationSaverTest` split for `expanded`: `StateRestorationTester` saves into an in-memory map, so it proves `rememberSaveable` is in use and nothing about the representation. Driving a real `SaveableStateRegistry` shows the picker saves the `MutableState` itself rather than the `Boolean`, which only survives a rotation because `mutableStateOf` on Android returns a `Parcelable` one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2bed40d080 |
Hold the three pickers to the constant they hand back
The format, quality and engine pickers are the same dozen lines with a
different enum substituted, and both ways they can go wrong are silent.
An onClick that closes over the picker's `selected` parameter instead of
the chip's own entry returns one constant for every chip; an inverted
`entry == selected` lights every chip but the right one. Neither throws,
neither changes the labels on screen, and a test that only asserted the
callback ran would pass over the first of them.
So each click test presses every chip in the row and compares the whole
recorded list against `entries`, which makes the constant load-bearing
rather than the click count, and each selection test asserts over every
chip rather than only the one that should be lit.
Verified by mutation, not by the suite going green:
- `onSelect(format)` -> `onSelect(OutputFormat.MP4_H264)` fails with
`expected:<[MP4_H264, MP4_H265, WEBM_VP9, ...]> but was:<[MP4_H264,
MP4_H264, MP4_H264, ...]>`
- `format == selected` -> `format != selected` fails both format
selection tests on `Selected = 'true'` for a chip that should not be
- `onSelect(preference)` -> `{}` fails with `expected:<[AUTO,
PREFER_HARDWARE, FORCE_SOFTWARE]> but was:<[]>`
- `selected.description` -> `QualityTier.FAST.description` fails the
quality prose test on the missing BEST line
Labels are read off the enums so a reword cannot redden this file for
the wrong reason. `EnginePreference` has no label of its own, so the
screen's own `label()` supplies that set. The one display literal with
no symbol behind it, the custom-spec line, was copied out of the source
byte for byte because it holds a U+2014 that would fail silently if
retyped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
df2e42a2b3 |
Name the screen leaves, and give the tests a tag vocabulary
Kotlin `private` on a top-level declaration is file-scoped, so every leaf composable in the two screens was invisible even to the JVM test source set, which is a friend of main. The only three declarations src/test could name were ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test against, which is why #52 could not be started as filed. `internal` is the same choice MainActivity already documents for Destination: the unit tests can name it, and it stays invisible to anything outside the module. Eleven declarations in ConverterScreen and FileRow in JoinScreen. The tag table is the other half. Tests reference a symbol rather than a literal, which is what keeps the five children that follow independent: "Cancel", "Start over" and "Save file" are each rendered by both screens and by more than one state branch, so rewording one would otherwise redden several PRs at once and no diff would explain why. There were zero testTag, semantics or contentDescription calls anywhere in main before this. Every tag is applied inside main. A tag a test hands down as a Modifier proves only that the test set it -- it would survive the affordance losing its own tag entirely, which is the vacuous shape CLAUDE.md records nine of in one review. That is why FileRow and DetailRow derive theirs from data they already hold rather than taking an index from the call site. TestTags is public rather than internal, and R38.8 is the reason. It reads the table from androidTest, and whether that is a friend source set of main under AGP 9 had no in-tree answer -- nothing referenced a main internal from there. Settled by compiling one: it is a friend, so internal would work today. Public anyway, because that friendship is AGP wiring rather than something this project states, and the KDoc now carries the measurement so nobody has to repeat it. Smoke tests cover each leaf: it renders, and its tag resolves to exactly one node. Counting rather than asserting existence is deliberate, since a duplicated tag fails differently depending on which finder a later test happens to use. The state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars -- have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the state matrix is R38.6/R38.7 by design. Five mutations, all red on the named test alone: FORMAT_CHIPS deleted, FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label, FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value -- the last caught only by the uniqueness check, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
64c1a60a97 |
Drain the escaped coroutine error before the next test starts
`ConversionViewModelProbeFailureTest.an OutOfMemoryError is not swallowed` deliberately lets a real error escape `viewModelScope.launch`, which has no exception handler by design -- the ViewModel's KDoc says an OOM raised in the probe should reach the thread's handler and take the process down. On the JVM something else takes it. kotlinx-coroutines-test installs a process-wide collector for uncaught coroutine errors, keeps whatever it catches, and hands the backlog to the next `runTest` that starts, which throws UncaughtExceptionsBeforeTest. Every Compose test is a `runTest`: that is how `createComposeRule` runs a composition. So the error lands on an unrelated test in an unrelated file, and the message names neither the test that caused it nor the error's origin. Nothing has hit it yet only because the sole Compose test in the repo happens to run before the ViewModel one. R38 adds six more Compose classes in exactly the two packages that surround it, and the first two of them made the suite fail in two different files on two consecutive runs of identical, green code -- the throw is on a real Dispatchers.IO thread, delivered after the state assertion that ends the test responsible, so which class catches it is a race. Drain it where a Compose rule is built. A @Before cannot: the rule's `runTest` wraps the statement that calls it, so it has already thrown. @BeforeClass cannot either, because Robolectric runs it outside the sandbox classloader, where the collector is a different object. Constructing the rule is early enough, since JUnit builds a fresh test-class instance -- and every @get:Rule field on it -- before evaluating any rule. This is containment, not the cure. The cure is a seam: give the probe hop an injectable dispatcher the way the constructor already does for cleanupDispatcher, so the error has somewhere to land. That is a production change and deserves its own commit. kotlinx-coroutines-test was already on the unit-test classpath through compose-ui-test-junit4; it is declared now because a file imports it. Pinned, like robolectric and the linters: org.jetbrains.kotlinx is not one of the groups the prerelease guard covers, so a float here would be free to take a milestone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
225ecdd7e6 |
Split the API 37 leg so the part that works can gate
CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
d3975617a8 |
Put a gate on the three pieces of wiring that had none
Three separate mutations passed the full 257-test suite, all for the same reason: the tool was tested and the thing that calls it was not. The join half of per-job staging. Reverting ConcatWorker to the constant the audit's own D8 table names -- "joined.<ext>", one string for every join of a format -- left everything green: PerJobStagingTest drives only the conversion worker, and StagingNamesTest pins only the pure function. The new case drives two real ConcatWorkers with different ids and reads what they asked for rather than what is on disk, because ConcatEngine is native, so neither join gets past it here and the catch on the way out deletes what it staged. The recorder moves into WorkerStubs, which is what that file is for, and the enum test that already had a private copy now uses it. The process-start sweep. Deleting the one line in LibreMediaConverterApp.onCreate() -- the only reason that class exists, and the backstop for every leak discardStaged cannot reach -- left everything green too. The test stages one file a day old and one written now, calls onCreate() again, and asserts both halves: the abandoned one is collected and the live one is not. The second half is what says this is a sweep rather than the clearStaging() it replaced, which could take a file out from under a running job. The mtime is set explicitly, because "written long enough ago" is not something a test can wait for when the period is twenty-four hours. Casting the Robolectric application to LibreMediaConverterApp is an assertion in itself: it fails if android:name ever stops pointing here, in which case the swept line would be correct code that never runs. The backup and device-transfer exclusions. Reverting data_extraction_rules.xml to the template's boilerplate left the unit tests green AND lintDebug green -- it is a resource, so nothing was reading it -- and the failure it causes is one nobody meets in development. WorkManager's queue is the app's whole backup payload, and its rows name content:// grants and cacheDir paths that do not survive a transfer; reattachment queries by tag on launch, so a fresh install would come up attached to a job the user never ran on it. The test reads the compiled resource table, so what it pins is what the APK carries, and it asserts domain and path for all four entries in both sections -- an <exclude> with no path is skipped unchecked by lint's own detector, so half an entry could protect nothing. Its KDoc records the one thing it does not cover: the manifest attribute that points the system at the file. R8 / #17, R9 / #18, R11 / #20 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3534c6d996 |
Clean up the empty document a refused open leaves, and stop the space sums wrapping
publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a6cf4f4ff4 |
Stop three enum reads escaping doWork, and pin what the attempt bound buys
Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit
ABOVE the try. A name this build does not define -- which is what a downgrade or a
rollback with work still in the queue produces, since WorkManager keeps work for about a
week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature
verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen
says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays
in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc
says why.
So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default.
Consistency argues for it as much as correctness does -- the file already contains the
right answer to this question, three times.
WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the
unknown name rather than only that nothing threw: a quality tier falling back to something
arbitrary would convert at a setting nobody chose, and a join's format decides the
extension its output is staged with, which is where FFmpeg infers the container from. The
engine-preference case asserts through the space check instead, because predicting which
engine AUTO picks would tie the test to a routing rule it is not about. Against the
unfixed code all three fail with "No enum constant ...".
MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written
against the symbol, which pins the relationship and leaves the number free: changed to 2,
the job gives up about ninety seconds after process death -- exactly the long conversion
the retry exists to protect -- and all 257 tests stayed green.
The new assertion is the property the KDoc argues, not the literal: summed against
WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have
to span at least eight hours, which is what makes "the user will have opened the app by
then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the
accident does not, and reports the span it got (0.025 hours at 2).
R22 / #31, R10 / #19
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
c5c4c5323b |
Test the edge that feeds reattachment, and stop it reporting ENOENT
Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c2e6344aad |
Let WorkManager keep sole ownership of the progress notification
`publishProgress` posted with `NotificationManager.notify(NOTIFICATION_ID, …)` directly, on the
very id WorkManager owns through `setForeground`, using a notification built `setOngoing(true)`.
Two posters for one id is a race about which of them wrote last, and on a Pixel 10 Pro XL it was
lost on attempt 3 of 12 while cancelling a `BEST`-tier job:
attempt 3: terminal state = CANCELLED
attempt 3: +300ms active=0 id1001=false ongoing=null <- WorkManager tore it down
attempt 3: +700ms active=1 id1001=true ongoing=true <- a progress tick put it back
attempt 3: +5000ms active=1 id1001=true ongoing=true
Still there ten minutes later, with no app process left in the world to withdraw it. The orphan's
record carried `flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and **no `FOREGROUND_SERVICE`**, which is
what identifies the poster rather than merely suggesting one: WorkManager's own post goes out
with that flag and this one did not.
Whether the user could then swipe it away is deliberately not claimed here. An earlier reading
said "cannot dismiss", on the strength of `isClearable()` returning false — but that is false
purely because `FLAG_ONGOING_EVENT` is set, the record carries neither `FOREGROUND_SERVICE` nor
`NO_CLEAR`, and API 34+ lets an ongoing notification with no foreground service behind it be
swiped. The SystemUI test was impossible behind a secure lock screen and was never done. The
resurrection is what is reproduced, and it is what this fixes.
Progress now goes through `setForegroundAsync` — the non-suspending half of the same call
`doWork` already makes, which is the supported way to update a running worker's foreground
notification. That is not merely a different mechanism for the same post: it hands the id back to
its owner, so the notification WorkManager withdraws when the job ends is the same one the last
progress update wrote, and there is nothing left holding a second reference to it.
Two guards, and they are independent on purpose:
- **`isStopped` short-circuits the whole function.** A tick arriving after the stop has nobody
left to report to, so a stopped worker publishes nothing at all — the `setProgressAsync`
included, which WorkManager refuses for finished work anyway.
- **`setForegroundAsync` refuses on its own** for work whose state is already terminal. That
closes the window between the `isStopped` check and the update reaching the task thread,
which a check alone can only narrow.
The `~1/sec` throttle is unchanged, in effect and in reason: FFmpeg's statistics callback and
Media3's progress polling both fire several times a second, and pushing every one of them janks
the system UI. Routing them through WorkManager does not make them cheap, so the interval stays
exactly where it was. It is reordered into an early return rather than a nested `if`, which is
the only difference.
The initial `setForeground(...)` in `doWork` is untouched and stays a suspending call inside the
`try`. That placement is the whole of the previous commit on this file: a denied background
foreground-service start throws there, and `FailureOutcome` turns it into a retry rather than a
terminal failure. Converting it to `setForegroundAsync` for symmetry would have moved that
exception into an unobserved future and undone it silently.
The future `publishProgress` gets is not awaited — it is called from an engine callback, which is
not a coroutine, and a progress update is not worth blocking one for. Both of its failure modes
are benign, so a refusal is logged rather than propagated: dropping it entirely would make the
one that actually happens, a job finishing mid-update, invisible.
`ProgressNotificationTest` drives the real worker with a recording `ForegroundUpdater` — the same
seam `DeniedForegroundStartTest` uses — and asserts on both halves, because either alone passes
against something wrong. The positive half is that the update reached WorkManager on the id it
already holds, carrying `Notification.EXTRA_PROGRESS` of 42; a test that only checked nothing was
posted directly would pass just as well against a `publishProgress` that had been deleted. The
negative half is that Robolectric's notification manager holds nothing at all, asserted over the
whole manager rather than one id, so a renamed constant cannot make it pass by asking about a
notification nobody posts.
All three were red before the change: `one throttled progress update expected expected:<1> but
was:<0>` (nothing had reached WorkManager), `and must resurrect nothing expected:<0> but was:<1>`
(the device's resurrection, on the JVM), and `50 ticks inside one throttle window must not be 0
updates`. Each guard was then removed on its own to check the test that names it bites: without
`isStopped` the stopped worker publishes twice — `a stopped worker must publish nothing
expected:<1> but was:<2>` — and without the throttle fifty ticks become fifty updates.
`ConcatWorker` reports no progress at all and owns id 1002 by itself, so it has none of this and
none of it is added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b86df47c43 |
Tell an unknown input size apart from an empty file
`queryFile` started at `var size = 0L` and only moved off it when a provider answered the
`OpenableColumns.SIZE` column, which the platform documents providers *may* omit. So "this file
is empty" and "nobody would tell me how big it is" reached `OutputPublisher.hasSpaceFor` as the
same number, and `hasSpaceFor(0)` is not a space check -- it is "is there 128 MB free", which any
phone with a working camera passes.
The reachable value is not an inference. On a Pixel 10 Pro XL, `contentResolver.query` on a
`file://` URI returns null outright, so the cursor block never runs at all and the default
survives untouched: `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that
exactly -- null query, and a descriptor that reports 4321 bytes for the same file -- which is why
every test here is a JVM test rather than a device one.
`InputQuery` replaces the two copies of `queryFile`, which were byte for byte identical in
`ConversionViewModel` and `JoinViewModel`, so a fix to either would have been a fix to half the
app. It asks for the size twice: what the provider says, and then what the file itself says
through `openFileDescriptor(uri, "r").statSize`. The second needs no cooperation beyond the input
being openable, which a conversion is about to require anyway, and it is what answers the device
case above. Only when both decline is the answer null, and `InputFile.sizeBytes` is `Long?` so
that null cannot be spelled the same way as zero again.
**What an unknown size does was the decision, and it is deliberately not a refusal.**
`OutputPublisher.hasSpaceForUnknownSize()` produces the same number the defect produced by
accident -- with no size to reserve for, the headroom is all there is left to check -- and that is
worth saying plainly rather than dressing up. What changed is that it is now the answer to a
question that was asked. `hasSpaceFor` means "there is room for this many bytes" and nothing else
claims it.
Refusing was the obvious alternative and would have been worse than the bug: it turns "no
provider answered the SIZE column" into "this file cannot be converted", for a user who can do
nothing about either. A fixed floor was the other, and there is no honest number for it -- a
1 GB floor refuses a 10 MB conversion on a device with 500 MB free, which is the same failure in
a costume. `SpaceCheckTest` pins the choice from both sides: an unmeasurable input must not end
the job, and a full disk must still refuse it.
That second half is why the default answers *through* `hasSpaceFor`. `FakeFailures.FullDisk` in
the instrumented suite overrides `hasSpaceFor` and nothing else, so the delegation is the only
reason it still refuses an unknown-size job. Replacing the delegation with a bare `true` leaves
the full-disk test red with `expected:<Failure {error : Not enough free space to convert.}> but
was:<Failure {error : FFmpegKit failed to start on brand: robolectric...}>` -- the job sailed past
the guard and died at the engine instead.
Both workers get the same shape. The size arrives as input `Data`, which has no null, so the
absence of the key *is* the unknown -- `getLong(key, 0L)` was the other half of the conflation.
When it is absent the worker measures the input itself, which it can do because it holds the URI:
that covers a `request(...)` built by hand and work enqueued before the size became optional, and
it costs an ordinary job nothing because it runs only on the fallback. `ConversionWorker.request`
writes neither the `Data` entry nor the `JobTags.sizeBytes` tag for a size nobody knows, since a
tag reading `size-bytes:0` would come back through `Reattachment` as a confident claim that the
user's file is empty -- and `reattach()`'s `?: 0L` is gone for the same reason.
A join's total is `InputQuery.total`, which is null the moment a *single* input cannot be sized.
Summing the ones that answered was the competing reading and is rejected: a lower bound is
indistinguishable from a real total once it reaches the space check, so the guard would reserve
for half the job and pass. Reverting it to `sumOf { it ?: 0L }` records `[1111]` for a two-file
join whose second input nothing can measure.
Work already in the queue keeps the old conflation, and there is no fixing it. The previous
`request()` always wrote `putLong(KEY_SIZE_BYTES, sizeBytes)`, so a job enqueued before this
commit for a file nothing could size carries the key *present* and set to zero -- which reads
back as a declared size of zero and is trusted, exactly as before. Its `lmc.size-bytes:0` tag
reads back the same way, so `reattach()` shows such a card "0 B" rather than "Size unknown".
Nothing can separate that from a genuinely empty file after the fact, and a rule that treated a
declared zero as suspect would only rebuild the conflation facing the other way. WorkManager
keeps finished work for about a week, so this is a bounded window that clears itself; new work
never enters it.
`hasSpaceFor`'s KDoc claimed peak usage was "roughly input + output at once" while the arithmetic
reserved `input + 128 MB`. The arithmetic is what stays and the doc now says why: `bytes` is the
input's size standing in for the output's, generous for the ordinary conversion (which is asked
for precisely because it shrinks its input) and short for a re-encode to a bulkier codec; the
128 MB absorbs that error and the transient double copy while `publish` runs. Reserving
`input + output` outright would refuse jobs that fit. This is a pre-flight check that stops an
obviously impossible job from spending minutes finding out, not a guarantee -- a conversion that
runs out of space anyway still fails through its engine.
The measurement side of that line is untouched on purpose. `StorageManager.getAllocatableBytes`
is a separate entry with its own device evidence and its own `informational += "UsableSpace"` in
the lint block; this commit is about the number going *in*. `hasSpaceFor(bytes: Long)` keeps its
signature and stays open, so nothing overriding it had to change.
The file card says "Size unknown" rather than `0 B`. Handled at the call site rather than inside
`formatBytes`, because a formatter that invented a number would be the defect on screen; the card
already degrades in words for a file nothing could read.
Five of the seven new tests were red before a line of production code moved, with the numbers
they were about: `expected:<4321> but was:<0>` for a picked file, `expected null, but was:<0>` for
one nothing can measure, `expected:<[1111, 2222]> but was:<[0, 0]>` for the join picker, and
`expected:<[4321]> but was:<[0]>` and `expected:<[3333]> but was:<[0]>` for what the two workers
asked the space check. The tests assert on the *question* rather than the verdict, which matters:
one that only checked whether the job ran would have passed against the defect, since the defect
is that the guard is vacuous rather than that it refuses.
The remaining two needed the new call to exist first, so each was proved by mutation instead.
Answering the unknown with `hasSpaceFor(0L)` inline leaves `expected:<[]> but was:<[0]>` in both
workers; refusing it instead leaves `an unknown size must not end the job; got Failure {error :
Not enough free space to convert.}`. Making the worker always measure rather than trust a declared
size leaves `expected:<[9999]> but was:<[4321]>`.
`join()`'s own use of `InputQuery.total` is tested separately from the function, because
`StagingCleanupSupport` already records what that distinction costs: a tool can be provably right
while nothing calls it. `SucceedingWorkerFactory` now keeps the input `Data` of every request that
reaches a worker -- the only place it is legible, since `WorkInfo` hands back a job's tags and its
output and never the `Data` it was built with -- and the test reads the enqueued total off it.
Restoring `inputs.sumOf { it.sizeBytes ?: 0L }` leaves every other test in the change green and
this one red with `a total that could not be worked out must not be enqueued as a number`.
`ConversionViewModelProbeFailureTest` expected `InputFile(INPUT, "input", 0L)` for an authority no
provider serves. It expects `sizeBytes = null` now, which is the behaviour change stated where a
reader will meet it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
49535998b5 | Merge branch 'fix/worker-durability-and-naming' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
159320dfa0 |
Name a finished file after the job that made it
Six places decided what a converted or joined file should be called, and not one of them asked
the job. `JoinViewModel` reported `JoinState.Saved("joined.mp4")` whatever the format;
`JoinScreen` opened `CreateDocument("video/mp4")` and launched it with `"joined.mp4"`. On the
convert side `save()` and `suggestedOutputName()` built the name from `_settings.value.spec`, and
the screen took the MIME type from the same place -- the picker as it stands *now*, which is not
the spec the job ran with.
All six are right today, and all six are right by accident. The join screen has no format picker,
so the three MP4 literals agree with `ConcatWorker.request`'s default. The conversion pickers are
drawn only in the `Ready` state, so the settings cannot move between enqueue and save. Neither
accident is load-bearing anywhere it is written down.
One of them has already stopped holding, quietly. A job picked up by `reattach()` ran with a spec
that was never in this ViewModel's settings, because those settings belong to a process that no
longer exists -- so a reattached MP3 conversion is offered `.mp4` and `video/mp4` today. The
previous commit made that path more reachable rather than less: `Reattachment` exists precisely
to find work this ViewModel did not start.
The fix is to ask the only thing that knows. The spec travels to the worker as input `Data` and
`WorkInfo` hands input `Data` back to nobody, so the worker is the single point at which the
input's name and the spec that ran are both in scope. Both workers now report the two derived
strings -- the name to suggest and the type to open the dialog with -- in their output `Data`,
and they ride on `ConversionState.Converted` and `JoinState.Joined` from there. `save()` reports
what it saved rather than recomputing it, and both screens read the name and the MIME type off
the state they already collect, remembering their `CreateDocument` contract against that type
instead of a literal. The MIME type is not cosmetic: some providers rewrite a document's
extension to match it, so an MP3 offered as `video/webm` can arrive with the wrong one.
`suggestedOutputName()` is deleted rather than repaired. With the answer on the state there is no
caller left for it, and an accessor recomputing the same string would only be a second place for
it to be wrong -- which is what it was.
`ConcatWorker` gains `DEFAULT_FORMAT` and `outputNameFor(format)`. Three copies of "MP4" is the
shape this entry is about, and the input-Data default, `request`'s parameter default and the
ViewModel's fallback for a job that predates this change are exactly three copies.
Work already in the queue carries neither string, and WorkManager keeps finished work for about a
week, so that is the ordinary case for a few days rather than a corner. Those fall back to the
old derivation, which is a guess -- but it is the same guess the app was already making, it is
confined to jobs enqueued before this commit, and for such a job there is genuinely nothing
better to hand. New work never reaches it. The join's fallback is not even a guess: the format
`ConcatWorker.request` has always defaulted to is the format such a job really used.
`reattach()`'s KDoc carried this as a known wart it was deliberately leaving alone. That
paragraph is now a description of the fix rather than of a defect.
Tested at both ends, since either alone would pass while the other was wrong.
`WorkerOutputNamingTest` runs the real worker on an MP3 job -- nothing like the default preset, so
a name built from the picker is visibly wrong rather than accidentally right -- and compares the
whole success `Result`, which is what makes a missing key fail rather than go unnoticed. The two
ViewModel tests drive a finished job and then move the picker, which is what a reattached job
amounts to from the ViewModel's point of view.
Restoring just the two name sources -- `save()`'s `outputNameFor(displayName, _settings.value.spec)`
and `JoinState.Saved("joined.mp4")` -- turns them red with
`expected:<holiday_converted.mp3> but was:<input_converted.webm>` and
`expected:<joined.mkv> but was:<joined.mp4>`. The convert side loses the extension *and* the name:
`_settings.value` had been moved to WebM, and the display name a reattached job cannot supply had
already fallen back to the placeholder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2a68f03134 |
Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.
The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.
`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.
Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.
The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.
`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.
Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:
- `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
its own". It now does -- for work enqueued from here on. The case is **kept**, because the
queue outlives the change: WorkManager holds finished work for about a week, and the jobs
likeliest to be sitting in it when this code first runs are the ones named the old way.
Behaviour is unchanged and `ReattachmentTest` is untouched.
- `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
from sharing a file, and say nothing about whether a file's job is still running, which is the
question a sweep actually asks. Same for `StagingSweep` and the note in
`LibreMediaConverterApp`.
- Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
what is still true of work already in the queue.
`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.
`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
dcdcbfd3af |
Let a cancelled conversion stay cancelled
Both workers' outer `catch (e: Throwable)` caught `CancellationException` along with everything else and answered it with a `Result`. `runMedia3OrFallBack` goes out of its way to rethrow cancellation rather than fall back to software, and then the catch above it converted it anyway. A coroutine that reports completion inside a scope which has already been cancelled is structured concurrency's one rule broken, and it is the kind of break that stays quiet: nothing downstream complains, and the next thing to hold a resource across that boundary is the thing that finds out. Nothing on screen disagreed today, which is why this is a low-severity entry rather than a bug report. WorkManager cancels the worker's coroutine through `WorkerWrapper.interrupt`, which cancels `workerJob` with a `WorkerStoppedException`; the surrounding `withContext(workerJob)` then throws that whatever the worker returned, and `launch()` resolves it as `ResetWorkerStatus`. The returned `Result` is read only when nothing stopped the worker at all. So the change is about the shape of the code rather than about a symptom. One behaviour does move, and it is worth naming rather than discovering later. A cancellation that is *not* WorkManager stopping us -- FFmpegKit reporting `ReturnCode.isCancel`, which cancels the continuation -- now leaves `doWork` as a cancellation, and `WorkerWrapper` resolves a self-cancelled worker as `Resolution.Failed()` with no output data instead of the `Result.failure(KEY_ERROR ...)` it used to build. Both ViewModels already fall back on blank output data, deliberately and with a test, so the user sees "Conversion failed." either way. That is also the honest answer: the only route to `isCancel` is a cancellation someone asked for. The `staged.delete()` on that path is kept, and moved into the new branch rather than left to the one below it. A cancelled attempt leaves a partial in staging, the next attempt starts `doWork()` from the top rather than resuming it, and this catch holds the only handle to the file. Reaching any of this from a JVM test needed one more thing: `doWork` called `MediaProbe.probe` directly, and it was the last caller bypassing `ConversionDependencies.probe`. FFprobe's loader throws a bare `java.lang.Error` with no native library present, so no unit test could reach a single line below it. The seam's own KDoc says this is what it is for -- coverage of the branches that only run when something goes wrong -- and the default is the same real probe, so the app and the instrumented tests are unchanged. `WorkerCancellationTest` drives the real worker through a real `WorkManager` to the software engine, forced with `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, so the test does not depend on a routing rule it is not about. The engine stub writes bytes before it throws, which is what makes the delete assertion mean something: a stub that only threw would let a missing `delete()` pass. Three cases -- cancellation propagates, cancellation still deletes, and an ordinary failure is still answered with a `Result` carrying its message, which is the half that would break if the rethrow were widened past cancellation. Before the fix the first of those failed with "cancellation must leave doWork as cancellation, not as a Result; got null" -- `runCatching` had nothing to report, because `doWork` had returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5c27801c77 |
Retry work the system refused to start, instead of failing it terminally
`ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated
reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute
transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with
no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the
system refused it:
WM-ForceStopRunnable: Found unfinished work, scheduling it.
ActivityManager: Background started FGS: Disallowed [callingPackage:
org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED]
WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException
WM-WorkerWrapper: Worker result FAILURE
WM-Processor: Processor 3d1c9862 executed; reschedule = false
`Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was
ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have
carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the
runtime guarantee once a service is running, not permission to start one.
The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its
throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the
`workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the
device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data`
of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed."
with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging.
`ConcatWorker` had the same shape.
So `setForeground` moves inside the `try` in both workers, and the staging handle is named just
above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it,
so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the
`try` for the same reason and had the same problem; it is now inside too. On a retry the staged
name is unchanged, so the delete on the way out collects the partial the killed attempt left
behind rather than orphaning it.
What to return was the real decision. `Result.retry()`, because the denial is about *when* the
job ran and not about the job: the file is fine, the settings are fine, and the one thing that
grants an app permission to start a foreground service is being in the foreground, which the
user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded
retry means a job nobody comes back for waking the device forever while the screen says "paused"
and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against
the default backoff rather than as a round number: exponential from
`WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS`
(5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can
act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones --
WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget
will fail on its first denial rather than getting ten of its own. That is accepted rather than
overlooked, and the timeout branch ignores the count entirely so the budget's own retries are
untouched.
The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate
a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason
*and* the cause, with precedence stated rather than left to branch order -- once something has
stopped the worker, the exception it was holding describes that stop and not a reason of its own.
The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its
`IllegalStateException` supertype: a muxer that was never started throws one of those too, and
matching the supertype would retry every ordinary failure for eight hours.
Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler
expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the
exemption it buys is for starting, and the quota it costs would be paid by every job.
Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for
the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must
not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout
ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect
actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground`
throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it
propagates the exception to the caller and `ListenableFuture.await()` unwraps the
`ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of
those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same
line the device logged.
Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting`
said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause,
where a denied restart is now the likelier of the two; and `Reattachment.choose` justified
excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is
the sentence this commit falsifies. The exclusion stands on its own and says so now.
The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through
a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh
attempt meets an empty screen rather than the message. What the bound reliably buys is the end of
the retrying.
Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind
it, which cannot distinguish the two causes that now reach it, and the old copy named only the
six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along"
covers both, and for a denied start it is not filler but the actual remedy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
07933c80c5 | Merge branch 'fix/native-boundary-guards' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
7db320018a |
Correct the JDK claim and decide the backup rules
D11's documentation and scaffold items, less the one row that belongs to another change stream. README's "Requires JDK 17+ (AGP 9 will not run on older)" was wrong twice over, and `f496291` already corrected the same claim in CLAUDE.md. The floor is not AGP's, and 17 is not what compiles anything: Gradle 9.7.1's own `SupportedJavaVersions` carries MINIMUM_CLIENT_JAVA_VERSION = 8 and MINIMUM_DAEMON_JAVA_VERSION = 17, and this repo then overrides the daemon upward to 25 in gradle-daemon-jvm.properties. So the honest statement is that the launcher floor is 8, the daemon is 25 whatever JAVA_HOME says, and the app's bytecode is 25 -- which is what `./gradlew --version` shows on this machine right now, launcher 21 against daemon 25. The data_extraction_rules TODO is filled in rather than deleted, because `android:allowBackup="true"` makes it a live question and the answer is not "nothing to say". The app stores nothing of its own -- no settings, no history -- so WorkManager's queue is the entire backup payload, and restoring it is wrong rather than merely useless: every row names a content:// grant and a cacheDir path that do not survive reaching another device, and cacheDir is not backed up at all. Since `ec969c4` the ViewModel queries WorkManager by tag on launch, so those rows would not sit inert either -- a fresh install would come up reattached to a job the user never ran on it. WorkManager declares no exclusion of its own, so nothing upstream prevents it. allowBackup stays true. The decision belongs in the rules file, where it is per-file and legible to whoever adds real user data later, rather than in an app-wide switch that would also turn off device-to-device transfer. Each file is named instead of excluding the "database" domain in one line. Lint's FullBackupContent detector skips an <exclude> that carries no path without checking it, so the one-line spelling could have silently protected nothing; the enumerated paths are ones the gate actually verifies, and they are present in the built APK's compiled resource. backup_rules.xml and android:fullBackupContent are deleted rather than filled in. That attribute is only read on Android 11 and lower and minSdk is 33, so it could never have applied here -- an equally empty template that, unlike the other one, had no live question behind it. Not touched: OutputPublisher's hasSpaceFor KDoc, which the audit lists under D11. That code belongs to a parked branch and another change stream. The stale com/example/androidmediaconverter package directory needs no commit: it is empty, and git has never tracked it because git cannot track an empty directory. Removed from the working copy directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97fdc49f01 |
Guard the native boundary against what it actually throws
D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
a6baf41866 | Merge branch 'fix/rotation-and-partial-publish' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
dbfc463c6d |
Delete the half-written destination instead of leaving it under the user's name
publish() streamed the staged file into the SAF destination with copyTo and had no
answer for a copy that failed partway. The destination volume filling up is the obvious
way in; a provider giving out mid-write is the other. Either way the bytes it had
managed stayed at the name the user picked, while the UI said "Could not save the file".
The user was left holding a truncated file they had just been told was never written,
and nothing in the app would ever tidy it up -- staging cleanup reaches
<cacheDir>/conversions and nowhere else, by design.
So a failed copy now deletes the document. The interesting part is not the delete, it is
what stops it, because removing a file the user already had would be a far worse defect
than the one being fixed.
Only a document URI. DocumentsContract.deleteDocument is the only delete this code has
any right to attempt and it is defined on document URIs, so isDocumentUri() gates it.
That is not a formality: it asks the package manager whether anything answers
ACTION_DOCUMENTS_PROVIDER for the authority, so a file:// path, a MediaStore item or a
content URI from an ordinary provider all fall out here untouched.
Only a destination that was empty when we started. The size is read BEFORE the stream
is opened -- opening for write truncates, so afterwards the question can no longer be
asked -- and the delete runs only when the answer was positively zero. Every
destination that reaches publish() today comes from the SAF CreateDocument contract, so
in practice it is a document this app created seconds earlier; but publish() cannot
verify that from a Uri, and a provider that hands back an EXISTING document for a name
the user re-picked would otherwise have its file deleted rather than merely truncated.
Truncated is bad. Gone is worse, and it is the user's file either way.
That guard fails towards doing nothing. A provider that does not report _size, a query
that returns no row, a resolver call that throws -- all of them land in "not known to
be empty", so the fix is conservative rather than universal: it will not clean up
behind such a provider, and it will not delete anything of theirs either. The defect is
closed for providers that answer a size query; ExternalStorageProvider backs its
documents with real files, so a freshly created one reports 0, but that is reasoning
about it rather than a run against it. Stated here rather than implied, because
"fixed" would overclaim what was verified.
The original failure is what the caller sees. Cleanup runs in its own runCatching and a
throw from it is attached to the original exception as a suppressed one. deleteDocument
reports failure two different ways -- false, or a rethrown RuntimeException -- and
neither is worth failing the save over, because the save has already failed.
ConversionViewModel.save() reports e.message, and "could not delete the half-written
file" is not what to tell someone whose disk just filled up.
Two smaller decisions in the control flow. The whole `use` is guarded, not just copyTo:
a close() that throws while flushing IS the disk-full case and it arrives after copyTo
has returned successfully, so guarding only the copy would miss exactly the failure this
commit is about. The cost is that a file whose every byte reached the provider before a
failing flush is deleted too, which is the right way round -- a flush that failed means
the bytes are not durably there. And openOutputStream's own failure is deliberately
OUTSIDE the guard: nothing has been written at that point, so there is nothing of ours to
remove.
Nothing else in the class moves. hasSpaceFor, discardStaged and sweepStaging are
untouched, and so is save(): the staged file is still deliberately kept on a failed save,
for the reason its own comment gives -- it may be the only copy of an hour of
transcoding, and now the destination genuinely does not have it either.
Seven tests, JVM, Robolectric. The failure has to be injected, which is what shapes them:
Robolectric's ShadowContentResolver consults its registered-stream map before it reaches
any provider, so a test can hand out a stream that writes 512 bytes to a real file and
then throws "No space left on device" while a fake provider answers the size query and
the delete against that same file. The provider is registered twice under two authorities
and two component names, once with the ACTION_DOCUMENTS_PROVIDER intent filter and once
without, which is the only way to have a content URI that is not a document URI. The
delete is observed by the wire names DocumentsContract.deleteDocument actually sends --
"android:deleteDocument" and the "uri" extra -- because both constants are hidden from
the public SDK.
fails partway leaves nothing RED before the fix: expected the delete, got []
close that fails while flushing RED before: the file was still there
cleanup that fails RED before: suppressed was []
already held bytes is not deleted green before the fix -- see below
not a document is left alone green before the fix -- see below
cannot be opened at all green before, and must stay so
succeeds, every byte, no delete green before, and must stay so
The last four are green against the unfixed code for a reason worth writing down: the old
publish() never deleted anything, so every guard passes trivially. They pin the guards
rather than the defect, and pinning is only worth something if the pin is real, so both
were reverted on their own:
- dropping `if (destinationWasEmpty)` fails "a destination that already held bytes is
not deleted" with "a document this app did not create must survive"
- dropping the isDocumentUri() check fails "a destination that is not a document is
left alone" with "deleteDocument has no business on a URI that is not a document
expected:<[]> but was:<[content://...test.plain/document/holiday_plain.mp4]>"
Both were restored. 193 unit tests green.
UnopenableUriTest's unwritable-destination case (an authority with no provider behind it)
is unchanged and keeps passing by two independent routes: the size query on a dead
authority yields "not known to be empty", and the failure itself comes out of
openOutputStream, which sits outside the guard. It is an instrumented test and was
compile-verified here, not executed -- instrumented tests do not run on this host
(CLAUDE.md). Its JVM twin is in the list above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
ce4d0ff7d4 |
Keep the selected tab through the recreation targetSdk 37 guarantees
AppRoot held the selected tab in `remember`, which survives recomposition and nothing
else. MainActivity declares no configChanges, so every rotation and every resize
destroys and recreates the Activity, and the tab went back to Convert each time.
The KDoc directly above that line is the argument for why it matters: from targetSdk 37
Android ignores screenOrientation, resizableActivity and the aspect-ratio limits on any
display at least 600dp wide, and the Android 16 opt-out is gone, so the app is resized
and rotated whether or not it is ready. The shell was written for that case and then
lost its own state to it. Both ViewModels are Activity-scoped and come back intact, so
a conversion in flight was never at risk -- only the tab, which is what makes this
visibly wrong rather than merely stale.
rememberSaveable, with a Saver that writes the constant's NAME. Three ways to make an
enum saveable and the reasons for this one:
- autoSaver already accepts it. An enum is Serializable, so plain
`rememberSaveable { mutableStateOf(Destination.CONVERT) }` compiles, works, and
passes the restoration test below unchanged. That is a reason to be explicit, not a
reason not to be: nothing in the declaration says Destination has to stay
Serializable, so the implicit route keeps working right up until someone makes it a
value class or a sealed interface -- and then stops, silently, on a path only a
rotation reaches.
- The ordinal is a position, not an identity. Inserting a tab between Convert and Join
would redefine every value already written down. A name only changes when someone
renames a constant, which is an edit that shows up in a diff. It also reads as
itself in a Bundle dump.
- An unknown name restores to null, which rememberSaveable treats as "nothing saved"
and falls back to Convert. That is exactly what a downgrade or a renamed constant
leaves behind, and Convert is the right answer for it.
The test runs on the JVM, which took two changes to reach.
AppRoot and Destination are `internal` rather than `private` -- the unit test source set
is a friend of main, so this stays invisible outside the module -- and AppRoot takes its
`content` as a defaulted parameter instead of calling Content() directly. Nothing in the
app passes it. It is there because both screens resolve a ViewModel, which builds a
WorkManager and a media probe, and none of that has anything to do with which tab is
selected; the test hands in a tagged Box and drives the shell alone. Content() stays
private and is still what the app gets.
compose-ui-test-junit4 joins the JVM test source set. It was already in the catalog for
androidTest, it is inside the prerelease guard via its androidx. group, and its version
comes from the BOM, so this adds no new pinning argument. It is there because
createComposeRule() runs under Robolectric: a red test in androidTest is one nobody on
this host can execute (CLAUDE.md), which is not a loop anyone can work in.
ui-test-manifest is NOT repeated on that source set. It supplies the ComponentActivity
the rule launches, and the existing debugImplementation entry already puts it in the
merged manifest the unit tests build against -- checked by removing the line and
watching AppRootRestorationTest stay green, rather than assumed.
What the test does and does not prove. StateRestorationTester's
emulateSavedInstanceStateRestore() disposes the composition and rebuilds it, so anything
held only by `remember` is gone -- that is what makes it bite. It saves into an in-memory
map rather than parcelling through a Bundle, so it cannot tell a name from an ordinal
from autoSaver. The saved representation is pinned separately by three pure-JVM tests
over the Saver itself, which is where the choice above is actually held down.
Verified by writing it red first, against the restructured AppRoot with `remember` still
in place: all three restoration tests failed at the post-restore assertion with
"Expected exactly '1' node but could not find any node that satisfies:
(TestTag = 'content:JOIN')", while every assertion before the restore -- including the
bar's own selected state -- passed. Six new tests, 186 green in all.
Not addressed here, and deliberately: MainActivity still declares no configChanges, and
should not. Handling the configuration change is not the same as keeping one enum, and
Compose's saved-state machinery is the mechanism the platform intends for it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
8bc2a337d0 |
Merge the staging-cleanup fix, and pin the seam the two changes share
The two changes meet at one line. `pendingStaged` is recorded in the `SUCCEEDED` branch of each ViewModel's `observe()` collector, and reattachment reaches `Converted`/`Joined` through that same collector rather than by building the state itself -- so a job picked up from a previous process arrives with its cleanup handle already set, and "Start over" on it deletes the staged file exactly as it does for a conversion run in this process. Nothing had to be added for that; it falls out of routing reattachment through `observe()`. Which is precisely why it needed a test. The claim is structural -- one assignment, in one function, that both changes assume -- and the conflict here was `JoinViewModel`, where the cleanup change edits a collector body that the reattachment change had moved out of `join()` into a private `observe()`. Resolving that by putting the assignment back in `join()` would compile, pass every test either branch brought, and silently leak a full-size file on the one path both changes were written for. `ReattachedCleanupTest` fails if it lands anywhere else. Verified the way the cleanup commit verified its own wiring: deleting `pendingStaged = staged` from `ConversionViewModel.observe()` fails "start over on a reattached conversion deletes the staged file" alongside the two `ConversionViewModelCleanupTest` cases that share the line. Restored, all 183 JVM tests pass. The reattachment path also gains JVM coverage it could not have had before this merge, since Robolectric and the `ConversionDependencies` seams arrived with it: a ViewModel constructed after a job has already finished, with nothing left that observed it, now demonstrably reaches `Converted` with the display name recovered from the job's tags -- on a machine where no instrumented test can run. Conflict resolution: both sides kept in `JoinViewModel`, with the cleanup handle declared alongside the other fields and the reattachment `init` after them. Nothing else conflicted; `ConversionViewModel` merged clean because the reattachment change touches the `CANCELLED` and `FAILED` branches while the cleanup change touches `SUCCEEDED`. No build file, manifest or `OutputPublisher` line in this merge is mine -- they arrive from the cleanup commit verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
eb37f9ebea |
Merge branch 'fix/reattach-unfinished-work' into scratch/integrate-d2-d3
# Conflicts: # app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt |