Put a gate on the three pieces of wiring that had none
Three separate mutations passed the full 257-test suite, all for the same reason: the tool was tested and the thing that calls it was not. The join half of per-job staging. Reverting ConcatWorker to the constant the audit's own D8 table names -- "joined.<ext>", one string for every join of a format -- left everything green: PerJobStagingTest drives only the conversion worker, and StagingNamesTest pins only the pure function. The new case drives two real ConcatWorkers with different ids and reads what they asked for rather than what is on disk, because ConcatEngine is native, so neither join gets past it here and the catch on the way out deletes what it staged. The recorder moves into WorkerStubs, which is what that file is for, and the enum test that already had a private copy now uses it. The process-start sweep. Deleting the one line in LibreMediaConverterApp.onCreate() -- the only reason that class exists, and the backstop for every leak discardStaged cannot reach -- left everything green too. The test stages one file a day old and one written now, calls onCreate() again, and asserts both halves: the abandoned one is collected and the live one is not. The second half is what says this is a sweep rather than the clearStaging() it replaced, which could take a file out from under a running job. The mtime is set explicitly, because "written long enough ago" is not something a test can wait for when the period is twenty-four hours. Casting the Robolectric application to LibreMediaConverterApp is an assertion in itself: it fails if android:name ever stops pointing here, in which case the swept line would be correct code that never runs. The backup and device-transfer exclusions. Reverting data_extraction_rules.xml to the template's boilerplate left the unit tests green AND lintDebug green -- it is a resource, so nothing was reading it -- and the failure it causes is one nobody meets in development. WorkManager's queue is the app's whole backup payload, and its rows name content:// grants and cacheDir paths that do not survive a transfer; reattachment queries by tag on launch, so a fresh install would come up attached to a job the user never ran on it. The test reads the compiled resource table, so what it pins is what the APK carries, and it asserts domain and path for all four entries in both sections -- an <exclude> with no path is skipped unchecked by lint's own detector, so half an entry could protect nothing. Its KDoc records the one thing it does not cover: the manifest attribute that points the system at the file. R8 / #17, R9 / #18, R11 / #20 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,98 @@
|
||||
package org.libremediaconverter
|
||||
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Assert.fail
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.StagingSweep
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
import java.util.concurrent.TimeUnit
|
||||
|
||||
/**
|
||||
* That process start actually sweeps.
|
||||
*
|
||||
* [StagingSweepTest][org.libremediaconverter.convert.StagingSweepTest] pins the age rule and
|
||||
* `OutputPublisherStagingTest` pins the sweep against a real filesystem; neither says anything
|
||||
* about whether anything calls it, and deleting the one line that does left the whole suite green.
|
||||
* That line is the only reason this Application class exists, and it is the backstop for every leak
|
||||
* `discardStaged` cannot reach — a process reclaimed before a save, a worker that failed before its
|
||||
* output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity.
|
||||
*
|
||||
* `onCreate()` is called again rather than a second Application being built: it is what the
|
||||
* framework calls at process start, the scope it launches on is already there, and the first test
|
||||
* below is what pins that the framework calls it on *this* class.
|
||||
*/
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class AppStartSweepTest {
|
||||
|
||||
private lateinit var app: LibreMediaConverterApp
|
||||
private lateinit var stagingDir: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
// The cast is an assertion in itself: Robolectric builds the Application named in the
|
||||
// merged manifest, so this fails if `android:name` ever stops pointing here -- in which
|
||||
// case the sweep below would be perfectly correct code that never runs.
|
||||
app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp
|
||||
stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() }
|
||||
stagingDir.listFiles()?.forEach { it.delete() }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the application the manifest starts is the one that sweeps`() {
|
||||
assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `process start collects an abandoned staged file and leaves a live one alone`() {
|
||||
val abandoned = stagedFile("abandoned.mp4")
|
||||
val live = stagedFile("live.mp4")
|
||||
// Set explicitly. Relying on a file being written "long enough ago" is not something a test
|
||||
// can arrange, and the grace period is a day.
|
||||
assertTrue(
|
||||
abandoned.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - ONE_MINUTE_MS),
|
||||
)
|
||||
|
||||
// Both files are still here on the way in. The Application was already constructed once
|
||||
// before this test ran, so without this the sweep that call started could be the one that
|
||||
// collected the file, and the assertion below would be about the wrong process start.
|
||||
assertTrue(abandoned.exists() && live.exists())
|
||||
|
||||
app.onCreate()
|
||||
|
||||
awaitGone(abandoned)
|
||||
// The other half, and the one that says the sweep is a sweep rather than a
|
||||
// `clearStaging()`: the directory is shared by the convert tab, the join tab and
|
||||
// ConcatEngine's list file, so deleting everything could take a file from a running job.
|
||||
assertTrue("a file written moments ago belongs to a live job", live.exists())
|
||||
}
|
||||
|
||||
/**
|
||||
* Waits for [file] to be deleted.
|
||||
*
|
||||
* The sweep runs on `Dispatchers.IO`, deliberately: it lists a directory and stats every entry
|
||||
* on the path that decides how long the launcher icon stays unresponsive. So there is nothing
|
||||
* to join, and the wait is a bounded poll — long enough for a directory listing, short enough
|
||||
* that a sweep which never happens fails rather than hangs.
|
||||
*/
|
||||
private fun awaitGone(file: File) {
|
||||
val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS)
|
||||
while (System.nanoTime() < deadline) {
|
||||
if (!file.exists()) return
|
||||
Thread.sleep(POLL_INTERVAL_MS)
|
||||
}
|
||||
fail("process start left ${file.name} in staging; nothing swept it")
|
||||
}
|
||||
|
||||
private fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) }
|
||||
|
||||
private companion object {
|
||||
const val ONE_MINUTE_MS = 60L * 1000
|
||||
const val AWAIT_TIMEOUT_SECONDS = 10L
|
||||
const val POLL_INTERVAL_MS = 5L
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,106 @@
|
||||
package org.libremediaconverter
|
||||
|
||||
import android.content.res.XmlResourceParser
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import org.xmlpull.v1.XmlPullParser
|
||||
|
||||
/**
|
||||
* What the app lets leave the device.
|
||||
*
|
||||
* `data_extraction_rules.xml` is a resource rather than code, so nothing was checking it: reverting
|
||||
* the whole file to the template's boilerplate left the unit tests green AND `lintDebug` green, and
|
||||
* a future edit dropping the excludes would ship in silence. The failure it would cause is one
|
||||
* nobody meets in development — a cloud restore or a device-to-device transfer.
|
||||
*
|
||||
* What is at stake is written in the file itself. WorkManager's queue is the app's entire backup
|
||||
* payload, and every row in it references a `content://` URI granted to one install on one device
|
||||
* and an output path under that install's `cacheDir`. Neither survives the transfer, and the rows
|
||||
* are not inert when they arrive: reattachment queries WorkManager by tag on launch, so a fresh
|
||||
* install would come up attached to a job the user never ran on it.
|
||||
*
|
||||
* Read out of the compiled resource table rather than off `src/main/res`, so what is asserted is
|
||||
* what the APK actually carries. Note the limit of that: this pins the rules' content, not the
|
||||
* `android:dataExtractionRules` attribute that points the system at them.
|
||||
*/
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class BackupExclusionsTest {
|
||||
|
||||
@Test
|
||||
fun `the work queue is excluded from cloud backup and from device transfer alike`() {
|
||||
// Both sections, because they are separately honoured: `allowBackup` stays true and the
|
||||
// exclusion is per-file, so an edit that dropped either half would leave the other looking
|
||||
// like the whole answer.
|
||||
assertEquals(
|
||||
mapOf(
|
||||
"cloud-backup" to WORK_MANAGER_STATE,
|
||||
"device-transfer" to WORK_MANAGER_STATE,
|
||||
),
|
||||
excludesBySection(),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Every `<exclude>` in the rules, as `domain:path`, grouped by the section it sits in.
|
||||
*
|
||||
* Both halves of each entry, because an `<exclude>` carrying no path is skipped unchecked by
|
||||
* lint's own detector — so that spelling could protect nothing while still looking like a rule.
|
||||
*/
|
||||
private fun excludesBySection(): Map<String, Set<String>> {
|
||||
// Both sections start present and empty, so a section deleted outright fails as an empty
|
||||
// set rather than as a missing key -- the same finding either way, said the same way.
|
||||
val found = SECTIONS.associateWith { mutableSetOf<String>() }
|
||||
var section: String? = null
|
||||
RuntimeEnvironment.getApplication().resources.getXml(R.xml.data_extraction_rules).use { parser ->
|
||||
while (parser.next() != XmlPullParser.END_DOCUMENT) {
|
||||
section = parser.sectionAfter(section, found)
|
||||
}
|
||||
}
|
||||
return found
|
||||
}
|
||||
|
||||
/** Folds one parse event into [found], and answers which section the parser is now inside. */
|
||||
private fun XmlResourceParser.sectionAfter(section: String?, found: Map<String, MutableSet<String>>): String? =
|
||||
when {
|
||||
eventType == XmlPullParser.START_TAG && name in SECTIONS -> name
|
||||
eventType == XmlPullParser.END_TAG && name == section -> null
|
||||
eventType == XmlPullParser.START_TAG && name == "exclude" && section != null ->
|
||||
section.also { found.getValue(it) += entry() }
|
||||
|
||||
else -> section
|
||||
}
|
||||
|
||||
private fun XmlResourceParser.entry(): String = "${attribute("domain")}:${attribute("path")}"
|
||||
|
||||
/**
|
||||
* The value of the attribute called [name] on the current tag.
|
||||
*
|
||||
* Walked by index rather than looked up by namespace. These attributes carry the `android`
|
||||
* namespace in the source file, but a parser over the *compiled* resource reports them with
|
||||
* none, so `getAttributeValue(namespace, name)` answers null for every one of them.
|
||||
*/
|
||||
private fun XmlResourceParser.attribute(name: String): String? =
|
||||
(0 until attributeCount).firstOrNull { getAttributeName(it) == name }?.let { getAttributeValue(it) }
|
||||
|
||||
private companion object {
|
||||
/** The two ways data leaves a device, both of which these rules have to answer. */
|
||||
val SECTIONS = setOf("cloud-backup", "device-transfer")
|
||||
|
||||
/**
|
||||
* WorkManager's own storage, spelled the way WorkManager spells it.
|
||||
*
|
||||
* The database is Room-backed and therefore in WAL mode, hence the two sidecars. Pinning
|
||||
* the spelling is the point rather than a cost: a WorkManager release renaming its database
|
||||
* would silently un-exclude the queue, and this failing is how anyone would find out.
|
||||
*/
|
||||
val WORK_MANAGER_STATE = setOf(
|
||||
"database:androidx.work.workdb",
|
||||
"database:androidx.work.workdb-wal",
|
||||
"database:androidx.work.workdb-shm",
|
||||
"sharedpref:androidx.work.util.preferences.xml",
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -10,11 +10,11 @@ import kotlinx.coroutines.runBlocking
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
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.installTestWorkManager
|
||||
import org.libremediaconverter.model.DeviceCodecs
|
||||
import org.libremediaconverter.model.EnginePreference
|
||||
@@ -35,19 +35,24 @@ import java.util.UUID
|
||||
*
|
||||
* These drive the real worker rather than the naming function, because the naming function was
|
||||
* never the part that was wrong. What was wrong is which name the worker asked for.
|
||||
*
|
||||
* Both workers, for the same reason. The join side collided harder — `joined.<ext>` is one string
|
||||
* for every join of a format, where a conversion at least needed two inputs of the same name — and
|
||||
* it was the half with no test at all: reverting `ConcatWorker` to that constant left all 257
|
||||
* tests green.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class PerJobStagingTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: OutputPublisher
|
||||
private lateinit var publisher: NamingPublisher
|
||||
private lateinit var stagingDir: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = AlwaysRoomPublisher(app)
|
||||
publisher = NamingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
|
||||
@@ -56,6 +61,9 @@ class PerJobStagingTest {
|
||||
|
||||
stagingDir = publisher.createStagingFile("anything").parentFile!!
|
||||
stagingDir.listFiles()?.forEach { it.delete() }
|
||||
// Asking for the directory above is itself a staging request; the tests are about the ones
|
||||
// the workers make.
|
||||
publisher.requestedNames.clear()
|
||||
}
|
||||
|
||||
@After
|
||||
@@ -104,8 +112,34 @@ class PerJobStagingTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `two joins of the same format stage under names of their own`() {
|
||||
runBlocking { concatWorker(JOB_A).doWork() }
|
||||
runBlocking { concatWorker(JOB_B).doWork() }
|
||||
|
||||
// Read off what the worker asked for rather than off the directory, and not for
|
||||
// convenience: ConcatEngine is native, so neither join gets past it here, and the catch on
|
||||
// the way out deletes whatever was staged. The name is where the collision lived --
|
||||
// "joined.${format.extension}" is one string for every join of a format, so two joins were
|
||||
// one file, exactly as two conversions of a same-named input were.
|
||||
val names = publisher.requestedNames
|
||||
assertEquals("each join must stage under a name of its own, got $names", 2, names.toSet().size)
|
||||
assertTrue("the first join's name must carry its own job id, got ${names[0]}", names[0].contains("$JOB_A"))
|
||||
assertTrue("the second join's name must carry its own job id, got ${names[1]}", names[1].contains("$JOB_B"))
|
||||
}
|
||||
|
||||
private fun stagedNames(): List<String> = stagingDir.listFiles().orEmpty().map { it.name }.sorted()
|
||||
|
||||
private fun concatWorker(id: UUID): 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 JOIN_FORMAT.name,
|
||||
),
|
||||
runAttemptCount = 0,
|
||||
).setId(id).build()
|
||||
|
||||
private fun conversionWorker(
|
||||
id: UUID,
|
||||
runAttemptCount: Int = 0,
|
||||
@@ -129,6 +163,7 @@ class PerJobStagingTest {
|
||||
const val DISPLAY_NAME = "input.mp4"
|
||||
const val INPUT_BYTES = 1024L
|
||||
val SPEC = OutputFormat.MP4_H265.spec
|
||||
val JOIN_FORMAT = OutputFormat.MP4_H264
|
||||
val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a")
|
||||
val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b")
|
||||
}
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
package org.libremediaconverter.work
|
||||
|
||||
import android.app.Application
|
||||
import android.content.Context
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.Data
|
||||
@@ -15,7 +14,6 @@ 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.StagingNames
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
@@ -85,7 +83,7 @@ class WorkerEnumFallbackTest {
|
||||
fun `an engine preference this build does not define does not end the job`() {
|
||||
// Refused on space, which is the first thing below the three reads: it proves the reads
|
||||
// were reached and returned, without dragging in a routing decision this test is not about.
|
||||
publisher.refuse = true
|
||||
publisher.refuseSpace = true
|
||||
|
||||
val result = runBlocking { conversionWorker(preference = "FORCE_QUANTUM").doWork() }
|
||||
|
||||
@@ -153,25 +151,6 @@ class WorkerEnumFallbackTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A real [OutputPublisher] that records the staging names it is asked for, and can refuse on space.
|
||||
*
|
||||
* The name is the only place a join's format is legible from outside: the engine that would use it
|
||||
* is native, and the worker deletes the staged file on its way out of a failed attempt.
|
||||
*/
|
||||
private class NamingPublisher(context: Context) : OutputPublisher(context) {
|
||||
|
||||
val requestedNames = mutableListOf<String>()
|
||||
var refuse = false
|
||||
|
||||
override fun hasSpaceFor(bytes: Long): Boolean = !refuse
|
||||
|
||||
override fun createStagingFile(name: String): File {
|
||||
requestedNames += name
|
||||
return super.createStagingFile(name)
|
||||
}
|
||||
}
|
||||
|
||||
/** An engine that writes the output and remembers what it was asked to produce. */
|
||||
private class RequestRecordingTranscoder : SoftwareTranscoder {
|
||||
|
||||
|
||||
@@ -24,6 +24,31 @@ open class AlwaysRoomPublisher(context: Context) : OutputPublisher(context) {
|
||||
override fun hasSpaceFor(bytes: Long): Boolean = true
|
||||
}
|
||||
|
||||
/**
|
||||
* An [AlwaysRoomPublisher] that records the staging names it is asked for.
|
||||
*
|
||||
* For a conversion the staged file survives the job and a directory listing says everything. For a
|
||||
* join it does not: `ConcatEngine` is native, so no test here gets past it, and the catch on the way
|
||||
* out deletes what was staged. The name the worker *asked* for is then the only place its job id
|
||||
* and its output format are legible at all — the same reason `SpaceCheckTest` records the question
|
||||
* rather than the verdict.
|
||||
*/
|
||||
open class NamingPublisher(context: Context) : AlwaysRoomPublisher(context) {
|
||||
|
||||
/** Every name passed to [createStagingFile], in order. */
|
||||
val requestedNames = mutableListOf<String>()
|
||||
|
||||
/** Set to refuse every space check, the way `FakeFailures.FullDisk` does. */
|
||||
var refuseSpace = false
|
||||
|
||||
override fun hasSpaceFor(bytes: Long): Boolean = !refuseSpace
|
||||
|
||||
override fun createStagingFile(name: String): File {
|
||||
requestedNames += name
|
||||
return super.createStagingFile(name)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* An engine that writes the output file and nothing else.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user