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

W1: cut ConversionViewModel.observe's state mapping into a seam, and choose all six arms #154

New Issue
Closed
opened 2026-08-27 11:58:59 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-08-27 11:58:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

ConversionViewModel.observe maps a WorkInfo onto a ConversionState. That mapping is the app's main UI state machine, and no test chooses which arm it takes.

The claim, stated precisely

This is not cold code. ConversionViewModel$observe$1$1 reports 28 covered lines and 24 covered branches — the collect block runs on every test that drives a real worker through installTestWorkManager. But a real worker only ever produces a terminal state with well-formed output, so those are the only arms reached:

arm line(s) reached today?
SUCCEEDED with an output path 463–503 yes — real workers produce it
FAILED with a non-blank message 505–507 yes — real workers produce it
RUNNING, reading KEY_PROGRESS 446–448 no
ENQUEUED, runAttemptCount > 0 → Waiting 457–458 no
ENQUEUED, runAttemptCount == 0 → Converting 460 no
SUCCEEDED with a null output path 466 no
FAILED with a blank or missing message 508 no
CANCELLED 511 no
BLOCKED 512 no

grep for WorkInfo.State. across app/src/test finds all six constants, which looks like coverage until you see where they are: ReattachmentTest drives every one of them into Reattachment.choose, a different function. The other two files that mention them (ConverterStateAffordancesTest, FailedSaveRetryTest) name the FAILED arm in KDoc and do not construct a WorkInfo at all.

So the enqueued-means-retry rule is tested in one of its two homes. Reattachment.choose has a test for runAttemptCount = 0 versus 1; observe, which is what the user's screen actually reads, does not — and the comment above line 456 explains at length why the distinction matters. A rule with a documented rationale and two implementations should not have one of them untested.

Why a seam, and which one

There is nothing to inject. workManager is private val workManager = WorkManager.getInstance(app), hard-wired in the constructor at line 183, and observe is private. A test cannot hand this a chosen WorkInfo.

Take the same answer #141 took for MediaProbe: lift the when (info.state) out as an internal pure function over the values it actually reads, and leave the collect, the null check and the ownership check behind as the thin edge. internal rather than private for the reason MediaProbe.Extracted gives — the JVM test source set is a friend of main, so it stays invisible outside the module.

Two things the seam must not swallow:

  • The ownership check at 444 stays outside it. Its KDoc is explicit that it guards the SUCCEEDED arm's file ownership, not just the assignment. Moving it inside a pure mapper would change what it protects.
  • The SUCCEEDED arm takes ownership of the staged file (468 onward), which is a side effect. The mapper should return the decision; the caller performs it. If that split is awkward, say so on the ticket rather than fusing them — a pure function that deletes files is not the seam this asks for.

Done when

Each of the six arms is chosen by a test, and each is killed by a mutation of its own. Note ENQUEUED needs two tests, not one — a single test passes against a mapper that ignores runAttemptCount entirely.

Name any arm you decide is not worth pinning, and why. BLOCKED is the likeliest candidate: it maps to the same Converting as a fresh ENQUEUED, so a test for it asserts an equality rather than a rule.

`ConversionViewModel.observe` maps a `WorkInfo` onto a `ConversionState`. That mapping is the app's main UI state machine, and **no test chooses which arm it takes.** ## The claim, stated precisely This is not cold code. `ConversionViewModel$observe$1$1` reports 28 covered lines and 24 covered branches — the collect block runs on every test that drives a real worker through `installTestWorkManager`. But a real worker only ever produces a *terminal* state with well-formed output, so those are the only arms reached: | arm | line(s) | reached today? | |---|---|---| | `SUCCEEDED` with an output path | 463–503 | yes — real workers produce it | | `FAILED` with a non-blank message | 505–507 | yes — real workers produce it | | `RUNNING`, reading `KEY_PROGRESS` | 446–448 | **no** | | `ENQUEUED`, `runAttemptCount > 0` → `Waiting` | 457–458 | **no** | | `ENQUEUED`, `runAttemptCount == 0` → `Converting` | 460 | **no** | | `SUCCEEDED` with a null output path | 466 | **no** | | `FAILED` with a blank or missing message | 508 | **no** | | `CANCELLED` | 511 | **no** | | `BLOCKED` | 512 | **no** | `grep` for `WorkInfo.State.` across `app/src/test` finds all six constants, which looks like coverage until you see where they are: `ReattachmentTest` drives every one of them into **`Reattachment.choose`**, a different function. The other two files that mention them (`ConverterStateAffordancesTest`, `FailedSaveRetryTest`) name the `FAILED` arm in KDoc and do not construct a `WorkInfo` at all. **So the enqueued-means-retry rule is tested in one of its two homes.** `Reattachment.choose` has a test for `runAttemptCount = 0` versus `1`; `observe`, which is what the user's screen actually reads, does not — and the comment above line 456 explains at length why the distinction matters. A rule with a documented rationale and two implementations should not have one of them untested. ## Why a seam, and which one There is nothing to inject. `workManager` is `private val workManager = WorkManager.getInstance(app)`, hard-wired in the constructor at line 183, and `observe` is private. A test cannot hand this a chosen `WorkInfo`. Take the same answer #141 took for `MediaProbe`: lift the `when (info.state)` out as an `internal` pure function over the values it actually reads, and leave the collect, the null check and the ownership check behind as the thin edge. `internal` rather than `private` for the reason `MediaProbe.Extracted` gives — the JVM test source set is a friend of `main`, so it stays invisible outside the module. Two things the seam must not swallow: - **The ownership check at 444 stays outside it.** Its KDoc is explicit that it guards the `SUCCEEDED` arm's file ownership, not just the assignment. Moving it inside a pure mapper would change what it protects. - **The `SUCCEEDED` arm takes ownership of the staged file** (468 onward), which is a side effect. The mapper should return the decision; the caller performs it. If that split is awkward, say so on the ticket rather than fusing them — a pure function that deletes files is not the seam this asks for. ## Done when Each of the six arms is chosen by a test, and each is killed by a mutation of its own. Note `ENQUEUED` needs two tests, not one — a single test passes against a mapper that ignores `runAttemptCount` entirely. Name any arm you decide is not worth pinning, and why. `BLOCKED` is the likeliest candidate: it maps to the same `Converting` as a fresh `ENQUEUED`, so a test for it asserts an equality rather than a rule.
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
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#154