JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

Three sites need a seam cut before their branches can be tested, and one closed boundary that new evidence revises #133

New Issue
Closed
opened 2026-08-27 02:21:02 +00:00 by JMR-dev · 2 comments
JMR-dev commented 2026-08-27 02:21:02 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Filed from a coverage read on main @ dc8b7c3, 2026-08-26. Companion to #132, the seven-item JVM-gap ticket from the same read, which holds everything testable without new seam work; the code findings are docs/coverage-read-findings.md (PR #131).

Three sites cannot be tested as they stand. Two need a seam cut. One needs a boundary re-decided, because evidence found in this read contradicts what a closed ticket recorded — and that is the item worth reading first.

Children

Decomposed 2026-08-27. All three are independent of each other; #142 and #143 touch the same file,
so take them in either order but not in parallel.

child site shape of the work
#141 — MediaProbe's track-walking loop is reachable on the JVM after all MediaProbe:114-132, :279-293 decide shadow vs pure seam, then write it — and correct #84
#142 — publish()'s null-return branch needs a seam OutputPublisher:174 one protected open fun openDestination
#143 — sweepStaging's re-read race has no seam OutputPublisher:267 one overridable listing call

#141 carries the decision; the other two are small and mechanical once their seam is cut.

1. MediaProbe's extractor half — this revises #84's boundary

app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:114-132   probeWithExtractor
app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:279-293   probeForConcat

#84 closed by classifying these as device-bound and explicitly not a gap:

probeWithExtractor (13/25) drives MediaExtractor … These are exercised by RemuxTest, ConcatEngineTest and RealMediaBenchmark in androidTest — which JaCoCo does not measure at all. Do not read their 0% as untested, and do not try to fix it by mocking FFprobe.

That was right about FFprobe and right about the measurement boundary. It is not right that these are only orchestration, and the reachability half of it is now falsified.

Reachability — verified, not assumed. Robolectric 4.16.1 ships ShadowMediaExtractor, and its shadowed methods include the exact overload MediaProbe calls:

protected void setDataSource(android.content.Context, android.net.Uri, java.util.Map<String,String>)
public static void addTrack(org.robolectric.shadows.util.DataSource, android.media.MediaFormat, byte[])
protected int getTrackCount()
protected android.media.MediaFormat getTrackFormat(int)

(read from shadows-framework-4.16.1.jar, already on the test classpath.) MediaProbe.kt:106 and :271 both call setDataSource(context, uri, null). The entry point is reachable on the JVM today.

Why it is worth reaching. The uncovered code is not plumbing — it is a branch matrix:

  • first-track-wins, per type (:120, :126, :281, :286) — a file with two video tracks must report the first, and video == null is what enforces it
  • maxOf duration across tracks (:117) — a container whose audio track is longer than its video track
  • the containsKey(KEY_DURATION) guard (:116) — a track that omits duration entirely, which MediaProbeTrackFieldsTest's KDoc already notes is common in real files
  • audio-before-video track ordering, which changes nothing and must be shown not to

RemuxTest reaches these only through whatever the committed fixtures happen to contain, so none of the four is chosen by any test. ShadowMediaExtractor.addTrack constructs each directly — a two-video-track file, a track with no duration key, an audio-first ordering — cases no fixture provides and no device test would provoke on purpose.

What to decide: whether to test through the shadow, or to cut the track-walking loop into a pure function over a List<MediaFormat> and test that (the work/FailureOutcome.kt pure-seam pattern). The second is more in keeping with the repo and makes the androidTest coverage the thin edge it should be. Either way, #84's boundary paragraph needs correcting, or the next read re-derives all of this.

Mutation: change video == null to true at :120 — a two-video-track test must go red.

2. OutputPublisher.publish — the null-return branch

app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:173-174

val out = context.contentResolver.openOutputStream(destination)
    ?: error("Could not open destination for writing: $destination")

UnopenableUriTest.publishingToAnUnwritableDestinationThrowsRatherThanSilentlySucceeding asserts only that something threw:

val failure = runCatching { publisher.publish(staged, bogus) }.exceptionOrNull()
assertTrue(..., failure != null)

A dead provider throws FileNotFoundException from inside openOutputStream; it does not return null. So that test passes through a different path and this line is untested — and the two are not interchangeable, because :177 decides whether to delete a partially-written destination and only one of them has written anything.

Seam needed: the ContentResolver call, so a test can force a null return specifically. publish is already open and OutputPublisher is already subclassed for tests (WorkerStubs.kt's AlwaysRoomPublisher / NamingPublisher), so the shape exists — it wants one protected open fun openDestination(uri: Uri): OutputStream? and nothing more.

3. OutputPublisher.sweepStaging — the re-read race

app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:267

if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete()

The second timestamp read, whose comment states exactly what it prevents: between the directory listing and this line, a worker resumed by WorkManager in this same process could have started writing this file, and unlinking an inode a running job holds open ends with the job reporting success for a path that no longer exists.

The false branch — a file that was collectable in the listing and is not by the time this runs — has never executed. StagingSweep.collectable is pure and well tested; this is the guard around it, and it is the one thing standing between the sweep and a live job's output.

Seam needed: something that can change a file's mtime between the listing and the re-read. A protected open fun entriesIn(dir: File) hook, or hoisting the listing into an overridable call, both do it without touching the delete logic.

Mutation: delete the if and delete unconditionally — the race test must go red.

Considered and deliberately not included: AndroidDeviceCodecs.probe()

Recorded so the next read does not repeat the spike.

probe() (codec/AndroidDeviceCodecs.kt:49-75) is 19 of 31 lines uncovered, and #86 closed by ruling it device-bound:

probe() queries the real MediaCodecList … That is the class's whole purpose and it cannot be answered on the JVM. ConversionRouterTest already tests the decisions against fabricated DeviceCodecs profiles, which is the right seam and is why forTesting exists.

Robolectric does in fact offer ShadowMediaCodecList.addCodec(MediaCodecInfo) plus MediaCodecInfoBuilder, which exposes setName, setIsEncoder, setIsVendor, setIsSoftwareOnly and setIsHardwareAccelerated. So the reachability objection is technically answerable — but it does not change the answer, for two reasons:

  1. The builder cannot drive the only interesting logic. It has no setIsAlias and no setCanonicalName, so the alias skip (:58) and the canonical-name dedup (:59) — the two things the class's KDoc calls out as easy to get wrong — are not reachable through it. What is reachable is the isHardwareAccelerated && !isSoftwareOnly filter and the video/non-video split, which is enumeration bookkeeping.
  2. #86's seam argument stands on its own. The decisions that depend on this run through DeviceCodecs, and ConversionRouterTest already drives 31 tests against fabricated profiles. A ShadowMediaCodecList test would cover the uninteresting half of an already-correct boundary.

#86 stays closed. This paragraph exists so the shadow spike is not run a third time.

Not the acceptance

The coverage number, for the reason CLAUDE.md and #84, #86 and #88 all give. A seam is worth cutting when it turns a device-bound behaviour into a decision a test can choose the inputs for — which is the case for all three above and is not the case for AndroidDeviceCodecs.probe(). A seam cut only to make a percentage move is worse than the uncovered line it replaces.

_Filed from a coverage read on `main` @ `dc8b7c3`, 2026-08-26. Companion to #132, the seven-item JVM-gap ticket from the same read, which holds everything testable **without** new seam work; the code findings are `docs/coverage-read-findings.md` (PR #131)._ Three sites cannot be tested as they stand. Two need a seam cut. One needs a **boundary re-decided**, because evidence found in this read contradicts what a closed ticket recorded — and that is the item worth reading first. ### Children Decomposed 2026-08-27. All three are independent of each other; **#142 and #143 touch the same file**, so take them in either order but not in parallel. | child | site | shape of the work | |---|---|---| | #141 — `MediaProbe`'s track-walking loop is reachable on the JVM after all | `MediaProbe:114-132`, `:279-293` | decide shadow vs pure seam, then write it — **and correct #84** | | #142 — `publish()`'s null-return branch needs a seam | `OutputPublisher:174` | one `protected open fun openDestination` | | #143 — `sweepStaging`'s re-read race has no seam | `OutputPublisher:267` | one overridable listing call | #141 carries the decision; the other two are small and mechanical once their seam is cut. --- ## 1. `MediaProbe`'s extractor half — this revises #84's boundary ``` app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:114-132 probeWithExtractor app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:279-293 probeForConcat ``` **#84 closed by classifying these as device-bound and explicitly not a gap:** > `probeWithExtractor` (13/25) drives `MediaExtractor` … These are exercised by `RemuxTest`, `ConcatEngineTest` and `RealMediaBenchmark` in `androidTest` — which JaCoCo does not measure at all. **Do not read their 0% as untested**, and do not try to fix it by mocking FFprobe. That was right about FFprobe and right about the measurement boundary. It is **not** right that these are only orchestration, and the reachability half of it is now falsified. **Reachability — verified, not assumed.** Robolectric 4.16.1 ships `ShadowMediaExtractor`, and its shadowed methods include the exact overload `MediaProbe` calls: ``` protected void setDataSource(android.content.Context, android.net.Uri, java.util.Map<String,String>) public static void addTrack(org.robolectric.shadows.util.DataSource, android.media.MediaFormat, byte[]) protected int getTrackCount() protected android.media.MediaFormat getTrackFormat(int) ``` (read from `shadows-framework-4.16.1.jar`, already on the test classpath.) `MediaProbe.kt:106` and `:271` both call `setDataSource(context, uri, null)`. The entry point is reachable on the JVM today. **Why it is worth reaching.** The uncovered code is not plumbing — it is a branch matrix: - **first-track-wins, per type** (`:120`, `:126`, `:281`, `:286`) — a file with two video tracks must report the first, and `video == null` is what enforces it - **`maxOf` duration across tracks** (`:117`) — a container whose audio track is longer than its video track - **the `containsKey(KEY_DURATION)` guard** (`:116`) — a track that omits duration entirely, which `MediaProbeTrackFieldsTest`'s KDoc already notes is common in real files - **audio-before-video track ordering**, which changes nothing and must be shown not to `RemuxTest` reaches these only through whatever the committed fixtures happen to contain, so none of the four is *chosen* by any test. `ShadowMediaExtractor.addTrack` constructs each directly — a two-video-track file, a track with no duration key, an audio-first ordering — cases no fixture provides and no device test would provoke on purpose. **What to decide:** whether to test through the shadow, or to cut the track-walking loop into a pure function over a `List<MediaFormat>` and test that (the `work/FailureOutcome.kt` pure-seam pattern). The second is more in keeping with the repo and makes the `androidTest` coverage the thin edge it should be. Either way, **#84's boundary paragraph needs correcting**, or the next read re-derives all of this. *Mutation:* change `video == null` to `true` at `:120` — a two-video-track test must go red. --- ## 2. `OutputPublisher.publish` — the null-return branch ``` app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:173-174 ``` ```kotlin val out = context.contentResolver.openOutputStream(destination) ?: error("Could not open destination for writing: $destination") ``` `UnopenableUriTest.publishingToAnUnwritableDestinationThrowsRatherThanSilentlySucceeding` asserts only that *something* threw: ```kotlin val failure = runCatching { publisher.publish(staged, bogus) }.exceptionOrNull() assertTrue(..., failure != null) ``` A dead provider throws `FileNotFoundException` from inside `openOutputStream`; it does not return null. So that test passes through a different path and this line is untested — and the two are not interchangeable, because `:177` decides whether to delete a partially-written destination and only one of them has written anything. **Seam needed:** the `ContentResolver` call, so a test can force a null return specifically. `publish` is already `open` and `OutputPublisher` is already subclassed for tests (`WorkerStubs.kt`'s `AlwaysRoomPublisher` / `NamingPublisher`), so the shape exists — it wants one `protected open fun openDestination(uri: Uri): OutputStream?` and nothing more. --- ## 3. `OutputPublisher.sweepStaging` — the re-read race ``` app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:267 ``` ```kotlin if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete() ``` The second timestamp read, whose comment states exactly what it prevents: between the directory listing and this line, a worker resumed by WorkManager **in this same process** could have started writing this file, and unlinking an inode a running job holds open ends with the job reporting success for a path that no longer exists. The false branch — a file that was collectable in the listing and is not by the time this runs — has never executed. `StagingSweep.collectable` is pure and well tested; this is the guard *around* it, and it is the one thing standing between the sweep and a live job's output. **Seam needed:** something that can change a file's mtime between the listing and the re-read. A `protected open fun entriesIn(dir: File)` hook, or hoisting the listing into an overridable call, both do it without touching the delete logic. *Mutation:* delete the `if` and delete unconditionally — the race test must go red. --- ## Considered and deliberately not included: `AndroidDeviceCodecs.probe()` Recorded so the next read does not repeat the spike. `probe()` (`codec/AndroidDeviceCodecs.kt:49-75`) is 19 of 31 lines uncovered, and **#86 closed by ruling it device-bound**: > `probe()` queries the real `MediaCodecList` … That is the class's whole purpose and it cannot be answered on the JVM. `ConversionRouterTest` already tests the *decisions* against fabricated `DeviceCodecs` profiles, which is the right seam and is why `forTesting` exists. Robolectric does in fact offer `ShadowMediaCodecList.addCodec(MediaCodecInfo)` plus `MediaCodecInfoBuilder`, which exposes `setName`, `setIsEncoder`, `setIsVendor`, `setIsSoftwareOnly` and `setIsHardwareAccelerated`. So the reachability objection is technically answerable — **but it does not change the answer**, for two reasons: 1. **The builder cannot drive the only interesting logic.** It has no `setIsAlias` and no `setCanonicalName`, so the alias skip (`:58`) and the canonical-name dedup (`:59`) — the two things the class's KDoc calls out as easy to get wrong — are not reachable through it. What *is* reachable is the `isHardwareAccelerated && !isSoftwareOnly` filter and the video/non-video split, which is enumeration bookkeeping. 2. **#86's seam argument stands on its own.** The decisions that depend on this run through `DeviceCodecs`, and `ConversionRouterTest` already drives 31 tests against fabricated profiles. A `ShadowMediaCodecList` test would cover the uninteresting half of an already-correct boundary. **#86 stays closed.** This paragraph exists so the shadow spike is not run a third time. --- ### Not the acceptance The coverage number, for the reason `CLAUDE.md` and #84, #86 and #88 all give. A seam is worth cutting when it turns a device-bound behaviour into a decision a test can *choose* the inputs for — which is the case for all three above and is not the case for `AndroidDeviceCodecs.probe()`. **A seam cut only to make a percentage move is worse than the uncovered line it replaces.**
JMR-dev commented 2026-08-27 04:03:15 +00:00 (Migrated from github.com)
Copy Link
Copy Source

All children are implemented, and the batch integrates. Verified locally rather than assumed, because eight PRs touching overlapping files is exactly where a clean-per-PR result stops meaning much.

Merged all eight branches onto current main in a throwaway branch:

  • no conflicts — the stack (#144 → #149 → #151) and the five independent branches merge cleanly in any order
  • 500 tests in 71 classes, 0 failures, 0 errors (baseline was 456 in 68)
  • full gate green on the integrated result: ktlintCheck, detekt, lintDebug, testDebugUnitTest, compileDebugAndroidTestKotlin
metric before integrated
line 84.9% (1971/2321) 87.1% (2024/2324)
branch 63.8% (900/1410) 69.0% (973/1410)

Branch moved more than line, which is what this batch was aimed at — nearly every test here targets a guard rather than a new code path.

Files this batch was about, after integration:

file missed lines missed branches
InputQuery.kt 0 3
OutputPublisher.kt 0 2
ContainerCapabilities.kt 0 12
MediaProbe.kt 43 72 (was 91)
ConcatWorker.kt 15 3 (was 4)
ConversionWorker.kt 22 14

What remains in those files is the native/device half and the named exemptions recorded in each PR — not unclaimed gaps.

Follow-up worth its own ticket, not folded in here: CLAUDE.md's coverage entry quotes 84.9%/63.8% and instructs re-measuring before quoting. It goes stale the moment this batch lands. Updating it now would be quoting a number that is not true of main yet, which is the exact failure that entry documents about itself.

**All children are implemented, and the batch integrates.** Verified locally rather than assumed, because eight PRs touching overlapping files is exactly where a clean-per-PR result stops meaning much. Merged all eight branches onto current `main` in a throwaway branch: - **no conflicts** — the stack (#144 → #149 → #151) and the five independent branches merge cleanly in any order - **500 tests in 71 classes, 0 failures, 0 errors** (baseline was 456 in 68) - full gate green on the integrated result: `ktlintCheck`, `detekt`, `lintDebug`, `testDebugUnitTest`, `compileDebugAndroidTestKotlin` | metric | before | integrated | |---|---|---| | line | 84.9% (1971/2321) | **87.1%** (2024/2324) | | branch | 63.8% (900/1410) | **69.0%** (973/1410) | Branch moved more than line, which is what this batch was aimed at — nearly every test here targets a guard rather than a new code path. Files this batch was about, after integration: | file | missed lines | missed branches | |---|---|---| | `InputQuery.kt` | **0** | 3 | | `OutputPublisher.kt` | **0** | 2 | | `ContainerCapabilities.kt` | **0** | 12 | | `MediaProbe.kt` | 43 | 72 (was 91) | | `ConcatWorker.kt` | 15 | 3 (was 4) | | `ConversionWorker.kt` | 22 | 14 | What remains in those files is the native/device half and the named exemptions recorded in each PR — not unclaimed gaps. **Follow-up worth its own ticket, not folded in here:** `CLAUDE.md`'s coverage entry quotes 84.9%/63.8% and instructs re-measuring before quoting. It goes stale the moment this batch lands. Updating it now would be quoting a number that is not true of `main` yet, which is the exact failure that entry documents about itself.
JMR-dev commented 2026-09-02 01:41:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

All three children closed. #142 and #143 closed on merge; #141 did not, for the reason recorded on #132 — its PR merged into a stack base rather than main, so its Closes keyword never fired.

#141 verified against main before closing by re-running its own named mutation. The ticket named video == null -> true at :120; that line moved when the seam was cut, so the equivalent guard in extractedFrom was dropped instead — and both the video and audio first-track-wins tests go red.

This parent's premise held up better than expected. All three seams were cut, and two of them exposed something a test alone would not have: #143's proposed seam turned out not to reach the branch it was for (the override fired before the snapshot, so collectable never saw the old mtime), and the seam had to move to snapshot(listing) before the race test would bite. That is recorded on #143.

The pattern has since carried into wave 2 (#153), where cutting seams for ConversionViewModel.observe and JoinViewModel.observe exposed a crash — ConcatStrategy::valueOf throwing inside a viewModelScope collect with no handler. Same shape as here: the defect was not hidden, it was unreadable in place.

**All three children closed.** #142 and #143 closed on merge; #141 did not, for the reason recorded on #132 — its PR merged into a stack base rather than `main`, so its `Closes` keyword never fired. #141 verified against `main` before closing by re-running its own named mutation. The ticket named `video == null -> true` at `:120`; that line moved when the seam was cut, so the equivalent guard in `extractedFrom` was dropped instead — and both the video and audio first-track-wins tests go red. This parent's premise held up better than expected. All three seams were cut, and **two of them exposed something a test alone would not have**: #143's proposed seam turned out not to reach the branch it was for (the override fired before the snapshot, so `collectable` never saw the old mtime), and the seam had to move to `snapshot(listing)` before the race test would bite. That is recorded on #143. The pattern has since carried into wave 2 (#153), where cutting seams for `ConversionViewModel.observe` and `JoinViewModel.observe` exposed a **crash** — `ConcatStrategy::valueOf` throwing inside a `viewModelScope` collect with no handler. Same shape as here: the defect was not hidden, it was unreadable in place.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels enhancement sev:low
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#133