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

C5 — ConversionWorker's missing-input-URI and invalid-spec refusals have no test on any path #139

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

Child 5 of 7 decomposing #132 — item 5 there. Independent.

Why this exists

Two cold exits in ConversionWorker.doWork, both reachable and neither tested.

1. :61-62 — no input URI at all

val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
    ?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))

Untested everywhere, JVM and device. The nearest e2e test,
ForcedFailureTest.aMissingInputFailsRatherThanCrashing, passes a URI pointing at a file that does
not exist — which reaches the engine and fails much later, on a different path, with a different
message. A missing key is what a job enqueued by an older build looks like, the same premise C1 (#135) and
JobTags are written on.

2. :123-126 — a spec the picker would never have allowed

val validation = ContainerCapabilities.validate(spec, probe)
if (validation is Validation.Invalid) {
    Log.w(TAG, "Refusing $spec for $displayName: ${validation.message}")
    return Result.failure(workDataOf(KEY_ERROR to validation.message))
}

The comment above it names exactly why it exists:

The picker refuses an impossible combination before Convert is tappable, but a job can also arrive
from a queued request made before the settings changed, or from a direct
ConversionWorker.request(...) call. Checking here means an invalid spec fails with the reason
rather than being silently coerced into something else.

Both arrival paths are real, and nothing exercises either. This is the guard that decides whether the
user reads why their conversion was refused or just "Conversion failed."

Scope

Two tests. Both fit the existing worker-test shape — TestListenableWorkerBuilder with a hand-built
Data, which is what WorkerEnumFallbackTest, SpaceCheckTest and PerJobStagingTest all do.

For the second, C2 (#136) lands the ContainerCapabilities refusals themselves; this asserts only that the
worker surfaces the message rather than routing on regardless. The two are independent — take
them in either order.

Done means

Missing key → Result.failure carrying "No input file.", with no staging file created. Invalid
spec → Result.failure carrying validation.message verbatim, and the engine never invoked
(assert on a recording transcoder, not just on the Result).

Mutation: delete the if (validation is Validation.Invalid) block. The test must go red on the
error message — if it instead goes red on a downstream conversion failure, it is asserting the wrong
thing and has not closed this row.

_Child 5 of 7 decomposing #132 — item 5 there. Independent._ ### Why this exists Two cold exits in `ConversionWorker.doWork`, both reachable and neither tested. **1. `:61-62` — no input URI at all** ```kotlin val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse) ?: return Result.failure(workDataOf(KEY_ERROR to "No input file.")) ``` Untested **everywhere**, JVM and device. The nearest e2e test, `ForcedFailureTest.aMissingInputFailsRatherThanCrashing`, passes a URI pointing at a *file that does not exist* — which reaches the engine and fails much later, on a different path, with a different message. A missing *key* is what a job enqueued by an older build looks like, the same premise C1 (#135) and `JobTags` are written on. **2. `:123-126` — a spec the picker would never have allowed** ```kotlin val validation = ContainerCapabilities.validate(spec, probe) if (validation is Validation.Invalid) { Log.w(TAG, "Refusing $spec for $displayName: ${validation.message}") return Result.failure(workDataOf(KEY_ERROR to validation.message)) } ``` The comment above it names exactly why it exists: > The picker refuses an impossible combination before Convert is tappable, but a job can also arrive > from a queued request made before the settings changed, or from a direct > `ConversionWorker.request(...)` call. Checking here means an invalid spec fails with the reason > rather than being silently coerced into something else. Both arrival paths are real, and nothing exercises either. This is the guard that decides whether the user reads *why* their conversion was refused or just "Conversion failed." ### Scope Two tests. Both fit the existing worker-test shape — `TestListenableWorkerBuilder` with a hand-built `Data`, which is what `WorkerEnumFallbackTest`, `SpaceCheckTest` and `PerJobStagingTest` all do. For the second, C2 (#136) lands the `ContainerCapabilities` refusals themselves; this asserts only that the **worker surfaces the message** rather than routing on regardless. The two are independent — take them in either order. ### Done means Missing key → `Result.failure` carrying `"No input file."`, with no staging file created. Invalid spec → `Result.failure` carrying `validation.message` verbatim, and **the engine never invoked** (assert on a recording transcoder, not just on the Result). **Mutation:** delete the `if (validation is Validation.Invalid)` block. The test must go red on the error message — if it instead goes red on a downstream conversion failure, it is asserting the wrong thing and has not closed this row.
JMR-dev commented 2026-09-02 01:40:36 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Already done, and verified on main today — closing.

This stayed open through a bookkeeping failure, not an unfinished one. PR #148 carried Closes #139, but GitHub only fires a closing keyword when the PR merges into the default branch. #148 merged into its stack base instead (the async-retarget race written up on #160 and now in CLAUDE.md), so the keyword never ran. The content reached main later via #160, which did not carry the keywords.

Verified against main at d354f64 just now, by re-running this ticket's own named mutation rather than by checking the files exist:

the `if (validation is Validation.Invalid)` block deleted
  red: a spec the picker would never have allowed is refused with the reason
  red: a refused spec never reaches an engine

Both halves red, which is the distinction this ticket insisted on: the second is what says the job failed before converting rather than during.

Gate green on main: 546 tests in 76 classes, 0 failures.

**Already done, and verified on `main` today — closing.** This stayed open through a bookkeeping failure, not an unfinished one. PR #148 carried `Closes #139`, but **GitHub only fires a closing keyword when the PR merges into the default branch.** #148 merged into its stack base instead (the async-retarget race written up on #160 and now in `CLAUDE.md`), so the keyword never ran. The content reached `main` later via #160, which did not carry the keywords. Verified against `main` at `d354f64` just now, by re-running **this ticket's own named mutation** rather than by checking the files exist: ``` the `if (validation is Validation.Invalid)` block deleted red: a spec the picker would never have allowed is refused with the reason red: a refused spec never reaches an engine ``` Both halves red, which is the distinction this ticket insisted on: the second is what says the job failed *before* converting rather than during. Gate green on `main`: 546 tests in 76 classes, 0 failures.
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#139