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

MediaProbe: three pure helpers have no test, while the rest is device-bound and fine #84

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

Filed from a coverage read on ad28293, but the number is not the reason — see the last section.

MediaProbe is 75/145 lines on the JVM. What is missing splits cleanly into two halves, and only one of them is a gap worth closing.

Genuinely untested, and unit-testable today

function missed why it is testable
shortName(mime) 14/14 pure when over MediaFormat.MIMETYPE_* constants -> short names, with mime.substringAfter('/') as the fallback
isImageFormat(formatName) 2/2 pure string logic: splits on ,, matches image2 or a _pipe suffix
intOr 1/1 pure

All three are private. #57 established the precedent for widening exactly this way — internal, with the JVM test source set as a friend of main (MainActivity.kt carries the reasoning).

shortName is a lookup table over Android MIME constants, which is the shape that rots silently: nothing today would notice a wrong or missing arm. isImageFormat's _pipe suffix rule is the kind of thing that looks arbitrary until it breaks.

Device- or native-bound, and NOT a gap

readMediaInformation (14/14) spawns FFprobe; probeWithExtractor (13/25) drives MediaExtractor; probe, probeWithFFprobe and probeForConcat are the orchestration over those. These are exercised by RemuxTest, ConcatEngineTest and RealMediaBenchmark in androidTest — which JaCoCo does not measure at all, because the report covers testDebugUnitTest only.

Do not read their 0% as untested, and do not try to fix it by mocking FFprobe. MediaProbeFormatTest and MediaProbeNativeLoadTest already cover the parsing and the native-failure edge at the seams that exist.

Done means

The three pure helpers have tests that name their cases — every arm of shortName including the fallback, and both halves of isImageFormat's rule.

Mutation: delete one arm of shortName (say MIMETYPE_VIDEO_HEVC -> "hevc") and the test must go red naming that mime; change endsWith("_pipe") to contains("pipe") and the isImageFormat test must go red.

On the coverage number

This will move it barely at all — 17 lines of 2246. That is not the point and must not become the acceptance. JaCoCo was found on 2026-08-24 never to have counted a Robolectric test in this repo (#75, fixed in #76), and #52's entire premise turned out to be an artifact of that. The reason to write these is that three lookup tables have no test, not that a percentage moves.

_Filed from a coverage read on `ad28293`, but **the number is not the reason** — see the last section._ `MediaProbe` is 75/145 lines on the JVM. What is missing splits cleanly into two halves, and only one of them is a gap worth closing. ### Genuinely untested, and unit-testable today | function | missed | why it is testable | |---|---|---| | `shortName(mime)` | **14/14** | pure `when` over `MediaFormat.MIMETYPE_*` constants -> short names, with `mime.substringAfter('/')` as the fallback | | `isImageFormat(formatName)` | **2/2** | pure string logic: splits on `,`, matches `image2` or a `_pipe` suffix | | `intOr` | **1/1** | pure | All three are `private`. #57 established the precedent for widening exactly this way — `internal`, with the JVM test source set as a friend of `main` (`MainActivity.kt` carries the reasoning). `shortName` is a **lookup table over Android MIME constants**, which is the shape that rots silently: nothing today would notice a wrong or missing arm. `isImageFormat`'s `_pipe` suffix rule is the kind of thing that looks arbitrary until it breaks. ### Device- or native-bound, and NOT a gap `readMediaInformation` (14/14) spawns FFprobe; `probeWithExtractor` (13/25) drives `MediaExtractor`; `probe`, `probeWithFFprobe` and `probeForConcat` are the orchestration over those. These are exercised by `RemuxTest`, `ConcatEngineTest` and `RealMediaBenchmark` in `androidTest` — **which JaCoCo does not measure at all**, because the report covers `testDebugUnitTest` only. Do not read their 0% as untested, and do not try to fix it by mocking FFprobe. `MediaProbeFormatTest` and `MediaProbeNativeLoadTest` already cover the parsing and the native-failure edge at the seams that exist. ### Done means The three pure helpers have tests that name their cases — every arm of `shortName` including the fallback, and both halves of `isImageFormat`'s rule. **Mutation:** delete one arm of `shortName` (say `MIMETYPE_VIDEO_HEVC -> "hevc"`) and the test must go red naming that mime; change `endsWith("_pipe")` to `contains("pipe")` and the `isImageFormat` test must go red. ### On the coverage number This will move it barely at all — 17 lines of 2246. **That is not the point and must not become the acceptance.** JaCoCo was found on 2026-08-24 never to have counted a Robolectric test in this repo (#75, fixed in #76), and #52's entire premise turned out to be an artifact of that. The reason to write these is that three lookup tables have no test, not that a percentage moves.
JMR-dev commented 2026-08-25 03:49:31 +00:00 (Migrated from github.com)
Copy Link
Copy Source

#87 and #74 landed together in PR #90, and it leaves a join point here rather than a fifth table.

MediaProbe.kt is untouched — that is this ticket's file, deliberately left alone. What changed
underneath it:

  • CodecNames.videoFromName/audioFromName are no longer when expressions. The vocabulary is
    now internal val CodecNames.VIDEO_ALIASES: Map<String, VideoCodec> and
    CodecNames.AUDIO_ALIASES: Map<String, AudioCodec>. That is the load-bearing part: a when
    cannot be enumerated, so no test could ask one table what another one knows. A map can.
  • app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt is the cross-check. It
    walks CodecNames.VIDEO_ALIASES.keys against AndroidDeviceCodecs.NAME_TO_MIME.keys in both
    directions, and asserts the two agree on each name's meaning, not merely that both know it.
    Adding an alias to one side alone fails it — measured, with the whole 386-test suite green
    except that file.

shortName is the third table and the one this ticket owns. The cheap way to bring it in is an
arm in CodecVocabularyTest, not a table of its own: every short name shortName can emit must
be a key in CodecNames.VIDEO_ALIASES or AUDIO_ALIASES, or be listed as a documented
exception the way AndroidDeviceCodecs.DECODE_ONLY_NAMES lists mpeg4. That exception list is
itself checked in both directions in #90, because otherwise it is an escape hatch — any future
divergence could be waved through by adding the name to it.

Two things worth copying rather than reinventing:

  • AndroidDeviceCodecs.mimeFor, mimeForCodecName, NAME_TO_MIME and DECODE_ONLY_NAMES were
    widened from private to internal so the JVM test source set (a friend of main) can reach
    them. MainActivity.kt:36-38 carries the precedent and #57 established it.
  • CodecVocabularyTest asserts one literal MIME ("video/avc") on purpose. Without it, if the
    MediaFormat constants ever stopped being inlined into the unit-test classpath, every MIME
    comparison would be null == null and green.
#87 and #74 landed together in PR #90, and it leaves a join point here rather than a fifth table. `MediaProbe.kt` is untouched — that is this ticket's file, deliberately left alone. What changed underneath it: - `CodecNames.videoFromName`/`audioFromName` are no longer `when` expressions. The vocabulary is now `internal val CodecNames.VIDEO_ALIASES: Map<String, VideoCodec>` and `CodecNames.AUDIO_ALIASES: Map<String, AudioCodec>`. **That is the load-bearing part**: a `when` cannot be enumerated, so no test could ask one table what another one knows. A map can. - `app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt` is the cross-check. It walks `CodecNames.VIDEO_ALIASES.keys` against `AndroidDeviceCodecs.NAME_TO_MIME.keys` in both directions, and asserts the two agree on each name's *meaning*, not merely that both know it. Adding an alias to one side alone fails it — measured, with the whole 386-test suite green except that file. `shortName` is the third table and the one this ticket owns. The cheap way to bring it in is an arm in `CodecVocabularyTest`, not a table of its own: **every short name `shortName` can emit must be a key in `CodecNames.VIDEO_ALIASES` or `AUDIO_ALIASES`**, or be listed as a documented exception the way `AndroidDeviceCodecs.DECODE_ONLY_NAMES` lists `mpeg4`. That exception list is itself checked in both directions in #90, because otherwise it is an escape hatch — any future divergence could be waved through by adding the name to it. Two things worth copying rather than reinventing: - `AndroidDeviceCodecs.mimeFor`, `mimeForCodecName`, `NAME_TO_MIME` and `DECODE_ONLY_NAMES` were widened from `private` to `internal` so the JVM test source set (a friend of `main`) can reach them. `MainActivity.kt:36-38` carries the precedent and #57 established it. - `CodecVocabularyTest` asserts one literal MIME (`"video/avc"`) on purpose. Without it, if the `MediaFormat` constants ever stopped being inlined into the unit-test classpath, every MIME comparison would be `null == null` and green.
JMR-dev commented 2026-08-27 03:51:30 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Revisiting the boundary this ticket drew — with new evidence, not a re-reading of the old.

This ticket closed on:

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.

Two claims there, and they have aged differently.

Still exactly right: the FFprobe half, and the measurement boundary. readMediaInformation is native, JaCoCo measures testDebugUnitTest only, and mocking FFprobe would have been the wrong fix. Nothing here touches any of that.

Not right: that the extractor half is only orchestration. It is a branch matrix — first-track-wins per type, maxOf duration across tracks, the containsKey(KEY_DURATION) guard, and track ordering. RemuxTest and ConcatEngineTest reach those lines, but only through whatever the committed fixtures happen to contain, so none of the rules is chosen by any test. A two-video-track file, a track that omits its duration, or an audio-before-video ordering is not something a device test produces on purpose.

Addressed in #141 / PR #150 by cutting the walk into extractedFrom(List<MediaFormat>) and concatInputFrom(List<MediaFormat>) — the pure-seam pattern work/FailureOutcome.kt documents — rather than by mocking anything. What is left needing a device is setDataSource / getTrackFormat / release, three lines, which is the thin edge androidTest should be covering. Eleven tests, six mutations, all six red. MediaProbe's missed branches went 91 → 70.

Recorded here rather than only in the PR because this ticket is what a future coverage read will find first, and re-deriving the distinction cost a full spike. The ShadowMediaExtractor route was also checked and is genuinely available — Robolectric 4.16.1 shadows the exact setDataSource(Context, Uri, Map) overload — so if the seam is ever reverted, that is the fallback rather than a dead end.

Leaving this closed; #141 carries the work.

**Revisiting the boundary this ticket drew** — with new evidence, not a re-reading of the old. This ticket closed on: > `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. Two claims there, and they have aged differently. **Still exactly right:** the FFprobe half, and the measurement boundary. `readMediaInformation` is native, JaCoCo measures `testDebugUnitTest` only, and mocking FFprobe would have been the wrong fix. Nothing here touches any of that. **Not right:** that the extractor half is only orchestration. It is a branch matrix — first-track-wins per type, `maxOf` duration across tracks, the `containsKey(KEY_DURATION)` guard, and track ordering. `RemuxTest` and `ConcatEngineTest` reach those lines, but only through whatever the committed fixtures happen to contain, so **none of the rules is chosen by any test**. A two-video-track file, a track that omits its duration, or an audio-before-video ordering is not something a device test produces on purpose. Addressed in #141 / PR #150 by cutting the walk into `extractedFrom(List<MediaFormat>)` and `concatInputFrom(List<MediaFormat>)` — the pure-seam pattern `work/FailureOutcome.kt` documents — rather than by mocking anything. What is left needing a device is `setDataSource` / `getTrackFormat` / `release`, three lines, which is the thin edge `androidTest` should be covering. Eleven tests, six mutations, all six red. `MediaProbe`'s missed branches went 91 → 70. Recorded here rather than only in the PR because this ticket is what a future coverage read will find first, and re-deriving the distinction cost a full spike. **The `ShadowMediaExtractor` route was also checked and is genuinely available** — Robolectric 4.16.1 shadows the exact `setDataSource(Context, Uri, Map)` overload — so if the seam is ever reverted, that is the fallback rather than a dead end. Leaving this closed; #141 carries the work.
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 confirmed 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#84