From d59e9acce5e0d12987f923d8e3f110f7870b2415 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:42:46 -0500 Subject: [PATCH] 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) --- .../convert/OutputPublisherPublishTest.kt | 49 +++++++++++++++++++ .../convert/OutputPublisherStagingTest.kt | 26 ++++++++++ 2 files changed, 75 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index c670a67..aa7fd24 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -20,6 +20,9 @@ import java.io.OutputStream /** What a destination volume says when it fills up mid-write. */ 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. * @@ -75,6 +78,8 @@ class OutputPublisherPublishTest { 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 plainUri: Uri = Uri.parse("content://$PLAIN_AUTHORITY/document/holiday_plain.mp4") private val deadUri: Uri = Uri.parse("content://org.libremediaconverter.nonexistent/document/gone.mp4") @@ -203,6 +208,50 @@ class OutputPublisherPublishTest { assertEquals(emptyList(), 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(), + FakeSafProvider.deleteRequests, + ) + assertTrue( + "$description must leave the destination where it was", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + } + @Test fun `a copy that succeeds delivers every byte and deletes nothing`() { shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index f778f5a..a1d2365 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -1,6 +1,7 @@ package org.libremediaconverter.convert import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -99,4 +100,29 @@ class OutputPublisherStagingTest { 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. + File(cacheDir, "conversions").deleteRecursively() + File(cacheDir, "conversions").writeBytes(ByteArray(8)) + + publisher.sweepStaging() + } + + @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)) + } }