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

Decide whether the workers getForegroundInfo has a seam worth cutting, or record that it does not #88

New Issue
Closed
opened 2026-08-25 03:22:09 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-08-25 03:22:09 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Filed from a coverage read on ad28293. Both workers miss the same method, which is what makes it worth one ticket rather than two.

getForegroundInfo is 5/5 missed in both workers

work/ConversionWorker.kt and work/ConcatWorker.kt each report 0 of 5 lines on it. Also missed: ConversionWorker.runMedia3OrFallBack (11/11) and about half of ConcatWorker.doWork (15/39).

Be careful what you conclude from that

The workers are heavily tested — sixteen JVM test classes touch ConversionWorker, plus six in androidTest, and ConversionWorkerTest drives a real setForeground with a foreground-service type on a device. This is not neglected code.

getForegroundInfo is what WorkManager calls to promote the job to a foreground service. It is reachable on a device and work/FailureOutcome.kt's KDoc already explains why the surrounding cases resist testing:

A refused foreground-service start needs a process death, a WorkManager recovery and a real system to do the refusing.

So the honest question is not "why is this 0%" but "is there a seam here, or is this genuinely the thin edge the pure-seam pattern leaves behind?"

What to actually decide

  1. Read getForegroundInfo in both workers. If it makes a decision — picking a notification, choosing a service type by API level, handling a null — that decision is a pure seam and belongs in a tested function, the way FailureOutcome already isolates the retry-vs-fail rule.
  2. If it only assembles a ForegroundInfo from values decided elsewhere, say so and close this: five lines of construction with no branch is the thin edge, and ProgressNotificationTest plus DeniedForegroundStartTest already cover the decisions feeding it.
  3. runMedia3OrFallBack (11/11) deserves the same read — ForcedFailureTest and HardwareFallbackTest drive its fallback on a device, so check whether the choice is separable from the execution.

Either outcome is a valid close. What is not valid is mocking WorkManager to make the number move.

Done means

Either a pure seam extracted and tested with a named mutation, or a written finding on this ticket saying which of the three sites have no seam worth cutting and why — recorded so the next coverage read does not re-open the same question.

Not the acceptance

The coverage number. JaCoCo did not count Robolectric here until #76, #52's premise was an artifact of that, and CLAUDE.md says plainly: the norm is behaviours having tests that bite, not a percentage rising.

_Filed from a coverage read on `ad28293`. Both workers miss the same method, which is what makes it worth one ticket rather than two._ ### `getForegroundInfo` is 5/5 missed in both workers `work/ConversionWorker.kt` and `work/ConcatWorker.kt` each report **0 of 5 lines** on it. Also missed: `ConversionWorker.runMedia3OrFallBack` (11/11) and about half of `ConcatWorker.doWork` (15/39). ### Be careful what you conclude from that The workers are **heavily** tested — sixteen JVM test classes touch `ConversionWorker`, plus six in `androidTest`, and `ConversionWorkerTest` drives a real `setForeground` with a foreground-service type on a device. This is not neglected code. `getForegroundInfo` is what WorkManager calls to promote the job to a foreground service. It is reachable on a device and `work/FailureOutcome.kt`'s KDoc already explains why the surrounding cases resist testing: > A refused foreground-service start needs a process death, a WorkManager recovery and a real system to do the refusing. So the honest question is not "why is this 0%" but **"is there a seam here, or is this genuinely the thin edge the pure-seam pattern leaves behind?"** ### What to actually decide 1. Read `getForegroundInfo` in both workers. If it makes a **decision** — picking a notification, choosing a service type by API level, handling a null — that decision is a pure seam and belongs in a tested function, the way `FailureOutcome` already isolates the retry-vs-fail rule. 2. If it only assembles a `ForegroundInfo` from values decided elsewhere, **say so and close this**: five lines of construction with no branch is the thin edge, and `ProgressNotificationTest` plus `DeniedForegroundStartTest` already cover the decisions feeding it. 3. `runMedia3OrFallBack` (11/11) deserves the same read — `ForcedFailureTest` and `HardwareFallbackTest` drive its fallback on a device, so check whether the *choice* is separable from the *execution*. **Either outcome is a valid close.** What is not valid is mocking WorkManager to make the number move. ### Done means Either a pure seam extracted and tested with a named mutation, or a written finding on this ticket saying which of the three sites have no seam worth cutting and why — recorded so the next coverage read does not re-open the same question. ### Not the acceptance The coverage number. JaCoCo did not count Robolectric here until #76, #52's premise was an artifact of that, and `CLAUDE.md` says plainly: the norm is behaviours having tests that bite, not a percentage rising.
JMR-dev commented 2026-08-25 03:35:03 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Investigated. The answer is outcome (2): there is no seam worth cutting here. Closing with the evidence, so the next coverage read does not re-open it.

ConcatWorker.getForegroundInfo — no decision at all

override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo(
    NOTIFICATION_ID,
    notifications.build(id, "Joining files", 0, indeterminate = true),
    ConversionForegroundType.current(),
)

A constant title, a constant 0, a constant flag. Five lines of construction with no branch. There is nothing here a pure function could hold.

ConversionWorker.getForegroundInfo — one fallback, and a delegation

override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
    inputData.getString(KEY_DISPLAY_NAME) ?: "input",
    percent = 0,
    indeterminate = true,
)

The only decision is ?: "input" when the display name is missing from input Data. Everything else is assembly. notifications.build(...) is covered by ProgressNotificationTest; the foreground-denial rules live in FailureOutcome and are covered by DeniedForegroundStartTest and FailureOutcomeTest.

The real decision was already extracted — and already tested

ConversionForegroundType.current() is the seam this ticket was looking for. Three API regimes: MEDIA_PROCESSING from 35, DATA_SYNC from 34, 0 at 33.

It has a test, and a good one — ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel:

val expected = when {
    Build.VERSION.SDK_INT >= 35 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING
    Build.VERSION.SDK_INT >= 34 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC
    else -> 0
}
assertEquals(expected, ConversionForegroundType.current())

Two things make it bite rather than tautologise:

  • It derives the expectation from the running API instead of pinning one value, so the same test is meaningful at every level — and CI runs 33/34/35/36/37, so all three regimes are exercised across the matrix rather than one being picked arbitrarily.
  • It spells the thresholds as literals (>= 35, >= 34) while production spells them as named constants (VANILLA_ICE_CREAM, UPSIDE_DOWN_CAKE). Moving a production threshold therefore reddens it rather than moving with it. That independence is what separates a mirror test from a real one.

So the 0% was a measurement boundary again

ConversionForegroundType is reached only from androidTest, and JaCoCo measures testDebugUnitTest only. Same shape as Media3Engine and AndroidDeviceCodecs.probe in #85 and #86: the report cannot see the tests that cover it.

runMedia3OrFallBack (11/11 missed) is the same story — its decision is isCancellation(e), covered by WorkerCancellationTest on the JVM, and the hardware-to-FFmpeg fallback itself by HardwareFallbackTest and ForcedFailureTest on a device.

Named exemption, deliberately not fixed

inputData.getString(KEY_DISPLAY_NAME) ?: "input" has no test. It is a one-line fallback for a key the worker's own request() always writes, and its only consequence is the title on a notification that is replaced by the first progress tick a second later. Testing it would need a hand-built Data missing a key nothing omits. Naming it rather than covering it — implied coverage is the problem this repo cares about, not a gap that is written down.

What would reopen this

A getForegroundInfo that starts branching — different notifications per engine, a service type chosen from something other than the API level, or a title derived rather than passed. None of those is true today.

Investigated. **The answer is outcome (2): there is no seam worth cutting here.** Closing with the evidence, so the next coverage read does not re-open it. ### `ConcatWorker.getForegroundInfo` — no decision at all ```kotlin override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo( NOTIFICATION_ID, notifications.build(id, "Joining files", 0, indeterminate = true), ConversionForegroundType.current(), ) ``` A constant title, a constant 0, a constant flag. Five lines of construction with no branch. There is nothing here a pure function could hold. ### `ConversionWorker.getForegroundInfo` — one fallback, and a delegation ```kotlin override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo( inputData.getString(KEY_DISPLAY_NAME) ?: "input", percent = 0, indeterminate = true, ) ``` The only decision is `?: "input"` when the display name is missing from input `Data`. Everything else is assembly. `notifications.build(...)` is covered by `ProgressNotificationTest`; the foreground-denial rules live in `FailureOutcome` and are covered by `DeniedForegroundStartTest` and `FailureOutcomeTest`. ### The real decision was already extracted — and already tested `ConversionForegroundType.current()` is the seam this ticket was looking for. Three API regimes: `MEDIA_PROCESSING` from 35, `DATA_SYNC` from 34, `0` at 33. **It has a test, and a good one** — `ConversionWorkerTest.foregroundTypeMatchesTheRunningApiLevel`: ```kotlin val expected = when { Build.VERSION.SDK_INT >= 35 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING Build.VERSION.SDK_INT >= 34 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC else -> 0 } assertEquals(expected, ConversionForegroundType.current()) ``` Two things make it bite rather than tautologise: - It **derives** the expectation from the running API instead of pinning one value, so the same test is meaningful at every level — and CI runs 33/34/35/36/37, so all three regimes are exercised across the matrix rather than one being picked arbitrarily. - It spells the thresholds as **literals** (`>= 35`, `>= 34`) while production spells them as **named constants** (`VANILLA_ICE_CREAM`, `UPSIDE_DOWN_CAKE`). Moving a production threshold therefore reddens it rather than moving with it. That independence is what separates a mirror test from a real one. ### So the 0% was a measurement boundary again `ConversionForegroundType` is reached only from `androidTest`, and JaCoCo measures `testDebugUnitTest` only. Same shape as `Media3Engine` and `AndroidDeviceCodecs.probe` in #85 and #86: the report cannot see the tests that cover it. `runMedia3OrFallBack` (11/11 missed) is the same story — its decision is `isCancellation(e)`, covered by `WorkerCancellationTest` on the JVM, and the hardware-to-FFmpeg fallback itself by `HardwareFallbackTest` and `ForcedFailureTest` on a device. ### Named exemption, deliberately not fixed `inputData.getString(KEY_DISPLAY_NAME) ?: "input"` has no test. It is a one-line fallback for a key the worker's own `request()` always writes, and its only consequence is the title on a notification that is replaced by the first progress tick a second later. Testing it would need a hand-built `Data` missing a key nothing omits. **Naming it rather than covering it** — implied coverage is the problem this repo cares about, not a gap that is written down. ### What would reopen this A `getForegroundInfo` that starts branching — different notifications per engine, a service type chosen from something other than the API level, or a title derived rather than passed. None of those is true today.
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#88