diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 8d75d97..9dd3c42 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.OutOfQuotaPolicy import androidx.work.WorkerParameters import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf @@ -27,6 +28,10 @@ import org.libremediaconverter.model.OutputFormat * Progress is not reported. FFmpeg's statistics callback gives a timestamp against a * single input's duration, which is meaningless once several files are being * concatenated; showing a fabricated percentage would be worse than showing none. + * + * Enqueued as **expedited** work for the same reasons, and with the same caveats, as + * [ConversionWorker] — its class KDoc carries both, and a join is user-initiated in exactly the + * way a conversion is. */ @UnstableApi class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) { @@ -68,13 +73,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker // which is where a WorkManager restart after process death always begins -- used to // throw straight past this catch, taking the retry, the error message and the delete // with it. See ConversionWorker.doWork and FailureOutcome. - setForeground( - ForegroundInfo( - NOTIFICATION_ID, - notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true), - ConversionForegroundType.current(), - ), - ) + // + // Posted through getForegroundInfo() rather than built here a second time -- see that + // override, and its twin in ConversionWorker. + setForeground(getForegroundInfo()) val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format) Result.success( @@ -129,11 +131,29 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker return publisher.hasSpaceFor(bytes) } - override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo( - NOTIFICATION_ID, - notifications.build(id, "Joining files", 0, indeterminate = true), - ConversionForegroundType.current(), - ) + /** + * The notification a starting join posts, and now the only definition of it. + * + * WorkManager's hook for expedited work, which **will not call this on any device this app + * supports** — see [ConversionWorker.getForegroundInfo] for the measurement and for why + * `setExpedited` alone would have left these lines exactly as cold as they were. What makes + * them live is [doWork] posting this instead of building its own copy. + * + * It counts the inputs itself rather than being handed the number, so that it is still answerable + * before [doWork] has parsed anything — which is the contract WorkManager's own caller wants. + * The count is read from the same key, so the two cannot disagree. The `?: 0` arm is + * unreachable and named rather than covered: [doWork] refuses a job with no URI array several + * lines above this call, and nothing else calls it. It is the shape `docs/coverage-read-findings.md` + * calls F4 — a second line of defence that cannot be provoked. + */ + override suspend fun getForegroundInfo(): ForegroundInfo { + val inputCount = inputData.getStringArray(KEY_INPUT_URIS)?.size ?: 0 + return ForegroundInfo( + NOTIFICATION_ID, + notifications.build(id, joiningTitle(inputCount), 0, indeterminate = true), + ConversionForegroundType.current(), + ) + } companion object { /** @@ -204,6 +224,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker */ fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}" + /** + * What the progress notification says while a join runs. + * + * Named once, for the convention #158 established about strings the user can see. It was + * two strings until 2026-09-06 — `"Joining N files"` built inline in [doWork] and a + * countless `"Joining files"` in [getForegroundInfo] — for one notification that only ever + * had one job, and the copy nothing executed was free to drift from the one that did. + */ + fun joiningTitle(inputCount: Int): String = "Joining $inputCount files" + private const val NOTIFICATION_ID = 1002 private const val TAG = "ConcatWorker" @@ -215,6 +245,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker */ fun request(inputs: List, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) = OneTimeWorkRequestBuilder() + // Expedited, exactly as ConversionWorker.request is and for the same reasons; that + // one's comment and class KDoc carry them. Nothing here sets an initial delay or a + // constraint, which is what makes it legal for `build()` to accept. + .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .addTag(JobTags.inputCount(inputs.size)) .setInputData( Data.Builder() diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 8e13e01..f52d4ee 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.OutOfQuotaPolicy import androidx.work.WorkerParameters import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf @@ -40,8 +41,28 @@ import java.io.File * observe. That durability is what makes the six-hour foreground-service timeout * recoverable instead of fatal. * - * Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs - * with a short quota, which is the wrong shape for a multi-minute transcode. + * Enqueued as **expedited** work, with `RUN_AS_NON_EXPEDITED_WORK_REQUEST`. This paragraph said + * the opposite until 2026-09-06 — "deliberately *not* used… the wrong shape for a multi-minute + * transcode" — and the quota it named does not reach a transcode the way it reads: + * + * - The quota belongs to the *JobScheduler* job, and `SystemJobInfoConverter:135` in + * work-runtime 2.11.2 sets `JobInfo.setExpedited(true)` only when `!isRetry && !isDelayed`. + * A retry is therefore scheduled exactly as every job is scheduled today. + * - A job the system stops mid-run does not get its answer from [FailureOutcome]. + * `WorkerWrapper.interrupt` cancels the worker's coroutine with a `WorkerStoppedException`, + * which its `launch` resolves as `ResetWorkerStatus` — the worker's own `Result` is discarded + * and the work re-enqueued with backoff, whatever it returned. So a quota stop is a retry, and + * the `CancellationException` arm in [doWork] is what deletes the partial on the way through. + * + * What it buys is narrower than "conversions start sooner", and the narrowness is the honest part: + * `GreedyScheduler` starts unconstrained, undelayed work in-process the moment it is enqueued and + * carries no `expedited` branch at all, so a conversion begun from the open app runs exactly when + * it ran before — the common case does not move. The flag is for the job that has to go *through* + * JobScheduler because no process is left to start it: one still enqueued when the app died. + * `SystemJobScheduler.schedule` re-converts the spec every time it schedules, so such a job is + * expedited on the way back in, and a retried one is not. `RUN_AS_NON_EXPEDITED_WORK_REQUEST` + * rather than `DROP_WORK_REQUEST`: an invisible quota is no reason to throw a user's conversion + * away, and `SystemJobScheduler:198` degrades it to an ordinary job instead. * * That durability is not free, and the queue surviving is not the same as the job surviving. * When WorkManager recovers a job after process death the app is by definition in the background, @@ -60,7 +81,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo override suspend fun doWork(): Result { val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse) ?: return Result.failure(workDataOf(KEY_ERROR to "No input file.")) - val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input" + val displayName = displayName() // Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what // made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free". val declaredSize = inputData @@ -100,7 +121,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo // process death is. With it above the try that throw escaped doWork() entirely: no // retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below // was outside for the same reason and had the same problem. - setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true)) + // + // Posted through getForegroundInfo() rather than built here a second time -- see that + // override for what the duplicate cost. + setForeground(getForegroundInfo()) // Through the seam rather than MediaProbe directly. The seam already existed for the // ViewModel and the worker was the last caller bypassing it, which is why nothing on @@ -339,8 +363,32 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo return OutputSpec(container, video, audio) } + /** + * What this job's input is called, or [InputQuery.FALLBACK_DISPLAY_NAME] when nothing named it. + * + * One read rather than the two copies of `?: "input"` that [doWork] and [getForegroundInfo] + * each carried, and against `InputQuery`'s constant rather than a third literal of the same + * string: it is the same fallback the picker uses, and it reaches the save dialog as + * `input_converted.mp4`. + */ + private fun displayName(): String = inputData.getString(KEY_DISPLAY_NAME) ?: InputQuery.FALLBACK_DISPLAY_NAME + + /** + * The notification a starting conversion posts, and now the only definition of it. + * + * This is WorkManager's hook for expedited work, and **it will not be called on any device + * this app supports.** `WorkForeground.kt:38` in work-runtime 2.11.2 opens with + * `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's + * only caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So #252's premise — that + * enqueueing expedited work would make these lines live — is false, and `setExpedited` alone + * would have left them exactly as cold as the first instrumented coverage read found them. + * + * What makes them live is [doWork] posting *this* instead of building its own copy. The two + * were identical — same title, `percent = 0`, `indeterminate = true` — so one was a duplicate + * that could drift, and the one nothing executed is the one that would have drifted silently. + */ override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo( - inputData.getString(KEY_DISPLAY_NAME) ?: "input", + displayName(), percent = 0, indeterminate = true, ) @@ -416,6 +464,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo quality: QualityTier = QualityTier.FAST, enginePreference: EnginePreference = EnginePreference.AUTO, ) = OneTimeWorkRequestBuilder() + // Expedited, so the jobs that do go through JobScheduler are treated as the + // user-initiated work they are -- see the class KDoc for what that is and is not worth. + // Safe to set here and only because of what this builder does not do: `build()` refuses + // an expedited request carrying an initial delay or any constraint but network and + // storage, and none of the three is set below. + .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .addTag(JobTags.displayName(displayName)) // Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has // no null, so the absence of the key *is* the unknown — and a tag reading diff --git a/app/src/test/java/org/libremediaconverter/work/ExpeditedRequestTest.kt b/app/src/test/java/org/libremediaconverter/work/ExpeditedRequestTest.kt new file mode 100644 index 0000000..83f20b9 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/ExpeditedRequestTest.kt @@ -0,0 +1,102 @@ +package org.libremediaconverter.work + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Constraints +import androidx.work.OutOfQuotaPolicy +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Both workers enqueue **expedited** work, and stay legal doing it. + * + * The two questions are separate and only one of them is about the flag. + * + * - **Is it set.** `expedited` is `false` by default, so `assertTrue` here is what a deleted + * `setExpedited(...)` reddens. That mutation was run. + * - **Is it legal.** `WorkRequest.Builder.build()` refuses an expedited request that carries an + * initial delay or any constraint but network and storage — `require(workSpec.initialDelay <= 0) + * { "Expedited jobs cannot be delayed" }` in work-runtime 2.11.2. Neither `request` sets either + * today, so both `build()` calls pass and the `IllegalArgumentException` is a *future* hazard + * rather than a current one. The delay and constraints assertions below are what name it: add a + * delay to either builder and this class fails on the throw, in the same second, instead of the + * app failing to enqueue a conversion on a device. + * + * **The policy assertion bites less than it reads, and that is worth writing down rather than + * leaving to be rediscovered.** `WorkSpec.outOfQuotaPolicy` *defaults* to + * `RUN_AS_NON_EXPEDITED_WORK_REQUEST`, so it is already this value on a request that was never + * expedited at all — deleting `setExpedited` does not redden it. What it does pin is the one + * alternative: `DROP_WORK_REQUEST` throws a user's conversion away because an invisible quota ran + * out, and that mutation *is* red here. + * + * The delay is not hypothetical either. Three tests deliberately build a delayed request to hold a + * job in `ENQUEUED` — `NotificationCancelActionTest`, `ReattachOnLaunchTest` and + * `CancelReachesWorkManagerTest` — and every one of them builds its own + * `OneTimeWorkRequestBuilder` rather than adding a delay to what `request` returns. That is why + * making these expedited broke none of them; the one that starts from `request` takes only + * `base.workSpec.input` from it. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ExpeditedRequestTest { + + @Test + fun `a conversion is enqueued as expedited work`() { + val spec = ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec + + assertTrue("a conversion the user asked for has to be expedited work", spec.expedited) + assertEquals( + "a quota nobody can see is no reason to drop a conversion", + OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST, + spec.outOfQuotaPolicy, + ) + } + + @Test + fun `a join is enqueued as expedited work`() { + val spec = ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec + + assertTrue("a join the user asked for has to be expedited work", spec.expedited) + assertEquals( + "a quota nobody can see is no reason to drop a join", + OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST, + spec.outOfQuotaPolicy, + ) + } + + /** + * The two properties that keep `build()` from throwing, asserted on both requests at once + * because the rule is WorkManager's rather than either worker's. + */ + @Test + fun `neither expedited request carries what would make it illegal`() { + val requests = listOf( + ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec, + ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec, + ) + + requests.forEach { spec -> + assertEquals( + "expedited work cannot be delayed: ${spec.workerClassName}", + 0L, + spec.initialDelay, + ) + assertEquals( + "expedited work takes only network and storage constraints: ${spec.workerClassName}", + Constraints.NONE, + spec.constraints, + ) + } + } + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday2.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1_024L + const val TOTAL_BYTES = 2_048L + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/ForegroundNotificationTest.kt b/app/src/test/java/org/libremediaconverter/work/ForegroundNotificationTest.kt new file mode 100644 index 0000000..00b2d6a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/ForegroundNotificationTest.kt @@ -0,0 +1,203 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.app.Notification +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +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.ConcatJoiner +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.ffmpeg.ConcatEngine +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * The first thing either worker posts is what its own `getForegroundInfo()` builds. + * + * **Both overrides were dead code until 2026-09-06, and #252 is where that was found** — the first + * instrumented coverage read reported `ConversionWorker:342-346` and `ConcatWorker:132-136` among + * the 32 lines *neither* suite reaches. The ticket's premise was that enqueueing expedited work + * would make them live, since `getForegroundInfo()` is WorkManager's expedited-work hook. + * + * **That premise is false at this `minSdk`, which is the finding underneath the fix.** + * `WorkForeground.kt:38` in work-runtime 2.11.2 opens `workForeground` with + * `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's only + * caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So `setExpedited` alone would have left + * both overrides exactly as cold as the read found them, and a test written to drive them through + * WorkManager would be testing a code path no device this app supports can take — E1's failure + * mode, where a test asserts and never reaches. + * + * What makes them live is a single-definition change instead. Each worker had **two** definitions + * of one notification: the override, and an identical `ForegroundInfo` built inline in `doWork`. + * `doWork` now posts the override's, so the copy nothing executed is gone and the one that remains + * runs on every job. + * + * These tests are what hold that wiring. Each asserts the notification's *contents* against + * constants rather than against `worker.getForegroundInfo()` — comparing the two would move + * together under every mutation and stay green — and the mutations that redden them are named on + * each test. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ForegroundNotificationTest { + + private lateinit var app: Application + private lateinit var updater: RecordingForegroundUpdater + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + updater = RecordingForegroundUpdater() + ConversionDependencies.publisher = { AlwaysRoomPublisher(app) } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + ConversionDependencies.software = { WritingTranscoder } + ConversionDependencies.concat = { WritingJoiner } + // The notification carries a WorkManager cancel PendingIntent, so without this the worker + // fails building the notification rather than on anything these tests are about. + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * Mutation that must go red, and did: inside `ConversionWorker.getForegroundInfo`, replace + * `displayName()` with a literal, or `percent = 0` with anything else. Both are in the override's + * own body, so a red here is proof `doWork` executes it rather than a copy of it. + */ + @Test + fun `a conversion's first foreground post is the one getForegroundInfo builds`() { + runBlocking { conversionWorker().doWork() } + + val first = updater.infos.first() + val extras = first.notification.extras + assertEquals( + "the notification has to name the file the user picked", + DISPLAY_NAME, + extras.getString(Notification.EXTRA_TITLE), + ) + assertEquals("a conversion starts at zero", 0, extras.getInt(Notification.EXTRA_PROGRESS)) + assertTrue( + "nothing is known about the length of the job yet, so the bar is indeterminate", + extras.getBoolean(Notification.EXTRA_PROGRESS_INDETERMINATE), + ) + assertEquals( + "the foreground service type is the regime's, not zero", + ConversionForegroundType.current(), + first.foregroundServiceType, + ) + } + + /** + * The count is the point. + * + * `getForegroundInfo` said `"Joining files"` and `doWork` said `"Joining N files"` — one + * notification with two texts, and the one nothing ran was free to drift. Now there is one, + * and it reads the input array itself so it can still answer before `doWork` has parsed + * anything. + * + * Mutation that must go red, and did: replace the array read in `ConcatWorker.getForegroundInfo` + * with a constant `0`, which yields `"Joining 0 files"`. Asserting merely that the title starts + * with "Joining" would survive that, which is why the whole string is pinned. + */ + @Test + fun `a join's first foreground post counts the files it was given`() { + runBlocking { joinWorker().doWork() } + + val first = updater.infos.first() + assertEquals( + "the notification has to say how many files are being joined", + ConcatWorker.joiningTitle(INPUTS.size), + first.notification.extras.getString(Notification.EXTRA_TITLE), + ) + assertEquals( + "the foreground service type is the regime's, not zero", + ConversionForegroundType.current(), + first.foregroundServiceType, + ) + } + + /** + * And the title is really the file's name rather than any string at all. + * + * [ConcatWorker.joiningTitle] is asserted above through the constant the worker itself uses, so + * that assertion cannot catch the sentence being reworded — deliberately, since the wording is + * not what the test is about. This one can: two files, two names, one worker each. + */ + @Test + fun `two conversions of differently named files post differently named notifications`() { + runBlocking { conversionWorker(displayName = OTHER_NAME).doWork() } + + assertEquals( + OTHER_NAME, + updater.infos.first().notification.extras.getString(Notification.EXTRA_TITLE), + ) + } + + private fun conversionWorker(displayName: String = DISPLAY_NAME) = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to displayName, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + // FORCE_SOFTWARE is the one preference that decides without consulting the input, + // and a file:// URI keeps the worker out of FFmpegKit's native SAF bridge. + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID) + .setForegroundUpdater(updater) + .build() + + private fun joinWorker() = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to INPUTS.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES, + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID) + .setForegroundUpdater(updater) + .build() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + val INPUTS: List = listOf(INPUT, Uri.parse("file:///tmp/holiday2.mp4")) + const val DISPLAY_NAME = "holiday.mp4" + const val OTHER_NAME = "birthday.mkv" + const val INPUT_BYTES = 1_024L + const val TOTAL_BYTES = 2_048L + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000252") + } +} + +/** A joiner that writes an output and reports a strategy; nothing here is about the engine. */ +private object WritingJoiner : ConcatJoiner { + override suspend fun join(inputs: List, output: File, format: OutputFormat): ConcatEngine.Result { + output.writeBytes(ByteArray(OUTPUT_BYTES)) + return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output) + } + + private const val OUTPUT_BYTES = 512 +} diff --git a/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt b/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt index cbb8402..83d9dd7 100644 --- a/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt @@ -3,16 +3,12 @@ package org.libremediaconverter.work import android.app.Application import android.app.Notification import android.app.NotificationManager -import android.content.Context import android.net.Uri import androidx.media3.common.util.UnstableApi import androidx.work.Data -import androidx.work.ForegroundInfo import androidx.work.WorkInfo -import androidx.work.testing.TestForegroundUpdater import androidx.work.testing.TestListenableWorkerBuilder import androidx.work.workDataOf -import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals @@ -224,26 +220,6 @@ class ProgressNotificationTest { } } -/** - * Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default. - * - * Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture`: the - * worker awaits what this returns, so a future that never completes would hang the initial - * `setForeground` rather than test anything. - */ -private class RecordingForegroundUpdater : TestForegroundUpdater() { - val infos = mutableListOf() - - override fun setForegroundAsync( - context: Context, - id: UUID, - foregroundInfo: ForegroundInfo, - ): ListenableFuture { - infos += foregroundInfo - return super.setForegroundAsync(context, id, foregroundInfo) - } -} - /** An engine that reports whatever [report] wants reported, then writes an output. */ private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder { override suspend fun run( diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt index 14f3abe..d515d52 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt @@ -1,11 +1,14 @@ package org.libremediaconverter.work import android.content.Context +import androidx.work.ForegroundInfo +import androidx.work.testing.TestForegroundUpdater import com.google.common.util.concurrent.ListenableFuture import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.model.ConversionRequest import java.io.File +import java.util.UUID import java.util.concurrent.ExecutionException import java.util.concurrent.Executor import java.util.concurrent.TimeUnit @@ -94,3 +97,27 @@ internal class FailedFuture(private val failure: Throwable) : ListenableFuture`: the + * worker awaits what this returns, so a future that never completes would hang the initial + * `setForeground` rather than test anything. + * + * Shared scaffolding since #252 moved it here out of `ProgressNotificationTest`, which asks what a + * *running* worker publishes; `ForegroundNotificationTest` asks what its *first* post is, and both + * questions need the same recorder. `infos.first()` is that first post in either. + */ +internal class RecordingForegroundUpdater : TestForegroundUpdater() { + val infos = mutableListOf() + + override fun setForegroundAsync( + context: Context, + id: UUID, + foregroundInfo: ForegroundInfo, + ): ListenableFuture { + infos += foregroundInfo + return super.setForegroundAsync(context, id, foregroundInfo) + } +} diff --git a/docs/coverage-read-findings.md b/docs/coverage-read-findings.md index eecf91a..70e155b 100644 --- a/docs/coverage-read-findings.md +++ b/docs/coverage-read-findings.md @@ -403,6 +403,27 @@ They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWo named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls `setExpedited`. +**Updated 2026-09-06 (#252, and the sentence above is half wrong).** "WorkManager calls +`getForegroundInfoAsync()` only for expedited work" is true and *not sufficient*, and the missing +half is what made the reopening trigger wrong. `WorkForeground.kt:38` in work-runtime 2.11.2 opens +the library's only caller with + +```kotlin +if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return +``` + +and `minSdk` is 33. So calling `setExpedited` reopens nothing: on **every** device this app +supports, WorkManager does not consult `getForegroundInfo()` whether the work is expedited or not. +#252 was filed on the trigger as this entry stated it, and its acceptance criterion — "a request +now carries `setExpedited` and the existing worker tests drive them" — cannot be met that way. + +What made the lines live instead was that each worker held **two** definitions of one notification: +the override, and an identical `ForegroundInfo` built inline in `doWork`. `doWork` now posts the +override's, so the duplicate is gone and what remains runs on every job. The general lesson is the +one E1 states from the other side: *check that the mechanism you are relying on actually fires on +the machine that runs it* — here the mechanism was a library early-return two source lines long, +and four waves of reading had taken the API summary's word for it. + --- ## F10 — Three arms that are reachable, uncovered, and cannot be made to bite @@ -452,7 +473,7 @@ the cheaper order. | F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix | | F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites | | F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap | -| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close | +| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **closed 2026-09-06 by #252** — and its stated reopening trigger was wrong; see the update on the entry | | F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them | Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible diff --git a/docs/e2e-read-findings.md b/docs/e2e-read-findings.md index e480025..9ca3eb3 100644 --- a/docs/e2e-read-findings.md +++ b/docs/e2e-read-findings.md @@ -495,7 +495,7 @@ Every one was read. **None of them is an e2e test gap**, which is the result: | lines | where | classification | |---|---|---| | 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` | -| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252** | +| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252 — closed 2026-09-06, and not the way this row expects.** Expedited work is now enqueued, but that is *not* what covers these lines: `WorkForeground.kt:38` returns before the hook whenever `SDK_INT >= 31`, and `minSdk` is 33. What covers them is `doWork` posting the override instead of a second copy of the same notification. See `coverage-read-findings.md` F9's update | | 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record | | 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 | | 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |