Compare commits

..
Author SHA1 Message Date
Jason Ross 795b456d94 Merge pull request #148 from JMR-dev/test/refused-jobs
C5: the two jobs ConversionWorker refuses before converting
2026-08-27 08:56:36 -05:00
JMR-dev 2c0bc4a583 Merge branch 'test/concatworker-failure-arms' into test/refused-jobs 2026-08-27 07:19:41 -05:00
JMR-dev 7a47285f37 Merge branch 'test/container-capabilities-audio' into test/concatworker-failure-arms 2026-08-27 07:19:39 -05:00
JMR-dev 8a2bc86cac Merge branch 'test/readspec-enum-fallbacks' into test/container-capabilities-audio 2026-08-27 07:19:38 -05:00
JMR-dev 9b3b9f952b Merge remote-tracking branch 'origin/test/outputpublisher-seams' into test/readspec-enum-fallbacks 2026-08-27 07:19:37 -05:00
JMR-dev 0e2525195b Merge branch 'test/concatworker-failure-arms' into test/refused-jobs 2026-08-27 07:18:28 -05:00
JMR-dev 713d813a65 Merge branch 'test/container-capabilities-audio' into test/concatworker-failure-arms 2026-08-27 07:18:27 -05:00
JMR-dev c360e82a10 Merge branch 'test/readspec-enum-fallbacks' into test/container-capabilities-audio 2026-08-27 07:18:25 -05:00
JMR-dev 699d608b47 Merge remote-tracking branch 'origin/main' into test/readspec-enum-fallbacks 2026-08-27 07:18:24 -05:00
JMR-devandClaude Opus 5 ad47ce6c96 S2 + S3 (#142, #143): the two OutputPublisher seams, and where the second one goes
#142 -- openOutputStream refuses two ways and only one was reachable. A
provider that has gone away throws from inside the call, which
`a destination the provider will not open...` already drives. A provider
that is present and declines returns null, and nothing could produce that
on demand. openDestination is the seam; the test asserts the failure names
the destination, which is what separates the `?: error(...)` from an NPE
inside `use`.

#143 -- the sweep's re-read. **The seam the ticket proposed does not reach
it.** Overriding the listing fires before the entries are snapshotted, so
StagingSweep.collectable is handed the new timestamp, the file is never
proposed for deletion, and the guard is never exercised. Measured: with an
entriesIn seam, deleting the guard outright left the test green.

The race is a file that *was* collectable when the snapshot was taken and
is not by the time the delete comes round, so the seam has to sit at the
snapshot. `snapshot(listing)` does, and deleting the guard now reddens the
test.

Three mutations after the move, three red:

  null stream returns silently   null-return test
  null stream via !! instead     null-return test
  sweep deletes unconditionally  race test

OutputPublisher.kt now has no never-executed lines at all. Two partial
branches are left and both are named exemptions rather than gaps:
L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are
the failure arms of a runCatching whose body cannot be made to throw
through any public entry point -- the same shape as the `size >= 0`
exemption recorded in the previous commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-27 07:14:19 -05:00
JMR-devandClaude Opus 5 c60d5d54c6 Stop the staging fixture losing a race with the app-start sweep (#159)
`the sweep tolerates a staging path that is not a directory` failed once on run
33069641674, against 468 tests that pass on this machine including under
`--rerun-tasks`:

    java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112
    468 tests completed, 1 failed

Line 112 was `writeBytes` immediately after `deleteRecursively()`.
`FileOutputStream` answers `FileNotFoundException` for an existing directory, so
something had recreated the path inside that window. That something is
`LibreMediaConverterApp.onCreate`, which ends with

    appScope.launch { OutputPublisher(...).sweepStaging() }

on `Dispatchers.IO`, and `sweepStaging` reads `stagingDir`, whose getter calls
`mkdirs()`. Robolectric builds the application for every test that asks for one,
so that background `mkdirs()` is in flight across the whole suite on a thread the
paused main looper does not control and no test awaits.

Retrying closes the window rather than narrowing it, because the race is not
symmetric: `mkdirs()` fails on an existing regular file, so the invariant only has
to survive being *established*. Once a write lands, nothing in the suite can turn
this path back into a directory -- which is also why the new assertion that the
sweep left a file behind is worth making.

The `check()` matters as much as the loop. The next failure here should say
"something recreated conversions/ as a directory", not `FileNotFoundException at
line 112` -- that is the difference between a flake someone reads and a flake
someone re-runs.

The wider problem is #159 and is deliberately not fixed here: `AppStartSweepTest`,
`JobSnapshotsTest` and `SpaceArithmeticTest` all name the same path, and the real
answer is an injectable scope rather than a retry loop in every staging test.
#159's done-when is that this loop can be deleted.

Mutation: `listFiles() ?: return` -> `listFiles()!!` reddens exactly this test.
Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-27 07:14:18 -05:00
JMR-devandClaude Opus 5 d59e9acce5 C6 (#140): OutputPublisher's guarded branches, three of them guarding a delete
destinationIsKnownEmpty's three short-circuits -- no SIZE column, no row,
a null cell -- each had to answer false and none was tested. Its KDoc is
unambiguous about why: "this decides whether a delete is allowed and 'I
could not tell' must never authorise one." The existing tests only ever
drove a provider that answers properly, where the answer is zero and the
delete is correct. Getting the uncertain cases backwards costs the user a
file they already had, on a save that failed.

Also discardStaged's null parentFile, and sweepStaging's null listing --
which is not the case the existing `tolerates a staging directory that
does not exist yet` covers, because stagingDir's own mkdirs() recreates a
missing directory and it then lists as empty. Only a path that cannot be
a directory makes listFiles() answer null.

Five mutations, three bite:

  drop !row.isNull(size)                 short-circuit test red
  drop row.moveToFirst()                 five tests red
  parentFile!! instead of ?: return false parentless test red

  size >= 0  ->  size >= -1              GREEN, does not bite
  parentless treated as staged           GREEN -- bad mutation, see below

The first green one is recorded in the test as a named exemption.
Measured: getColumnIndex returns -1 for an absent column and isNull(-1)
throws CursorIndexOutOfBoundsException, which the surrounding runCatching
already turns into `?: false`. Same answer, reached by the exception path,
so no behavioural test can pin that conjunct. It stays anyway -- control
flow through an exception is worse than a comparison, and another Cursor
implementation need not throw.

The second was my mistake rather than a finding: substituting stagingDir
for the null parent reaches `return false` by a different route, so it
proves nothing. parentFile!! is the honest mutation and it goes red.

:197, :235 and :258 are now covered. What is left in this file is exactly
what the ticket scoped out: :173-174 (#142) and :267 (#143).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-27 07:14:18 -05:00
Jason Ross 324c9a4555 Merge pull request #144 from JMR-dev/test/fake-provider-scaffolding
C0 + C3: fake-provider scaffolding, and what InputQuery makes of a metadata row
2026-08-27 07:14:14 -05:00
JMR-devandClaude Opus 5 44493d9943 C5 (#139): the join side's count refusal, found by the residual-gap audit
A gap audit over the eight branches merged together looked for lines still
never executed and asked, for each, whether something already accounts for it.
Everything mapped except one: `ConcatWorker.kt:42`, the refusal of a join with
fewer than two inputs.

Its neighbour maps. `ConcatWorker.kt:40` -- the missing-URI-array arm, two lines
above -- is covered on the device by
`UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. That is invisible to
JaCoCo, which measures `testDebugUnitTest` only, so the report shows both arms
cold and cannot distinguish the one that is e2e-covered from the one nothing
touches. Only reading the androidTest source separates them.

`grep` says nothing in either source set mentions "Pick at least two files to
join." Two tests here now do:

- `a join of a single file is refused with a message rather than joined` pins
  the verdict and the message together, via `Failure.equals`, for the reason the
  file's header already gives.
- `a join of two files is not refused for its count` is the control that puts
  the assertion on the boundary rather than on the string. It refuses the
  *space* rather than letting the job run: the next thing past the count guard
  is `ConcatEngine`, which is native, and `NamingPublisher`'s KDoc already
  records that no JVM test gets past it. A failure carrying the space message is
  proof execution reached line 57, which is proof it cleared line 42, at no
  engine cost.

Reachability is the header's argument plus one of its own: `request(...)` takes
a `List<Uri>` and checks nothing about its length, so a one-item join is a
well-formed call rather than a corrupted queue entry.

Mutations, each killing exactly the test it should:

| mutation | red |
|---|---|
| guard deleted outright | `a join of a single file is refused...` |
| `uris.size < 2` -> `< 3` | `a join of two files is not refused for its count` |

Restored, both green. Gate green: assembleDebug, testDebugUnitTest,
compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 23:14:46 -05:00
JMR-devandClaude Opus 5 de6d9526ba C5 (#139): the two jobs ConversionWorker refuses before converting
Both exits were cold, and both are reachable for the same reason: a job
does not have to come from the picker. WorkManager keeps work for about a
week, so a downgrade or rollback hands this build a job enqueued by
another one, and request(...) is callable directly.

:62 -- a missing KEY_INPUT_URI -- was 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 with a different message.

:124-126 -- the Validation.Invalid refusal -- had no test at all, though
its comment names both arrival paths it exists for.

Five tests, in a new file because both are about the *message*. A refusal
that fails with empty output Data renders the UI's generic "Conversion
failed." with nothing else to say, which is the defect shape
DeniedForegroundStartTest records from the device pass; asserting the
verdict alone would pass against exactly that.

Two of the five are there to stop the others passing for the wrong reason:
`a refused spec never reaches an engine` says it failed *before*
converting rather than during, and `a valid spec is not refused` is the
control -- without it every assertion here would still pass against a
worker that refused everything.

Three mutations, three red:

  change the no-input message         no-input message test
  drop the validation refusal         both refusal tests
  validate but keep converting        both refusal tests

L61-62 and L123-126 are now fully covered, branches included.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 22:37:23 -05:00
JMR-devandClaude Opus 5 bb3358f209 C4 (#138): ConcatWorker's cancellation and give-up arms
ConversionWorker has WorkerCancellationTest and DeniedForegroundStartTest.
Its twin had the retry case only -- `a join whose foreground start is
denied` already existed -- so two of ConcatWorker's three failure exits
were cold: the CancellationException arm, and FOREGROUND_DENIED.

Four tests, added to the files that own each rule rather than to a new
ConcatWorker file, which is how this suite is organised: a file per rule,
tested across both workers.

The cancellation seam is worth a look in review. The conversion twin
cancels inside the engine, which is honest there because
ConversionDependencies has a seam for it. ConcatWorker calls ConcatEngine
directly and has none -- it is native and nothing here gets past it -- so
the cancellation is injected at the only other point inside the try,
setForeground. That is a real shape rather than a contrivance: a job
cancelled while WorkManager is promoting it is exactly when that window is
open, and the catch arm cannot tell where in the try it came from.

FailedFuture moved to WorkerStubs.kt on the way. Two tests now inject two
different failures through it, and Kotlin will not take two file-private
top-level classes of one name in one package.

Four mutations, four red, each isolated:

  cancellation arm -> Result.failure     propagation test only
  drop delete on cancellation            cancellation-partial test only
  FOREGROUND_DENIED -> Result.retry      past-the-bound test only
  drop delete on the Throwable path      give-up-partial test only

ConcatWorker's :92, :95-96 and :105-106 are covered; missed branches 4 -> 3.
What is left is what the ticket scoped out: the two input guards (e2e), the
ConcatEngine success path (native), and getForegroundInfo (#88's named
exemption).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 22:33:04 -05:00
JMR-devandClaude Opus 5 04850a0415 C2 (#136): test the audio half of validate, and the one video refusal missing
The two halves of ContainerCapabilities.validate were written together
and only one of them was ever checked. Six audio outcomes had no test --
every one a string the user reads -- while the video twin of each was
already covered.

Seven tests, deliberately shaped like their twins rather than as a fresh
idea about what to assert:

  unidentifiable source audio on a COPY   twin of `an unidentifiable
                                          source codec cannot be copied`
  container cannot hold the copied source twin of `a codec the container
                                          cannot hold is refused...`
  container cannot carry it on encode     twin of `H265 in AVI is refused`
  this app cannot encode it               twin of `copying is offered as
                                          the fix when...`
  accepts(_, AudioCodec.NONE, _) -> true  twin of the VideoCodec.NONE arm
  accepts(_, AudioCodec.COPY, _) throws   twin of `resolving COPY before
                                          asking the matrix is required`

The seventh is not the audio axis: validateVideo's copy-into-a-container-
that-cannot-hold-it refusal was the one video outcome with no test, and it
is the same shape and the same file.

Each asserts the message verbatim and re-validates every suggestion the
refusal offers. Validation.Invalid promises its suggestions are themselves
valid and names this class as the proof; the existing property test walks
the presets, and no preset reaches suggestions() through validateAudio.

Seven mutations run, seven red, each isolated to exactly one test:

  CARRIES_AUDIO check -> false     encode-path test only
  drop the COPY error arm          resolve-first test only
  AudioCodec.NONE -> false         no-audio-track test only
  drop ENCODABLE_AUDIO check       unencodable test only
  drop audio copy container check  audio-copy test only
  drop video copy container check  video-copy test only
  drop unidentified-audio guard    unidentifiable test only

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 22:27:27 -05:00
JMR-devandClaude Opus 5 8ab433b647 C1 (#135): pin readSpec's three enum fallbacks
WorkerEnumFallbackTest already existed for this defect class -- a name
this build does not define, read above the try, throwing out of doWork
entirely: FAILED with reschedule=false, empty output Data so the screen
said "Conversion failed." with nothing else, and the staged file never
deleted. It covered 2 of the 5 above-the-try reads. readSpec's three
were the ones left, and all three were cold.

The baseline is the part worth reviewing. readSpec returns the *entire*
fallback spec the moment any one axis fails to resolve, so a test
starting from MP4_H265 -- which is itself the fallback -- cannot tell a
worker that read the spec correctly from one that gave up on it. These
start from MKV/H.264, which differs on container and video codec at
once, and assert the spec that actually reached the transcoder rather
than only that a Result came back.

Mutations run, four for three tests:

  KEY_CONTAINER    `?: return fallback` -> `?: error(...)`  -> container test red
  KEY_VIDEO_CODEC  same                                     -> video test red
  KEY_AUDIO_CODEC  same                                     -> audio test red
  fallback = MP4_H264 instead of MP4_H265                   -> all three red

The first three confirm the tests are isolated to their own axis; the
fourth confirms they pin *which* spec ran, which is what "a Result at
all" would have missed.

readSpec is now fully covered, branches included.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-26 22:24:10 -05:00
9 changed files with 805 additions and 29 deletions
@@ -5,6 +5,7 @@ import android.net.Uri
import android.provider.DocumentsContract import android.provider.DocumentsContract
import android.provider.OpenableColumns import android.provider.OpenableColumns
import java.io.File import java.io.File
import java.io.OutputStream
/** /**
* What a save has to say when the staged file is not there any more. * What a save has to say when the staged file is not there any more.
@@ -170,7 +171,7 @@ open class OutputPublisher(private val context: Context) {
open fun publish(staged: File, destination: Uri) { open fun publish(staged: File, destination: Uri) {
val destinationWasEmpty = destinationIsKnownEmpty(destination) val destinationWasEmpty = destinationIsKnownEmpty(destination)
try { try {
val out = context.contentResolver.openOutputStream(destination) val out = openDestination(destination)
?: error("Could not open destination for writing: $destination") ?: error("Could not open destination for writing: $destination")
out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } }
} catch (failure: Throwable) { } catch (failure: Throwable) {
@@ -179,6 +180,22 @@ open class OutputPublisher(private val context: Context) {
} }
} }
/**
* Opens [destination] for writing, or null when the provider will not.
*
* A seam, and a narrow one: it exists because `openOutputStream` has **two** ways of refusing
* and only one of them is reachable from a test otherwise. A provider that has gone away throws
* `FileNotFoundException` from inside the call; a provider that is present and declines returns
* null. The two are not interchangeable here — the `?: error(...)` above is the only thing that
* turns the second into a failure rather than an NPE further down — and no fake provider can be
* asked to produce a null return on demand.
*
* `protected open` rather than injected, matching `hasSpaceFor` and `createStagingFile`:
* `WorkerStubs.kt`'s publishers already override one method to force one condition.
*/
protected open fun openDestination(destination: Uri): OutputStream? =
context.contentResolver.openOutputStream(destination)
/** /**
* True only when the destination is *positively known* to hold no bytes yet. * True only when the destination is *positively known* to hold no bytes yet.
* *
@@ -256,7 +273,7 @@ open class OutputPublisher(private val context: Context) {
open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) { open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) {
val dir = stagingDir val dir = stagingDir
val listing = dir.listFiles() ?: return val listing = dir.listFiles() ?: return
val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } val entries = snapshot(listing)
StagingSweep.collectable(entries, nowMs).forEach { name -> StagingSweep.collectable(entries, nowMs).forEach { name ->
val file = File(dir, name) val file = File(dir, name)
// Re-read the timestamp rather than trusting the snapshot above. Between the // Re-read the timestamp rather than trusting the snapshot above. Between the
@@ -268,6 +285,22 @@ open class OutputPublisher(private val context: Context) {
} }
} }
/**
* The name and age of everything [sweepStaging] found, read once.
*
* A seam for the *race*, not for the clock — [sweepStaging] already takes `nowMs`, so the clock
* is the caller's. What has no seam otherwise is the window between this snapshot and the
* per-file re-read below it, and that window is the entire reason the re-read exists.
*
* **It has to be here and not around `listFiles()`.** A test that changes a file before the
* listing, or during it, changes what `StagingSweep.collectable` is given — so the file is
* never proposed for deletion and the re-read is never reached. The race being modelled is a
* file that *was* collectable when the snapshot was taken and is not by the time the delete
* comes round, which is exactly one worker resuming in this same process.
*/
protected open fun snapshot(listing: Array<File>): List<StagingSweep.Entry> =
listing.map { StagingSweep.Entry(it.name, it.lastModified()) }
private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile) private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile)
private companion object { private companion object {
@@ -20,6 +20,9 @@ import java.io.OutputStream
/** What a destination volume says when it fills up mid-write. */ /** What a destination volume says when it fills up mid-write. */
private const val NO_SPACE = "No space left on device" private const val NO_SPACE = "No space left on device"
/** How far a failing copy gets before the volume "fills up". Any value below the payload does. */
private const val PARTIAL_BYTES = 512
/** /**
* A sink that behaves like a volume filling up. * A sink that behaves like a volume filling up.
* *
@@ -75,6 +78,8 @@ class OutputPublisherPublishTest {
private val payload = ByteArray(8192) { (it % 251).toByte() } private val payload = ByteArray(8192) { (it % 251).toByte() }
/** How far a failing copy gets before the volume "fills up". Any value below the payload does. */
private val documentUri: Uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4") private val documentUri: Uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4")
private val plainUri: Uri = Uri.parse("content://$PLAIN_AUTHORITY/document/holiday_plain.mp4") private val plainUri: Uri = Uri.parse("content://$PLAIN_AUTHORITY/document/holiday_plain.mp4")
private val deadUri: Uri = Uri.parse("content://org.libremediaconverter.nonexistent/document/gone.mp4") private val deadUri: Uri = Uri.parse("content://org.libremediaconverter.nonexistent/document/gone.mp4")
@@ -203,6 +208,70 @@ class OutputPublisherPublishTest {
assertEquals(emptyList<Uri>(), FakeSafProvider.deleteRequests) assertEquals(emptyList<Uri>(), FakeSafProvider.deleteRequests)
} }
@Test
fun `a destination whose size cannot be determined is never deleted`() {
// The three short-circuits in destinationIsKnownEmpty, and the reason its KDoc gives for
// each of them answering false:
//
// "this decides whether a delete is allowed and 'I could not tell' must never authorise
// one."
//
// The contrast is `a copy that fails partway leaves nothing at the destination` above: a
// provider that *does* say zero gets the delete. These say nothing, so they must not.
// Getting this backwards costs the user a file they already had, on a save that failed.
//
// Named exemption: of the three conjuncts, `size >= 0` cannot be falsified behaviourally.
// Measured -- getColumnIndex returns -1 for an absent column, and isNull(-1) throws
// CursorIndexOutOfBoundsException, which the surrounding runCatching already turns into
// `?: false`. So relaxing it to `size >= -1` leaves this test green: same answer, reached
// by the exception path instead. The guard should stay -- control flow through an exception
// is worse than a comparison, and another Cursor implementation need not throw -- but no
// assertion here pins it, and saying so beats implying the missing-column case covers it.
// `!row.isNull(size)` and `row.moveToFirst()` do both bite.
listOf(
RowShape.NO_SIZE_COLUMN to "a cursor with no SIZE column",
RowShape.NULL_SIZE to "a cursor whose SIZE cell is null",
RowShape.NO_ROWS to "a cursor holding no rows",
).forEach { (shape, description) ->
FakeSafProvider.deleteRequests.clear()
FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0))
FakeSafProvider.rowShape = shape
failMidCopy(documentUri, afterBytes = PARTIAL_BYTES)
assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) }
assertEquals(
"$description must not authorise a delete",
emptyList<Uri>(),
FakeSafProvider.deleteRequests,
)
assertTrue(
"$description must leave the destination where it was",
FakeSafProvider.backingFile(documentUri).exists(),
)
}
}
@Test
fun `a provider that declines by returning null fails with the destination named`() {
// openOutputStream has two ways of refusing, and only one of them is otherwise reachable.
// `a destination the provider will not open...` above drives the throwing one -- a provider
// that has gone away. This is the other: a provider that is present, answers, and hands
// back null. Without the `?: error(...)` that becomes an NPE inside `use`, which reaches
// the user as "Conversion failed." with a null message.
val nullOpening = object : OutputPublisher(context) {
override fun openDestination(destination: Uri): OutputStream? = null
}
val failure = runCatching { nullOpening.publish(staged, documentUri) }.exceptionOrNull()
assertTrue("a null stream must not appear to succeed, got $failure", failure != null)
assertTrue(
"the failure must name the destination rather than being a bare NPE; got ${failure?.message}",
failure?.message?.contains("Could not open destination for writing") == true,
)
}
@Test @Test
fun `a copy that succeeds delivers every byte and deletes nothing`() { fun `a copy that succeeds delivers every byte and deletes nothing`() {
shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) {
@@ -1,6 +1,8 @@
package org.libremediaconverter.convert package org.libremediaconverter.convert
import android.app.Application
import org.junit.Assert.assertFalse import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue import org.junit.Assert.assertTrue
import org.junit.Before import org.junit.Before
import org.junit.Test import org.junit.Test
@@ -26,14 +28,18 @@ import java.util.UUID
@RunWith(RobolectricTestRunner::class) @RunWith(RobolectricTestRunner::class)
class OutputPublisherStagingTest { class OutputPublisherStagingTest {
private lateinit var app: Application
private lateinit var cacheDir: File private lateinit var cacheDir: File
private lateinit var publisher: OutputPublisher private lateinit var publisher: OutputPublisher
@Before @Before
fun setUp() { fun setUp() {
val context = RuntimeEnvironment.getApplication() // Held as a field rather than a local: the race test below builds an anonymous
cacheDir = context.cacheDir // OutputPublisher, and inside that `object` expression a bare `context` resolves to the
publisher = OutputPublisher(context) // superclass's own constructor property, which is not initialised at the super call.
app = RuntimeEnvironment.getApplication()
cacheDir = app.cacheDir
publisher = OutputPublisher(app)
} }
@Test @Test
@@ -99,4 +105,103 @@ class OutputPublisherStagingTest {
publisher.sweepStaging() publisher.sweepStaging()
} }
@Test
fun `the sweep tolerates a staging path that is not a directory`() {
// The other half of `listFiles() ?: return`, and not the same as the case above: a missing
// directory is created by `stagingDir`'s own mkdirs() and lists as empty. Only a path that
// cannot be a directory makes listFiles() answer null, and a sweep that dereferenced that
// would take the app down on a launch rather than on a conversion -- AppStartSweepTest is
// where this runs from.
val stagingPath = stagingPathAsRegularFile()
publisher.sweepStaging()
assertTrue("the sweep must not have replaced the fixture", stagingPath.isFile)
}
/**
* Makes `cacheDir/conversions` a regular file, which is the whole precondition of the test
* above -- and does it in a loop, because a single delete-then-write loses a race that CI
* caught and this machine does not reproduce.
*
* `LibreMediaConverterApp.onCreate` ends with
* `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`, and
* `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric instantiates
* the application for every test that asks for one, so that background `mkdirs()` is in flight
* across the whole suite, on a thread the paused main looper does not control. Between deleting
* this path and writing it there is a window where the path does not exist and that `mkdirs()`
* can win, which is `FileNotFoundException: ... (Is a directory)` out of `writeBytes` -- run
* 33069641674 on #149, once, against 468 tests that pass here.
*
* Retrying closes it rather than narrowing it, because the race is not symmetric: `mkdirs()`
* fails on an existing regular file, so the invariant only has to survive being *established*.
* Once a write lands, nothing in the suite can turn this back into a directory.
*
* The wider problem -- application-scope IO work racing every Robolectric test that shares
* `cacheDir` -- is #159, and is deliberately not fixed here.
*/
private fun stagingPathAsRegularFile(): File {
val stagingPath = File(cacheDir, "conversions")
repeat(FIXTURE_ATTEMPTS) {
if (stagingPath.isFile) return stagingPath
stagingPath.deleteRecursively()
runCatching { stagingPath.writeBytes(ByteArray(FIXTURE_BYTES)) }
}
check(stagingPath.isFile) {
"the fixture needs $stagingPath to be a regular file and it is a directory; " +
"something recreated it $FIXTURE_ATTEMPTS times -- see #159"
}
return stagingPath
}
@Test
fun `a file that stops being collectable between the listing and the delete survives`() {
// The race the second timestamp read exists for, and the only branch of it that had never
// run. The comment in sweepStaging states the cost precisely: a worker resumed by
// WorkManager -- in this same process -- could have started writing this very file, and
// unlinking an inode a running job still holds open ends with the job reporting success for
// a path that no longer exists.
//
// So: a file old enough to collect at listing time, touched to now before the delete is
// reached. StagingSweep.collectable already said yes; isCollectable has to say no.
val orphan = publisher.createStagingFile(
StagingNames.forJob(UUID.randomUUID(), "mp4"),
).apply { writeBytes(ByteArray(4096)) }
assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000))
// Touched *after* the snapshot is taken, which is the only window that reaches the
// re-read. Doing it around listFiles() instead changes what StagingSweep.collectable is
// given, so the file is never proposed for deletion and the guard is never exercised --
// measured, and the reason the seam sits where it does.
val racing = object : OutputPublisher(app) {
override fun snapshot(listing: Array<File>): List<StagingSweep.Entry> =
super.snapshot(listing).also { orphan.setLastModified(System.currentTimeMillis()) }
}
racing.sweepStaging()
assertTrue(
"a file a live job started writing after the listing must not be unlinked",
orphan.exists(),
)
}
@Test
fun `discarding a file with no parent at all is refused`() {
// A relative name has no parent directory, so `staged.parentFile` is null. The handle
// reaches the ViewModel as a path string out of WorkInfo.outputData and is turned straight
// into a File, so this is not a shape the caller can rule out -- and the guard has to
// answer false rather than dereference it.
val parentless = File("holiday.mp4")
assertNull("the fixture is supposed to have no parent", parentless.parentFile)
assertFalse("a file with no parent is not in staging", publisher.discardStaged(parentless))
}
private companion object {
/** Enough to outlast a burst of application-scope sweeps; one attempt is what CI lost. */
const val FIXTURE_ATTEMPTS = 50
const val FIXTURE_BYTES = 8
}
} }
@@ -358,4 +358,120 @@ class ContainerCapabilitiesTest {
assertEquals(emptyList<VideoCodec>(), ContainerCapabilities.encodableVideo(container)) assertEquals(emptyList<VideoCodec>(), ContainerCapabilities.encodableVideo(container))
} }
} }
// --- the audio axis -----------------------------------------------------
//
// Every rule below has a video twin already tested above. The two halves of `validate` were
// written together and only one of them was ever checked, so these are deliberately shaped like
// their twins rather than as a fresh idea about what to assert.
@Test
fun `an unidentifiable source audio codec cannot be copied`() {
// The audio twin of `an unidentifiable source codec cannot be copied`. Never guess: a copy
// of an unidentified codec is how you ship a file that does not play.
val unknownAudio = InputProbe(videoCodec = "h264", audioCodec = null, container = Container.MP4)
val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY)
val invalid = ContainerCapabilities.validate(spec, unknownAudio) as? Validation.Invalid
?: throw AssertionError("copying an unidentified audio codec must be refused")
assertTrue(invalid.message, invalid.message.contains("could not be identified"))
assertEverySuggestionValid(invalid, unknownAudio)
}
@Test
fun `copying an audio codec the container cannot hold is refused`() {
// MP4 carries AAC, MP3, Opus and FLAC. Vorbis lives in Ogg and Matroska, so a stream copy
// out of a Vorbis source into MP4 has nowhere to put the track.
val vorbisAudio = InputProbe(videoCodec = "h264", audioCodec = "vorbis", container = Container.MKV)
val spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.COPY)
val invalid = ContainerCapabilities.validate(spec, vorbisAudio) as? Validation.Invalid
?: throw AssertionError("Vorbis copied into MP4 must be refused")
assertEquals("MP4 cannot hold Vorbis audio.", invalid.message)
assertEverySuggestionValid(invalid, vorbisAudio)
}
@Test
fun `an audio codec the container cannot hold is refused on the encode path too`() {
// WAV carries PCM and nothing else. The twin is `H265 in AVI is refused`.
val spec = OutputSpec(Container.WAV, VideoCodec.NONE, AudioCodec.AAC)
val invalid = ContainerCapabilities.validate(spec, mp3Source) as? Validation.Invalid
?: throw AssertionError("AAC in WAV must be refused")
assertEquals("WAV cannot hold AAC audio.", invalid.message)
assertEverySuggestionValid(invalid, mp3Source)
}
@Test
fun `an audio codec this app cannot encode is refused, and copying is offered instead`() {
// Matroska carries Vorbis; nothing here encodes it. The refusal has to say so *and* say
// what would work, which is the audio twin of `copying is offered as the fix when the codec
// is right but unencodable`.
val spec = OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.VORBIS)
val invalid = ContainerCapabilities.validate(spec, h264Source) as? Validation.Invalid
?: throw AssertionError("encoding Vorbis must be refused")
assertEquals(
"This app cannot encode Vorbis audio. It can still be copied from a Vorbis source.",
invalid.message,
)
assertEverySuggestionValid(invalid, h264Source)
}
@Test
fun `copying a video codec the container cannot hold is refused`() {
// Not the audio axis, but the one video refusal with no test: AVI predates H.265, so a
// stream copy out of an HEVC source into AVI has nowhere to put the track. `H265 in AVI is
// refused` covers the matrix; this covers what validate() does with it.
val h265Source = InputProbe(videoCodec = "hevc", audioCodec = "mp3", container = Container.MP4)
val spec = OutputSpec(Container.AVI, VideoCodec.COPY, AudioCodec.MP3)
val invalid = ContainerCapabilities.validate(spec, h265Source) as? Validation.Invalid
?: throw AssertionError("H.265 copied into AVI must be refused")
assertEquals("AVI cannot hold H.265 video.", invalid.message)
assertEverySuggestionValid(invalid, h265Source)
}
@Test
fun `no audio track is accepted by every container in both modes`() {
// The audio twin of VideoCodec.NONE -> true. A container that refused "no audio" would make
// every video-only output invalid.
Container.entries.forEach { container ->
listOf(CodecMode.COPY, CodecMode.ENCODE).forEach { mode ->
assertTrue(
"$container should accept no audio track ($mode)",
ContainerCapabilities.accepts(container, AudioCodec.NONE, mode),
)
}
}
}
@Test
fun `resolving audio COPY before asking the matrix is required`() {
// The audio twin of `resolving COPY before asking the matrix is required`, and the reason is
// identical: silently answering "false" would refuse a perfectly good remux.
runCatching { ContainerCapabilities.accepts(Container.MP4, AudioCodec.COPY, CodecMode.COPY) }
.onSuccess { throw AssertionError("expected audio COPY to be rejected by the matrix") }
}
/**
* Every alternative a refusal offers has to be one the same input could actually take.
*
* `Validation.Invalid` promises exactly this and names this class as the proof. The global
* property test walks the presets; these paths reach `suggestions()` through `validateAudio`,
* which no preset does.
*/
private fun assertEverySuggestionValid(invalid: Validation.Invalid, probe: InputProbe) {
invalid.suggestions.forEach {
assertTrue(
"suggestion $it is itself invalid, so the chip leads to a second error",
ContainerCapabilities.validate(it, probe).isValid,
)
}
}
} }
@@ -27,9 +27,6 @@ import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment import org.robolectric.RuntimeEnvironment
import java.io.File import java.io.File
import java.util.UUID import java.util.UUID
import java.util.concurrent.ExecutionException
import java.util.concurrent.Executor
import java.util.concurrent.TimeUnit
/** /**
* That a refused foreground-service start does not end the job. * That a refused foreground-service start does not end the job.
@@ -125,6 +122,40 @@ class DeniedForegroundStartTest {
) )
} }
@Test
fun `a join denied past the attempt bound fails with a message the user can act on`() {
// The join twin of the conversion case above. ConcatWorker reaches the same FailureOutcome
// through its own `when`, and that arm was the only one of its three with no test -- so a
// join that gave up silently, or gave up with an empty Data, would have looked identical to
// one that retried.
val worker = concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS)
val result = runBlocking { worker.doWork() }
assertEquals(
ListenableWorker.Result.failure(
workDataOf(ConcatWorker.KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE),
),
result,
)
}
@Test
fun `a join that gives up collects the partial it had already staged`() {
// The delete lives on ConcatWorker's `catch (e: Throwable)` path, which every give-up goes
// through. Written first so a missing delete cannot pass by asking whether a file nobody
// wrote is absent.
concatStagedFile().writeBytes(ByteArray(PARTIAL_BYTES))
runBlocking { concatWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS).doWork() }
assertEquals(
"a join that gave up must not orphan what it staged",
emptyList<String>(),
stagedNames(),
)
}
private fun conversionWorker(runAttemptCount: Int = 0): ConversionWorker = private fun conversionWorker(runAttemptCount: Int = 0): ConversionWorker =
TestListenableWorkerBuilder<ConversionWorker>( TestListenableWorkerBuilder<ConversionWorker>(
context = app, context = app,
@@ -141,18 +172,22 @@ class DeniedForegroundStartTest {
.setForegroundUpdater(DenyingForegroundUpdater) .setForegroundUpdater(DenyingForegroundUpdater)
.build() .build()
private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>( private fun concatWorker(runAttemptCount: Int = 0): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
context = app, context = app,
inputData = workDataOf( inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "content://test/second.mp4"), ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "content://test/second.mp4"),
ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES,
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name,
), ),
runAttemptCount = 0, runAttemptCount = runAttemptCount,
).setId(CONCAT_ID) ).setId(CONCAT_ID)
.setForegroundUpdater(DenyingForegroundUpdater) .setForegroundUpdater(DenyingForegroundUpdater)
.build() .build()
/** The staging path the join will compute, asked for rather than spelled out here. */
private fun concatStagedFile(): File =
publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension))
/** The staging path the worker will compute, asked for rather than spelled out here. */ /** The staging path the worker will compute, asked for rather than spelled out here. */
private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension)) private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension))
@@ -164,6 +199,7 @@ class DeniedForegroundStartTest {
const val INPUT_BYTES = 1024L const val INPUT_BYTES = 1024L
const val PARTIAL_BYTES = 2048 const val PARTIAL_BYTES = 2048
val SPEC = OutputFormat.MP4_H265.spec val SPEC = OutputFormat.MP4_H265.spec
val CONCAT_FORMAT = OutputFormat.MP4_H264
val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000001") val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000001")
val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000002") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000002")
} }
@@ -182,18 +218,3 @@ private object DenyingForegroundUpdater : ForegroundUpdater {
), ),
) )
} }
/**
* An already-failed future, written out rather than pulled from a futures library.
*
* `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts
* the platform's own exception in front of the worker's catch rather than a wrapper.
*/
private class FailedFuture(private val failure: Throwable) : ListenableFuture<Void> {
override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener)
override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false
override fun isCancelled(): Boolean = false
override fun isDone(): Boolean = true
override fun get(): Void = throw ExecutionException(failure)
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
}
@@ -0,0 +1,276 @@
package org.libremediaconverter.work
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.ListenableWorker
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.ContainerCapabilities
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.Validation
import org.libremediaconverter.model.VideoCodec
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.UUID
/**
* Jobs the worker refuses before it converts anything, and what it says about them.
*
* Two exits, both cold before this file, and both reachable for the same underlying reason: **a job
* does not have to come from the picker.** WorkManager keeps queued and finished work for about a
* week, so a downgrade or a rollback hands this build a job enqueued by another one — the premise
* `WorkerEnumFallbackTest` and `JobTags` are both written on — and `ConversionWorker.request(...)`
* is callable directly.
*
* What makes these worth their own file rather than another case in an existing one is that both
* are about the *message*. A refusal that fails with empty output `Data` renders the UI's generic
* "Conversion failed." with nothing else to say, which is the defect shape `DeniedForegroundStartTest`
* records from the device pass. Asserting the verdict alone would pass against exactly that.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class RefusedJobTest {
private lateinit var app: Application
private lateinit var publisher: OutputPublisher
private lateinit var engine: RefusingTranscoder
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = AlwaysRoomPublisher(app)
engine = RefusingTranscoder()
ConversionDependencies.publisher = { publisher }
ConversionDependencies.software = { engine }
// Neither test is about probing or about this machine's codecs; both would otherwise decide
// the outcome for reasons no assertion mentions. See WorkerCancellationTest's setUp.
ConversionDependencies.probe = { _, _ -> InputProbe() }
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
installTestWorkManager(app, Data.EMPTY)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `a job with no input URI fails with a message rather than a bare failure`() {
val result = runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() }
// `Failure.equals` compares output data, so this pins the message and the verdict together.
assertEquals(
ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to "No input file.")),
result,
)
}
@Test
fun `a job with no input URI stages nothing`() {
// The URI read is the first thing doWork does -- above the space check, above the staging
// name, above the try. A refusal there must not have reserved anything.
runBlocking { workerWithout(ConversionWorker.KEY_INPUT_URI).doWork() }
assertEquals("a job refused for having no input must not stage a file", emptyList<String>(), stagedNames())
}
@Test
fun `a spec the picker would never have allowed is refused with the reason`() {
// WAV carries PCM and nothing else. The picker cannot produce this combination today, which
// is exactly why the worker checks: the job can arrive from a queue written before the
// settings changed, or from a direct request(...) call.
val expected = ContainerCapabilities.validate(REFUSED_SPEC, InputProbe()) as? Validation.Invalid
?: throw AssertionError("the fixture spec is supposed to be invalid; ContainerCapabilities disagrees")
val result = runBlocking { worker(REFUSED_SPEC).doWork() }
assertEquals(
ListenableWorker.Result.failure(workDataOf(ConversionWorker.KEY_ERROR to expected.message)),
result,
)
}
@Test
fun `a refused spec never reaches an engine`() {
// The half that says it failed *before* converting rather than during. Without this, a
// worker that ran the job and then reported the validation message would pass the test
// above -- and would have spent the user's battery on a file it was going to refuse.
runBlocking { worker(REFUSED_SPEC).doWork() }
assertTrue("a refused spec must be refused before any engine runs", engine.invocations.isEmpty())
}
@Test
fun `a valid spec is not refused`() {
// The control. Every assertion above is about a refusal, so without this they would all
// still pass against a worker that refused everything.
val result = runBlocking { worker(OutputFormat.MP4_H265.spec).doWork() }
assertEquals(ListenableWorker.Result.success(), stripOutput(result))
assertEquals(listOf(OutputFormat.MP4_H265.spec), engine.invocations)
}
// --- the same refusal, on the join side ----------------------------------
@Test
fun `a join of a single file is refused with a message rather than joined`() {
// The arm beside it -- a job with no URI array at all -- is covered on the device by
// `UnopenableUriTest.aJoinWithNoInputArrayFailsWithAMessage`. This one was covered by
// nothing in either source set, which a coverage report cannot say because it cannot see
// androidTest: the two arms are adjacent lines and only one of them had a test.
//
// Reachable for the reason this file's header gives, plus one of its own: `request(...)`
// takes a `List<Uri>` and checks nothing about its length, so a single-item join is a
// well-formed call, not a corrupted queue entry.
val result = runBlocking { joinWorker(INPUT).doWork() }
assertEquals(
ListenableWorker.Result.failure(
workDataOf(ConcatWorker.KEY_ERROR to "Pick at least two files to join."),
),
result,
)
}
@Test
fun `a join of two files is not refused for its count`() {
// The control, and the half that makes the test above bite on the boundary rather than on
// the message: without it, `uris.size < 3` passes everything here.
//
// It refuses the space instead of letting the job run, because the next thing past the
// count guard is `ConcatEngine`, which is native -- `NamingPublisher`'s KDoc records that
// no JVM test gets past it. A refusal with the *space* message is proof that execution
// reached line 57, which is proof it got past line 42, and it costs no engine to say so.
val noRoom = NamingPublisher(app).apply { refuseSpace = true }
ConversionDependencies.publisher = { noRoom }
val result = runBlocking { joinWorker(INPUT, SECOND_INPUT).doWork() }
assertEquals(
ListenableWorker.Result.failure(
workDataOf(ConcatWorker.KEY_ERROR to "Not enough free space to join these files."),
),
result,
)
}
/** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */
private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result =
if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result
private fun worker(spec: OutputSpec): ConversionWorker = build(
workDataOf(
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME,
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
ConversionWorker.KEY_CONTAINER to spec.container.name,
ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
)
/**
* The ordinary input `Data`, less one key.
*
* Built by removal rather than by spelling out a shorter map, so the test cannot drift into
* omitting something else as well and passing for a reason it does not name.
*/
private fun workerWithout(key: String): ConversionWorker {
val full = OutputFormat.MP4_H265.spec
val entries = mapOf(
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME,
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
ConversionWorker.KEY_CONTAINER to full.container.name,
ConversionWorker.KEY_VIDEO_CODEC to full.videoCodec.name,
ConversionWorker.KEY_AUDIO_CODEC to full.audioCodec.name,
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
) - key
return build(Data.Builder().putAll(entries).build())
}
private fun build(data: Data): ConversionWorker =
TestListenableWorkerBuilder<ConversionWorker>(context = app, inputData = data, runAttemptCount = 0)
.setId(JOB_ID)
.build()
/**
* A join job carrying [inputs], a declared total, and a format.
*
* The total is declared so `hasRoomFor` takes its `hasSpaceFor` branch: the other branch is
* `hasSpaceForUnknownSize`, which `NamingPublisher` does not override and which would measure
* this machine's real disk.
*/
private fun joinWorker(vararg inputs: Uri): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
context = app,
inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(),
ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES * inputs.size,
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name,
),
runAttemptCount = 0,
).setId(JOB_ID).build()
private fun stagedNames(): List<String> =
publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted()
private companion object {
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
const val DISPLAY_NAME = "holiday.mp4"
const val INPUT_BYTES = 1024L
/** A join needs two, and "two" is the boundary the count guard is about. */
val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday-2.mp4")
/** WAV carries PCM and nothing else, so AAC in WAV has nowhere to go. */
val REFUSED_SPEC = OutputSpec(
org.libremediaconverter.model.Container.WAV,
VideoCodec.NONE,
AudioCodec.AAC,
)
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000005")
}
}
/** An engine that records what it was asked for and writes an output, so a success is a success. */
private class RefusingTranscoder : SoftwareTranscoder {
/** Every spec that actually reached an engine. Empty is the assertion for a refused job. */
val invocations = mutableListOf<OutputSpec>()
override suspend fun run(
request: ConversionRequest,
inputPath: String,
output: File,
durationMs: Long,
onProgress: (Int) -> Unit,
) {
invocations += request.spec
output.writeBytes(ByteArray(OUTPUT_BYTES))
}
private companion object {
const val OUTPUT_BYTES = 512
}
}
@@ -1,12 +1,16 @@
package org.libremediaconverter.work package org.libremediaconverter.work
import android.app.Application import android.app.Application
import android.content.Context
import android.net.Uri import android.net.Uri
import androidx.media3.common.util.UnstableApi import androidx.media3.common.util.UnstableApi
import androidx.work.Data import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.ForegroundUpdater
import androidx.work.ListenableWorker import androidx.work.ListenableWorker
import androidx.work.testing.TestListenableWorkerBuilder import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf import androidx.work.workDataOf
import com.google.common.util.concurrent.ListenableFuture
import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.runBlocking
import org.junit.After import org.junit.After
@@ -18,6 +22,7 @@ import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.convert.StagingNames
import org.libremediaconverter.convert.installTestWorkManager import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.DeviceCodecs import org.libremediaconverter.model.DeviceCodecs
@@ -108,6 +113,54 @@ class WorkerCancellationTest {
assertEquals("a failed attempt must not leave its partial behind", emptyList<String>(), stagedNames()) assertEquals("a failed attempt must not leave its partial behind", emptyList<String>(), stagedNames())
} }
@Test
fun `a cancelled join propagates instead of being turned into a Result`() {
val thrown = runCatching { runBlocking { concatWorker().doWork() } }.exceptionOrNull()
assertTrue(
"cancellation must leave doWork as cancellation, not as a Result; got $thrown",
thrown is CancellationException,
)
}
@Test
fun `a cancelled join still deletes the partial it had already staged`() {
// Written first, so a missing delete cannot pass by asking whether a file nobody wrote is
// absent -- the same reason PartialThenFailingTranscoder writes before it throws.
concatStagedFile().writeBytes(ByteArray(PARTIAL_STAGED_BYTES))
runCatching { runBlocking { concatWorker().doWork() } }
assertEquals("a cancelled join must not leave its partial behind", emptyList<String>(), stagedNames())
}
/**
* A join whose foreground start is cancelled rather than denied.
*
* The conversion twin cancels *inside the engine*, which is the honest shape there because
* `ConversionDependencies` has a seam for it. `ConcatWorker` calls `ConcatEngine` directly and
* has no such seam -- it is native, and nothing here gets past it -- so the cancellation is
* injected at the only other point inside the `try`: `setForeground`. That is not a contrivance.
* A job cancelled while WorkManager is promoting it to the foreground is precisely when the
* window is open, and what is being tested is the `catch` arm, which cannot tell where in the
* `try` the cancellation came from.
*/
private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder<ConcatWorker>(
context = app,
inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"),
ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES,
ConcatWorker.KEY_FORMAT to CONCAT_FORMAT.name,
),
runAttemptCount = 0,
).setId(CONCAT_ID)
.setForegroundUpdater(CancellingForegroundUpdater)
.build()
/** The staging path the join will compute, asked for rather than spelled out here. */
private fun concatStagedFile(): File =
publisher.createStagingFile(StagingNames.forJob(CONCAT_ID, CONCAT_FORMAT.extension))
/** /**
* A worker routed to the software engine, which is [failure] and nothing else. * A worker routed to the software engine, which is [failure] and nothing else.
* *
@@ -142,7 +195,10 @@ class WorkerCancellationTest {
const val DISPLAY_NAME = "holiday.mp4" const val DISPLAY_NAME = "holiday.mp4"
const val INPUT_BYTES = 1024L const val INPUT_BYTES = 1024L
val SPEC = OutputFormat.MP4_H265.spec val SPEC = OutputFormat.MP4_H265.spec
val CONCAT_FORMAT = OutputFormat.MP4_H264
const val PARTIAL_STAGED_BYTES = 2048
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003") val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003")
val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000004")
} }
} }
@@ -167,3 +223,19 @@ private class PartialThenFailingTranscoder(private val failure: () -> Nothing) :
const val PARTIAL_BYTES = 2048 const val PARTIAL_BYTES = 2048
} }
} }
/**
* Stands in for a job cancelled while WorkManager is promoting it to the foreground.
*
* The mechanism `DeniedForegroundStartTest` documents, carrying a different exception:
* `WorkForegroundUpdater` propagates whatever the future failed with, and
* `ListenableFuture.await()` unwraps the `ExecutionException`, so the worker meets a bare
* `CancellationException` exactly where a real cancellation would put one.
*/
private object CancellingForegroundUpdater : ForegroundUpdater {
override fun setForegroundAsync(
context: Context,
id: UUID,
foregroundInfo: ForegroundInfo,
): ListenableFuture<Void> = FailedFuture(CancellationException("cancelled while going foreground"))
}
@@ -22,6 +22,7 @@ import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.QualityTier import org.libremediaconverter.model.QualityTier
import org.robolectric.RobolectricTestRunner import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment import org.robolectric.RuntimeEnvironment
@@ -109,6 +110,50 @@ class WorkerEnumFallbackTest {
) )
} }
@Test
fun `a container this build does not define falls back to the default spec`() {
assertFallsBackToDefault(container = "HOLOTAPE")
}
@Test
fun `a video codec this build does not define falls back to the default spec`() {
assertFallsBackToDefault(video = "H267")
}
@Test
fun `an audio codec this build does not define falls back to the default spec`() {
assertFallsBackToDefault(audio = "SUPER_AAC")
}
/**
* Drives a job whose spec is [NOT_THE_FALLBACK] on every axis but the one named, and asserts the
* whole spec came back as [DEFAULT_SPEC].
*
* **The baseline is the point.** `readSpec` returns the *entire* fallback spec the moment any
* one axis fails to resolve, so a test starting from `MP4_H265` -- which is itself the fallback
* -- could not tell a worker that read the spec correctly from one that gave up on it. Starting
* from MKV/H.264 makes the difference visible on two axes at once.
*
* Asserting the spec that *ran*, rather than only that a `Result` came back, is the other half:
* the defect these three are written for threw out of `doWork` entirely, so "a Result at all"
* would pass against a fallback to something arbitrary.
*/
private fun assertFallsBackToDefault(
container: String = NOT_THE_FALLBACK.container.name,
video: String = NOT_THE_FALLBACK.videoCodec.name,
audio: String = NOT_THE_FALLBACK.audioCodec.name,
) {
val transcoder = RequestRecordingTranscoder()
ConversionDependencies.software = { transcoder }
val result = runBlocking {
conversionWorker(container = container, video = video, audio = audio).doWork()
}
assertEquals(ListenableWorker.Result.success(), stripOutput(result))
assertEquals(listOf(DEFAULT_SPEC), transcoder.specs)
}
/** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */
private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result =
if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result
@@ -116,15 +161,18 @@ class WorkerEnumFallbackTest {
private fun conversionWorker( private fun conversionWorker(
quality: String = QualityTier.FAST.name, quality: String = QualityTier.FAST.name,
preference: String = EnginePreference.FORCE_SOFTWARE.name, preference: String = EnginePreference.FORCE_SOFTWARE.name,
container: String = SPEC.container.name,
video: String = SPEC.videoCodec.name,
audio: String = SPEC.audioCodec.name,
): ConversionWorker = TestListenableWorkerBuilder<ConversionWorker>( ): ConversionWorker = TestListenableWorkerBuilder<ConversionWorker>(
context = app, context = app,
inputData = workDataOf( inputData = workDataOf(
ConversionWorker.KEY_INPUT_URI to INPUT.toString(), ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME,
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
ConversionWorker.KEY_CONTAINER to SPEC.container.name, ConversionWorker.KEY_CONTAINER to container,
ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, ConversionWorker.KEY_VIDEO_CODEC to video,
ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, ConversionWorker.KEY_AUDIO_CODEC to audio,
ConversionWorker.KEY_QUALITY to quality, ConversionWorker.KEY_QUALITY to quality,
ConversionWorker.KEY_ENGINE_PREFERENCE to preference, ConversionWorker.KEY_ENGINE_PREFERENCE to preference,
), ),
@@ -146,6 +194,12 @@ class WorkerEnumFallbackTest {
const val DISPLAY_NAME = "holiday.mp4" const val DISPLAY_NAME = "holiday.mp4"
const val INPUT_BYTES = 1024L const val INPUT_BYTES = 1024L
val SPEC = OutputFormat.MP4_H265.spec val SPEC = OutputFormat.MP4_H265.spec
/** What `readSpec` returns when any axis fails to resolve. */
val DEFAULT_SPEC = OutputFormat.MP4_H265.spec
/** A spec that differs from [DEFAULT_SPEC] on container *and* video codec. See the helper. */
val NOT_THE_FALLBACK = OutputFormat.MKV_H264.spec
val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022") val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022")
} }
@@ -156,6 +210,9 @@ private class RequestRecordingTranscoder : SoftwareTranscoder {
val qualities = mutableListOf<QualityTier>() val qualities = mutableListOf<QualityTier>()
/** The spec each run was asked for. Which one ran is what the three readSpec tests assert. */
val specs = mutableListOf<OutputSpec>()
override suspend fun run( override suspend fun run(
request: ConversionRequest, request: ConversionRequest,
inputPath: String, inputPath: String,
@@ -164,6 +221,7 @@ private class RequestRecordingTranscoder : SoftwareTranscoder {
onProgress: (Int) -> Unit, onProgress: (Int) -> Unit,
) { ) {
qualities += request.quality qualities += request.quality
specs += request.spec
output.writeBytes(ByteArray(OUTPUT_BYTES)) output.writeBytes(ByteArray(OUTPUT_BYTES))
} }
@@ -1,10 +1,14 @@
package org.libremediaconverter.work package org.libremediaconverter.work
import android.content.Context import android.content.Context
import com.google.common.util.concurrent.ListenableFuture
import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.ConversionRequest
import java.io.File import java.io.File
import java.util.concurrent.ExecutionException
import java.util.concurrent.Executor
import java.util.concurrent.TimeUnit
/** /**
* Scaffolding more than one worker test needs. * Scaffolding more than one worker test needs.
@@ -68,3 +72,25 @@ object WritingTranscoder : SoftwareTranscoder {
private const val OUTPUT_BYTES = 512 private const val OUTPUT_BYTES = 512
} }
/**
* An already-failed future, written out rather than pulled from a futures library.
*
* `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts
* the original exception in front of the worker's `catch` rather than a wrapper. That is the whole
* mechanism behind driving a `ForegroundUpdater` to fail: `WorkForegroundUpdater` propagates
* whatever the future failed with rather than swallowing it, so `setForeground()` throws exactly
* what is handed here.
*
* Shared because two tests inject two different failures through it -- a denied foreground start
* and a cancellation -- and Kotlin will not take two file-private top-level classes of one name in
* one package.
*/
internal class FailedFuture(private val failure: Throwable) : ListenableFuture<Void> {
override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener)
override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false
override fun isCancelled(): Boolean = false
override fun isDone(): Boolean = true
override fun get(): Void = throw ExecutionException(failure)
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
}