Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
350b179c9e | ||
|
|
0cc4c4f3a3 | ||
|
|
c5b2dc0f55 | ||
|
|
8db9a6f52f | ||
|
|
31f249ae04 | ||
|
|
d6e1e3bf86 | ||
|
|
6992f0e783 | ||
|
|
bb920b5bd0 | ||
|
|
4d090d9a81 | ||
|
|
39327beea7 |
@@ -76,11 +76,11 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
|
||||
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
|
||||
table.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 61 instrumented
|
||||
tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail
|
||||
inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down
|
||||
when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate
|
||||
`continue-on-error` job; the gating leg runs the other 57.
|
||||
`continue-on-error` job; the gating leg runs the other 58.
|
||||
|
||||
That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer
|
||||
describes everything in it. The name is kept deliberately — it is not a required context and
|
||||
@@ -332,9 +332,24 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it
|
||||
passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ.
|
||||
|
||||
The read was a triage, not a test push, and five of its six findings are prose rather than code —
|
||||
The read was a triage, not a test push, and six of its seven findings are prose rather than code —
|
||||
the suite itself is in good shape. What had drifted is its self-description.
|
||||
|
||||
**Working the tickets then found the thing the read could not: one production defect.** #238 —
|
||||
joining files picked through the system picker failed outright on the stream-copy path. The
|
||||
concat demuxer whitelists protocols separately from `-safe 0`, and `ffkitsaf` was not on the
|
||||
list; only `STREAM_COPY` feeds it a list file, and every existing join test passed
|
||||
`Uri.fromFile`, so **the one broken combination was the only one a user could reach**. Not a
|
||||
missed line and not an unasserted value — two covered things no test put together, which is the
|
||||
gap shape a coverage number is worst at.
|
||||
|
||||
**E7 is the other reusable result**, because it re-scoped its own ticket. A real
|
||||
`DocumentsProvider` cannot be reached without the picker: an unprotected one is refused at
|
||||
install, instrumentation runs in the app's uid so the test APK's identity is no help, and shell
|
||||
identity is denied too — each denial naming `ACTION_OPEN_DOCUMENT`. So #226 has no cheap headless
|
||||
half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
|
||||
and is how #238 surfaced.
|
||||
|
||||
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
|
||||
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
|
||||
a change that is both needs both.
|
||||
|
||||
@@ -17,7 +17,7 @@ package org.libremediaconverter
|
||||
*
|
||||
* Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an
|
||||
* ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and
|
||||
* the gating one grows by two.
|
||||
* the gating one grows by [FAILS_ON_EMULATOR_API37_BASELINE].
|
||||
*
|
||||
* **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the
|
||||
* advisory job checks the run against it. Adding or removing a marker means changing that number
|
||||
@@ -52,4 +52,4 @@ annotation class FailsOnEmulatorApi37
|
||||
* `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report
|
||||
* records the truncation next to the counts for that reason.
|
||||
*/
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = 3
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = 4
|
||||
|
||||
@@ -7,6 +7,10 @@ import androidx.media3.common.MimeTypes
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.test.ext.junit.runners.AndroidJUnit4
|
||||
import androidx.test.platform.app.InstrumentationRegistry
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.cancelAndJoin
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import org.junit.After
|
||||
@@ -14,6 +18,7 @@ import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Assert.fail
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
@@ -262,6 +267,92 @@ class Media3EngineTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Cancelling a *running* export stops it, completing #224's third engine.
|
||||
*
|
||||
* The two FFmpeg engines were done first (`ad2a75d`, `d293646`); this is
|
||||
* `Media3Engine.transcode`'s `invokeOnCancellation`, which posts `transformer.cancel()` onto the
|
||||
* engine's own `HandlerThread` because `cancel()` has the same single-thread requirement as
|
||||
* `start()`.
|
||||
*
|
||||
* ## Why the assertion is the output file here, and was not for FFmpeg
|
||||
*
|
||||
* The FFmpeg side could not use the file: `invokeOnCancellation` unlinks it, and on POSIX ffmpeg
|
||||
* keeps writing to the unlinked inode, so the path stays gone whether or not the cancel landed.
|
||||
* It asserted the session's return code instead.
|
||||
*
|
||||
* `Media3Engine` deletes nothing — the partial is `ConversionWorker`'s to clean up — so the file
|
||||
* *is* the evidence. An export that was cancelled leaves no moov atom, so `MediaExtractor`
|
||||
* either finds no video track or refuses the file outright with
|
||||
* `IOException: Failed to instantiate extractor` — measured, and both mean interrupted. One
|
||||
* that ran to completion leaves a playable HEVC file, which is the only outcome treated as a
|
||||
* miss. The wait before
|
||||
* reading it is deliberately several times the length of the export, so a *non*-cancelled export
|
||||
* has certainly finished by then: the failure direction is "the file became valid", never "we
|
||||
* did not wait long enough".
|
||||
*
|
||||
* ## Why it retries
|
||||
*
|
||||
* Same reason as the other two, measured there: the committed fixture is 3 s at 320x240 and the
|
||||
* export outruns a naive cancel on a loaded runner. An attempt whose export finished before the
|
||||
* cancel landed has tested nothing, so it is a miss and is retried; only exhausting
|
||||
* [CANCEL_ATTEMPTS] fails. With `transformer.cancel()` removed every attempt produces a playable
|
||||
* file, so the mutation still bites — it just takes five tries to say so.
|
||||
*
|
||||
* Progress having been reported is what proves the export really started, so a miss is
|
||||
* distinguishable from an export that never ran at all — which matters on the API 37 image,
|
||||
* where the decoder is what fails.
|
||||
*/
|
||||
@Test
|
||||
@FailsOnEmulatorApi37
|
||||
fun cancellingARunningExportStopsIt(): Unit = runBlocking {
|
||||
val outcomes = mutableListOf<String>()
|
||||
|
||||
repeat(CANCEL_ATTEMPTS) { attempt ->
|
||||
val partial = File(context.cacheDir, "cancelled_export_$attempt.mp4").apply { delete() }
|
||||
|
||||
val job = launch(Dispatchers.IO) {
|
||||
engine.transcode(
|
||||
input = Uri.fromFile(input),
|
||||
output = partial,
|
||||
request = ConversionRequest(OutputFormat.MP4_H265.spec),
|
||||
)
|
||||
}
|
||||
|
||||
// The muxer creating the file is proof the export really started, and it is the
|
||||
// earliest such proof available -- earlier than the first progress tick.
|
||||
withTimeout(TIMEOUT_MS) {
|
||||
while (!partial.exists() && job.isActive) delay(POLL_MS)
|
||||
}
|
||||
val started = partial.exists()
|
||||
job.cancelAndJoin()
|
||||
|
||||
if (!started) {
|
||||
// The export failed before writing anything. That is not a cancellation result
|
||||
// either way, so it is not allowed to pass as one.
|
||||
outcomes += "attempt $attempt never produced an output file to cancel"
|
||||
return@repeat
|
||||
}
|
||||
|
||||
// Several times the export's own length, so a cancel that did not land has certainly
|
||||
// finished. The failure direction is "the file became playable", never "too soon".
|
||||
delay(SETTLE_MS)
|
||||
|
||||
// A cancelled export reports itself two ways and both mean the same thing: no video
|
||||
// track, or MediaExtractor refusing the file outright with "Failed to instantiate
|
||||
// extractor" because there is no moov atom to read. Only a *playable* file is a miss.
|
||||
val video = runCatching { videoMimeTypeOf(partial) }.getOrNull()
|
||||
partial.delete()
|
||||
if (video == null) return@runBlocking
|
||||
outcomes += "attempt $attempt produced a playable $video"
|
||||
}
|
||||
|
||||
fail(
|
||||
"never interrupted a running export in $CANCEL_ATTEMPTS attempts, so either every " +
|
||||
"export finished first or cancellation does not reach the transformer: $outcomes",
|
||||
)
|
||||
}
|
||||
|
||||
private fun videoMimeTypeOf(file: File): String? {
|
||||
val extractor = MediaExtractor()
|
||||
try {
|
||||
@@ -280,6 +371,19 @@ class Media3EngineTest {
|
||||
private companion object {
|
||||
const val TIMEOUT_SECONDS = 120L
|
||||
|
||||
/** Bounds the wait for the muxer to create the file; a hang here is a defect. */
|
||||
const val TIMEOUT_MS = 30_000L
|
||||
const val POLL_MS = 25L
|
||||
|
||||
/**
|
||||
* How long to let a *failed* cancel finish. Several times the export's own length, so
|
||||
* "the file is not playable" cannot mean "not yet".
|
||||
*/
|
||||
const val SETTLE_MS = 10_000L
|
||||
|
||||
/** See the KDoc: a miss is the loaded-runner case, not a defect. */
|
||||
const val CANCEL_ATTEMPTS = 5
|
||||
|
||||
/**
|
||||
* Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the
|
||||
* input outright — so anything approaching this is a hang, which is what the test is
|
||||
|
||||
@@ -13,6 +13,7 @@ import androidx.work.WorkManager
|
||||
import androidx.work.Worker
|
||||
import androidx.work.WorkerParameters
|
||||
import androidx.work.workDataOf
|
||||
import kotlinx.coroutines.CompletableDeferred
|
||||
import kotlinx.coroutines.flow.first
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.withTimeout
|
||||
@@ -27,7 +28,10 @@ import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.join.JoinState
|
||||
import org.libremediaconverter.join.JoinViewModel
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
import org.libremediaconverter.model.Engine
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
import org.libremediaconverter.model.QualityTier
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.libremediaconverter.work.JobTags
|
||||
@@ -64,6 +68,26 @@ class EchoWorker(context: Context, params: WorkerParameters) : Worker(context, p
|
||||
* path, foreground service included — into a synchronous test double, depending on class order.
|
||||
*/
|
||||
@UnstableApi
|
||||
/**
|
||||
* A [SoftwareTranscoder] that holds the worker in [WorkInfo.State.RUNNING] until released.
|
||||
*
|
||||
* Declared here rather than in `FakeFailures` because it is the only test that needs a job to stay
|
||||
* live on demand, and the shape is specific to that: the others fake a *failure*, this fakes
|
||||
* *duration*.
|
||||
*/
|
||||
private class BlockingTranscoder(private val released: CompletableDeferred<Unit>) : SoftwareTranscoder {
|
||||
override suspend fun run(
|
||||
request: ConversionRequest,
|
||||
inputPath: String,
|
||||
output: File,
|
||||
durationMs: Long,
|
||||
onProgress: (Int) -> Unit,
|
||||
) {
|
||||
released.await()
|
||||
output.writeBytes(ByteArray(1_024))
|
||||
}
|
||||
}
|
||||
|
||||
@RunWith(AndroidJUnit4::class)
|
||||
class ReattachOnLaunchTest {
|
||||
|
||||
@@ -75,7 +99,13 @@ class ReattachOnLaunchTest {
|
||||
fun clearTheQueue() = emptyQueueAndStaging()
|
||||
|
||||
@After
|
||||
fun leaveNothingBehind() = emptyQueueAndStaging()
|
||||
fun leaveNothingBehind() {
|
||||
// The suite runs without Android Test Orchestrator, so every class shares one process and
|
||||
// a swapped seam outlives the class that set it. Only one test here swaps one, but a
|
||||
// BlockingTranscoder left in place would hang the next class that converts anything.
|
||||
ConversionDependencies.reset()
|
||||
emptyQueueAndStaging()
|
||||
}
|
||||
|
||||
/**
|
||||
* The claim the whole fix rests on, checked against the production request builder rather
|
||||
@@ -261,6 +291,69 @@ class ReattachOnLaunchTest {
|
||||
return request.id
|
||||
}
|
||||
|
||||
/**
|
||||
* Reattaching to a conversion that is **running right now**, which nothing had ever driven.
|
||||
*
|
||||
* This class covers a job that finished, one whose staged file is gone, an ambiguous pair, one
|
||||
* still queued, and one the user cancelled. [Reattachment.rank] gives
|
||||
* [WorkInfo.State.RUNNING] the **highest** rank of all — "live work outranks a finished result
|
||||
* because a running job is holding a foreground service" — and no test on either source set
|
||||
* ever produced one. `ReattachmentTest` exercises the ranking as a pure function over
|
||||
* fabricated snapshots; what was missing is a ViewModel meeting a real running job.
|
||||
*
|
||||
* It is also the likeliest reattachment there is: the user starts a conversion, leaves, and
|
||||
* comes back while it is still going.
|
||||
*
|
||||
* ## Why the engine is a fake here, and why that is not a weakening
|
||||
*
|
||||
* The job has to still be running when the ViewModel is built, and every real conversion in
|
||||
* this suite finishes in about a second — racing that is what made the cancellation tests flaky
|
||||
* enough to need retries (#224). A [SoftwareTranscoder] that blocks until released removes the
|
||||
* race outright: the job is `RUNNING` for exactly as long as the test wants.
|
||||
*
|
||||
* Nothing about reattachment depends on which engine is transcoding. What is under test is the
|
||||
* tag query, [Reattachment.choose] over live WorkManager state, and `observe` mapping it to
|
||||
* [ConversionState.Converting] — all of which run identically whatever is doing the work.
|
||||
*
|
||||
* ## What this does not do, and cannot (#230)
|
||||
*
|
||||
* It does not kill the process. `docs/defect-audit.md` D3/D13 record that `am kill` refuses a
|
||||
* process holding a foreground service, and there is a more basic obstacle: **instrumentation
|
||||
* runs in the app's own process**, so any route that really killed it would take the test
|
||||
* runner with it and there would be nothing left to assert with. A relaunch-and-observe test
|
||||
* needs two instrumentation runs, which the runner does not provide.
|
||||
*
|
||||
* So process death stays device-manual, and this is the closest observable analogue: a fresh
|
||||
* ViewModel, with no memory of the work, meeting a job that is genuinely mid-flight.
|
||||
*/
|
||||
@Test
|
||||
fun reattachesToAConversionThatIsStillRunning(): Unit = runBlocking {
|
||||
val released = CompletableDeferred<Unit>()
|
||||
ConversionDependencies.software = { BlockingTranscoder(released) }
|
||||
|
||||
val request = ConversionWorker.request(
|
||||
inputUri = Uri.fromFile(stage("running_input.mp3")),
|
||||
displayName = RUNNING_NAME,
|
||||
sizeBytes = RUNNING_SIZE,
|
||||
spec = OutputFormat.MP3.spec,
|
||||
quality = QualityTier.FAST,
|
||||
)
|
||||
workManager.enqueue(request).result.get()
|
||||
|
||||
// Deterministic: the worker cannot finish until this test lets it.
|
||||
withTimeout(TIMEOUT_MS) {
|
||||
workManager.getWorkInfoByIdFlow(request.id).first { it?.state == WorkInfo.State.RUNNING }
|
||||
}
|
||||
|
||||
val reattached = awaitConversion<ConversionState.Converting>()
|
||||
|
||||
assertEquals(RUNNING_NAME, reattached.input.displayName)
|
||||
assertEquals(RUNNING_SIZE, reattached.input.sizeBytes)
|
||||
|
||||
released.complete(Unit)
|
||||
workManager.cancelWorkById(request.id).result.get()
|
||||
}
|
||||
|
||||
/**
|
||||
* Enqueues a job that stays [WorkInfo.State.ENQUEUED]. The delay is what holds it there: it
|
||||
* is long enough that nothing can run it during a test, and it is cancelled either way.
|
||||
@@ -326,5 +419,9 @@ class ReattachOnLaunchTest {
|
||||
* against WorkManager's database, so this is generous rather than tuned.
|
||||
*/
|
||||
const val SETTLE_MS = 5_000L
|
||||
|
||||
/** Read back off the job's tags by the reattaching ViewModel, so both have to survive. */
|
||||
const val RUNNING_NAME = "still_running.mp3"
|
||||
const val RUNNING_SIZE = 4_242L
|
||||
}
|
||||
}
|
||||
|
||||
@@ -25,6 +25,7 @@ import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.MediaProbe
|
||||
import org.libremediaconverter.convert.StagingNames
|
||||
import org.libremediaconverter.ffmpeg.ConcatEngine
|
||||
import org.libremediaconverter.ffmpeg.FFmpegEngine
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import java.io.File
|
||||
@@ -236,6 +237,55 @@ class ConcatEngineTest {
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* A failed join tells the user the return code and what FFmpeg said.
|
||||
*
|
||||
* **This is the device half of #203/#217**, whose PR closed by noting the join legs had not
|
||||
* been run. Running them would not have answered it: nothing on either source set drove a real
|
||||
* join *failure*, so the unified message was asserted only against values a JVM test hands to
|
||||
* `sessionOutcome` directly.
|
||||
*
|
||||
* What is device-only here is that the three reads behind that message work against a real
|
||||
* native session at all — `getReturnCode`, `getFailStackTrace` and `getAllLogsAsString`. If
|
||||
* the log tail came back null or empty on a device, the user would get `Joining failed (1): `
|
||||
* with nothing after the colon and every JVM test would still pass.
|
||||
*
|
||||
* **What this deliberately does not pin is the preference between the two detail sources.** On
|
||||
* an ordinary non-zero return code FFmpegKit reports no fail stack trace, so the stack-trace-
|
||||
* first rule and the log-tail-first rule produce the same text and no assertion here can tell
|
||||
* them apart. That ordering is [SessionOutcomeTest][org.libremediaconverter.ffmpeg.SessionOutcomeTest]'s
|
||||
* job, where both sources can be non-blank at once. Asserting it here would be a test whose
|
||||
* KDoc claims more than it checks — the `probeForConcat` mistake wave 3 caught.
|
||||
*
|
||||
* The failure is forced with an input that does not exist, which the concat demuxer rejects
|
||||
* the same way on every FFmpeg build, rather than with malformed media whose handling varies.
|
||||
*/
|
||||
@Test
|
||||
fun aFailedJoinReportsTheReturnCodeAndWhatFFmpegSaid(): Unit = runBlocking {
|
||||
val missing = File(context.cacheDir, "no_such_clip.mp4").also { it.delete() }
|
||||
val out = output("joined_failure.mp4")
|
||||
|
||||
val failure = runCatching {
|
||||
engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(missing)), out)
|
||||
}.exceptionOrNull()
|
||||
|
||||
assertTrue(
|
||||
"a join over a missing input must fail, got $failure",
|
||||
failure is FFmpegEngine.FFmpegException,
|
||||
)
|
||||
val message = failure?.message.orEmpty()
|
||||
assertTrue(
|
||||
"the message must name the operation and carry the return code, was: '$message'",
|
||||
message.startsWith("Joining failed ("),
|
||||
)
|
||||
// The half a JVM test cannot reach: a real session actually produced detail to show.
|
||||
val detail = message.substringAfter("): ", "")
|
||||
assertTrue(
|
||||
"the message stopped at the return code and told the user nothing, was: '$message'",
|
||||
detail.isNotBlank(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking {
|
||||
val out = output("joined_cleanup.mp4")
|
||||
|
||||
+66
-11
@@ -1,6 +1,6 @@
|
||||
# E2E-read findings
|
||||
|
||||
**Status:** six findings, none fixed, none urgent — **plus one confirmed vacuous test, which is a
|
||||
**Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a
|
||||
ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from
|
||||
the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation —
|
||||
something a new test would not fix, because the test already exists and the problem is what it
|
||||
@@ -242,6 +242,49 @@ visible skip or a red test, and that decision is the ticket's.
|
||||
|
||||
---
|
||||
|
||||
## E7 — a real `DocumentsProvider` cannot be reached without the picker, so there is no cheap SAF test
|
||||
|
||||
**Severity: n/a · Confirmed by measurement · this is a platform rule, not a gap**
|
||||
|
||||
Added 2026-09-06, from doing #225 and #226 rather than from reading.
|
||||
|
||||
`OutputPublisher.publish`'s destination side is asserted only against Robolectric fakes —
|
||||
`FakeSafProvider`, registered with `asDocumentsProvider = true`, which is the flag that *makes*
|
||||
`DocumentsContract.isDocumentUri` answer true. #226 split that into a cheap headless half (drive a
|
||||
real `DocumentsProvider` directly) and an expensive picker-driven half.
|
||||
|
||||
**The cheap half does not exist.** Three approaches, all measured on an API 34 emulator:
|
||||
|
||||
| approach | result |
|
||||
|---|---|
|
||||
| a second `DOCUMENTS_PROVIDER` declared **without** `MANAGE_DOCUMENTS` | refused at install: `SecurityException: Provider must be protected by MANAGE_DOCUMENTS` |
|
||||
| create the document as the **test APK**, which owns the provider | denied — instrumentation runs *in the target app's process*, so it carries the app's uid whatever `Context` is asked |
|
||||
| `uiAutomation.adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` | denied identically |
|
||||
|
||||
The denial names the only way in:
|
||||
|
||||
> `Permission Denial: opening provider …FixtureDocumentsProvider from
|
||||
> ProcessRecord{… org.libremediaconverter/u0a192} requires that you obtain access using
|
||||
> ACTION_OPEN_DOCUMENT or related APIs`
|
||||
|
||||
And the intent filter is not optional: without it `isDocumentUri` returns false, which is exactly
|
||||
the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing the
|
||||
ticket is about.
|
||||
|
||||
**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays
|
||||
#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so.
|
||||
|
||||
### What this does *not* block, which is the useful half
|
||||
|
||||
`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no
|
||||
documents provider. It opens a descriptor through the resolver, so **any readable `content://` URI
|
||||
exercises it**, and an ordinary `ContentProvider` may be exported without a permission. That is what
|
||||
`FixtureContentProvider` is, and it made #225 headless.
|
||||
|
||||
**That distinction was worth the trouble**: the first test ever to hand the join path a real
|
||||
`content://` input found #238, a defect that broke joining for every user who picks matched files.
|
||||
The expensive gate protects the *destination* side; the *input* side never needed it.
|
||||
|
||||
## Summary
|
||||
|
||||
| ID | Finding | Severity | Evidence | Action |
|
||||
@@ -249,11 +292,12 @@ visible skip or a red test, and that decision is the ticket's.
|
||||
| E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right |
|
||||
| E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 |
|
||||
| E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it |
|
||||
| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fix the sentence** |
|
||||
| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fixed** in #243 — it names the constant now |
|
||||
| E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not |
|
||||
| E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** |
|
||||
| E7 | A real `DocumentsProvider` is unreachable without the picker, so #226 has no cheap half | n/a | measured three ways on API 34; each denial names `ACTION_OPEN_DOCUMENT` | **no action** — it re-scoped #226 |
|
||||
|
||||
**Five of the six are prose, not code**, and that is the shape of this read. The instrumented suite
|
||||
**Six of the seven are prose, not code**, and that is the shape of this read. The instrumented suite
|
||||
is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation
|
||||
recipes, and the one class that asserts nothing says so in its first line. What this read found is
|
||||
that **the suite's self-description has drifted from the suite** in five small places and one large
|
||||
@@ -312,14 +356,25 @@ decision, not a detail — see **E6** for why no third option exists — and **#
|
||||
|
||||
| # | Gap |
|
||||
|---|---|
|
||||
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg |
|
||||
| **#224** | Cancelling a *running* native session, in any of the three engines |
|
||||
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider`, and the SAF premise it rests on |
|
||||
| **#227** | The notification's Cancel action has never been fired |
|
||||
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file |
|
||||
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere |
|
||||
| **#230** | *(spike)* whether a running conversion's process can be killed under instrumentation |
|
||||
| # | Gap | Outcome |
|
||||
|---|---|---|
|
||||
| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
|
||||
| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
|
||||
| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
|
||||
| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two |
|
||||
| **#227** | The notification's Cancel action has never been fired | closed |
|
||||
| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
|
||||
| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
|
||||
| **#230** | *(spike)* whether a running conversion's process can be killed | closed — it cannot; the runner shares the app's process |
|
||||
|
||||
**The read's own result, once the tickets were worked: one production defect.** #238 — joining files
|
||||
picked through the system picker failed outright on the stream-copy path, because the concat demuxer
|
||||
whitelists protocols separately from `-safe 0` and `ffkitsaf` was not on the list. Only `STREAM_COPY`
|
||||
feeds the demuxer a list file, and every existing join test passed `Uri.fromFile`, so the one broken
|
||||
combination was the only one a user could reach.
|
||||
|
||||
That is the argument for this kind of read in one line: the gap was not a missed line or an
|
||||
unasserted value, it was **a combination of two covered things that no test put together**.
|
||||
|
||||
**Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is
|
||||
the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before
|
||||
|
||||
Reference in New Issue
Block a user