diff --git a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt index fd1c93f..5b01383 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -78,4 +78,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 = 5 +const val FAILS_ON_EMULATOR_API37_BASELINE = 6 diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java index c5d7db9..3a082cd 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java +++ b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureDocumentsProvider.java @@ -14,6 +14,8 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.util.ArrayList; +import java.util.List; /** * One file, offered to the system file picker, so that picking one can be tested at all. @@ -104,6 +106,18 @@ public final class FixtureDocumentsProvider extends DocumentsProvider { private static final String ROOT_DOCUMENT_ID = "root"; private static final String FIXTURE_DOCUMENT_ID = "root/" + FIXTURE_DISPLAY_NAME; + /** + * Prefix for documents this provider CREATES, as opposed to the one it serves for reading. + * + *

Two namespaces rather than one so a destination can never be confused with the fixture. + * The fixture is read-only and must stay that way for the picker tests; a destination is + * writable and deletable, which is what {@code PublishToRealSafDestinationTest} needs. + */ + public static final String DESTINATION_PREFIX = "dest/"; + + /** Document ids {@link #deleteDocument} was called with, newest last. Cleared by {@link #reset}. */ + private static final List DELETED = new ArrayList<>(); + /** Already in this source set, and already a real H.264 MP4 the engines can open. */ private static final String FIXTURE_ASSET = "sample_h264.mp4"; @@ -147,7 +161,7 @@ public final class FixtureDocumentsProvider extends DocumentsProvider { .add(Root.COLUMN_TITLE, ROOT_TITLE) .add(Root.COLUMN_SUMMARY, "Instrumentation fixture") .add(Root.COLUMN_MIME_TYPES, FIXTURE_MIME_TYPE) - .add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY) + .add(Root.COLUMN_FLAGS, Root.FLAG_LOCAL_ONLY | Root.FLAG_SUPPORTS_CREATE) .add(Root.COLUMN_ICON, android.R.drawable.ic_menu_gallery); return cursor; } @@ -159,6 +173,8 @@ public final class FixtureDocumentsProvider extends DocumentsProvider { addDirectoryRow(cursor); } else if (FIXTURE_DOCUMENT_ID.equals(documentId)) { addFixtureRow(cursor); + } else if (documentId != null && documentId.startsWith(DESTINATION_PREFIX)) { + addDestinationRow(cursor, documentId); } else { throw new FileNotFoundException("no such document: " + documentId); } @@ -178,10 +194,76 @@ public final class FixtureDocumentsProvider extends DocumentsProvider { @Override public ParcelFileDescriptor openDocument(String documentId, String mode, CancellationSignal signal) throws FileNotFoundException { - if (!FIXTURE_DOCUMENT_ID.equals(documentId)) { + if (FIXTURE_DOCUMENT_ID.equals(documentId)) { + return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY); + } + if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) { throw new FileNotFoundException("no such document: " + documentId); } - return ParcelFileDescriptor.open(fixtureFile(), ParcelFileDescriptor.MODE_READ_ONLY); + int flags = "r".equals(mode) + ? ParcelFileDescriptor.MODE_READ_ONLY + : ParcelFileDescriptor.MODE_READ_WRITE | ParcelFileDescriptor.MODE_TRUNCATE; + return ParcelFileDescriptor.open(destinationFile(documentId), flags); + } + + /** + * Creates a real, empty file and reports the document id for it. + * + *

Empty is the whole point, and this provider does not get to decide it. The premise + * under test in {@code PublishToRealSafDestinationTest} is what DocumentsUI hands back + * from {@code ACTION_CREATE_DOCUMENT}, and {@code OutputPublisher.destinationIsKnownEmpty} + * authorises its cleanup delete only on a positive zero. This creates the file and writes + * nothing to it, which is what the SAF contract documents; the test asserts what actually came + * back rather than trusting either side. + */ + @Override + public String createDocument(String parentDocumentId, String mimeType, String displayName) + throws FileNotFoundException { + if (!ROOT_DOCUMENT_ID.equals(parentDocumentId)) { + throw new FileNotFoundException("cannot create in: " + parentDocumentId); + } + String documentId = DESTINATION_PREFIX + displayName; + File file = destinationFile(documentId); + try { + if (!file.createNewFile() && !file.exists()) { + throw new FileNotFoundException("could not create: " + documentId); + } + } catch (IOException e) { + throw new FileNotFoundException("could not create " + documentId + ": " + e); + } + return documentId; + } + + @Override + public void deleteDocument(String documentId) throws FileNotFoundException { + if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) { + throw new FileNotFoundException("refusing to delete: " + documentId); + } + synchronized (DELETED) { + DELETED.add(documentId); + } + destinationFile(documentId).delete(); + } + + /** Document ids {@link #deleteDocument} was called with, newest last. */ + public static List deletedDocumentIds() { + synchronized (DELETED) { + return new ArrayList<>(DELETED); + } + } + + /** Forgets recorded deletes and removes created destinations. The process outlives one class. */ + public static void reset(File filesDir) { + synchronized (DELETED) { + DELETED.clear(); + } + File dir = new File(filesDir, "destinations"); + File[] children = dir.listFiles(); + if (children != null) { + for (File child : children) { + child.delete(); + } + } } private void addDirectoryRow(MatrixCursor cursor) { @@ -189,10 +271,32 @@ public final class FixtureDocumentsProvider extends DocumentsProvider { .add(Document.COLUMN_DOCUMENT_ID, ROOT_DOCUMENT_ID) .add(Document.COLUMN_DISPLAY_NAME, ROOT_TITLE) .add(Document.COLUMN_MIME_TYPE, Document.MIME_TYPE_DIR) - .add(Document.COLUMN_FLAGS, 0) + .add(Document.COLUMN_FLAGS, Document.FLAG_DIR_SUPPORTS_CREATE) .add(Document.COLUMN_SIZE, null); } + private void addDestinationRow(MatrixCursor cursor, String documentId) throws FileNotFoundException { + File file = destinationFile(documentId); + if (!file.exists()) { + throw new FileNotFoundException("no such document: " + documentId); + } + cursor.newRow() + .add(Document.COLUMN_DOCUMENT_ID, documentId) + .add(Document.COLUMN_DISPLAY_NAME, documentId.substring(DESTINATION_PREFIX.length())) + .add(Document.COLUMN_MIME_TYPE, FIXTURE_MIME_TYPE) + .add(Document.COLUMN_FLAGS, Document.FLAG_SUPPORTS_DELETE | Document.FLAG_SUPPORTS_WRITE) + .add(Document.COLUMN_SIZE, file.length()) + .add(Document.COLUMN_LAST_MODIFIED, file.lastModified()); + } + + private File destinationFile(String documentId) throws FileNotFoundException { + File dir = new File(getContext().getFilesDir(), "destinations"); + if (!dir.isDirectory() && !dir.mkdirs()) { + throw new FileNotFoundException("could not make the destinations directory"); + } + return new File(dir, documentId.substring(DESTINATION_PREFIX.length())); + } + private void addFixtureRow(MatrixCursor cursor) throws FileNotFoundException { File file = fixtureFile(); cursor.newRow() diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt index c82e2ce..aacd7b5 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -1,12 +1,18 @@ package org.libremediaconverter.saf import android.app.UiAutomation +import android.content.Context +import android.net.Uri +import android.provider.DocumentsContract +import android.provider.OpenableColumns import androidx.compose.ui.test.ComposeTimeoutException +import androidx.compose.ui.test.assertIsEnabled import androidx.compose.ui.test.assertTextEquals import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule import androidx.compose.ui.test.onAllNodesWithTag import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo import androidx.media3.common.util.UnstableApi import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -20,14 +26,22 @@ import androidx.test.uiautomator.StaleObjectException import androidx.test.uiautomator.UiDevice import androidx.test.uiautomator.Until import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremediaconverter.FailsOnEmulatorApi37 import org.libremediaconverter.MainActivity +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.ui.TestTags +import java.io.File import java.util.concurrent.atomic.AtomicInteger +import java.util.regex.Pattern /** * Choosing a file, through the real system picker, and still having it after a rotation. @@ -241,12 +255,72 @@ import java.util.concurrent.atomic.AtomicInteger * file".** That is what API 33 through 36 are for, and they answer it. */ @UnstableApi +/** + * Reads what SAF handed back, then publishes for real. + * + * The premise `OutputPublisher.destinationIsKnownEmpty` depends on has only ever been asserted + * against a fake built to match it — `OutputPublisherPublishTest` writes `ByteArray(0)` into + * `FakeSafProvider` before each case, under a comment stating this is how `CreateDocument` behaves. + * This records what stock DocumentsUI actually produced, at the moment `publish` sees it and before + * a byte is written, and then lets the real copy proceed. See #226. + */ +private class RecordingPublisher(private val app: Context) : OutputPublisher(app) { + + override fun publish(staged: File, destination: Uri) { + seenDestination = destination + seenIsDocumentUri = DocumentsContract.isDocumentUri(app, destination) + seenSizeBefore = app.contentResolver + .query(destination, arrayOf(OpenableColumns.SIZE), null, null, null) + ?.use { row -> + val column = row.getColumnIndex(OpenableColumns.SIZE) + if (column >= 0 && row.moveToFirst() && !row.isNull(column)) row.getLong(column) else null + } + // Read before the copy: the ViewModel deletes the staged file once publish returns. + savedBytes = staged.readBytes() + super.publish(staged, destination) + } + + companion object { + var savedBytes: ByteArray = ByteArray(0) + var seenDestination: Uri? = null + var seenIsDocumentUri: Boolean? = null + var seenSizeBefore: Long? = null + + fun reset() { + savedBytes = ByteArray(0) + seenDestination = null + seenIsDocumentUri = null + seenSizeBefore = null + } + } +} + @RunWith(AndroidJUnit4::class) class SafPickerRoundTripTest { + /** + * Installs [RecordingPublisher] before the Activity exists. + * + * `ConversionViewModel` resolves its publisher through `ConversionDependencies` **at + * construction**, and the Compose rule launches `MainActivity` as part of the rule chain — + * which wraps `@Before`, so `@Before` is already too late. JUnit constructs the test instance + * before it evaluates the rules, so an initialiser is early enough, and it needs no + * `@BeforeClass` (this class's companion is private, and JUnit wants a public static there). + * + * Harmless for the other two tests: neither saves, so `publish` is never called and the + * subclass behaves exactly like `OutputPublisher`. `restoreOrientation` puts the seam back. + */ + init { + RecordingPublisher.reset() + ConversionDependencies.publisher = { RecordingPublisher(it) } + } + @get:Rule val composeRule = createAndroidComposeRule() + private val context: Context = + InstrumentationRegistry.getInstrumentation().targetContext + private val device: UiDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation()) @@ -291,6 +365,8 @@ class SafPickerRoundTripTest { */ @After fun restoreOrientation() { + // The suite runs without Android Test Orchestrator, so a swapped seam outlives the class. + ConversionDependencies.reset() ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher) if (!rotated) return device.setOrientationNatural() @@ -406,6 +482,173 @@ class SafPickerRoundTripTest { * are all warm and the only thing being waited on is one screen. That is what keeps the cost * of a genuinely absent root bounded — see the class KDoc. */ + /** + * The save side of SAF, end to end, against a document stock DocumentsUI created (#226). + * + * ## What this settles + * + * `publish` deletes a destination it could not write to — `docs/defect-audit.md` **D4**'s fix, + * so a failed save does not leave a truncated file at the name the user chose — but only when + * that destination was **positively zero bytes** first. `destinationIsKnownEmpty` is careful + * that "I could not tell" never authorises a delete, which is right, and which makes the + * precondition load-bearing. + * + * Until now that precondition was asserted only against a fake built to match it: + * `OutputPublisherPublishTest` writes `ByteArray(0)` into `FakeSafProvider` before each case, + * under a comment stating this is how `CreateDocument` behaves. **If it is false in production, + * D4's fix is inert and every existing test still passes.** [RecordingPublisher] reads what SAF + * actually handed over, at the moment `publish` sees it and before a byte is written. + * + * ## Why it has to go through the app, and through the picker + * + * Through the **picker** because a `DocumentsProvider` cannot be reached any other way — + * measured three ways and recorded as **E7** in `docs/e2e-read-findings.md`: an unprotected one + * is refused at install, instrumentation carries the app's uid so the test APK's own identity + * is no help, and shell identity is denied too, each denial naming `ACTION_OPEN_DOCUMENT`. + * + * Through the **app** because the same constraint sinks the obvious alternative. A host + * Activity in this source set that owns a `CreateDocument` launcher cannot be started: + * `ActivityScenario` refuses with *"Intent in process org.libremediaconverter resolved to + * different process org.libremediaconverter.test"*. Instrumentation runs in the target app's + * process, so the only Activity available to drive is the app's own — which is also the more + * faithful thing to drive. + * + * ## The conversion is setup, not subject + * + * Save is only offered on `Converted`, so the test converts first. MP3 is chosen because the + * router sends it to FFmpeg unconditionally at every API level, so the setup cannot depend on + * the device's codecs — #223 is what that costs. + */ + @Test + @FailsOnEmulatorApi37 + fun aSaveWritesToTheDocumentTheSystemPickerCreated() { + pickTheFixture() + convertToTheDefaultFormat() + + saveThroughTheSystemPicker() + + val destination = RecordingPublisher.seenDestination + assertNotNull("publish was never reached, so nothing was saved", destination) + assertTrue( + "SAF handed back something that is not a document URI, so publish's cleanup can " + + "never run and D4's fix is inert: $destination", + RecordingPublisher.seenIsDocumentUri == true, + ) + assertEquals( + "SAF handed back a document that is not positively empty, so " + + "destinationIsKnownEmpty answers false and a failed save keeps its partial file", + 0L, + RecordingPublisher.seenSizeBefore, + ) + + // And the bytes really arrived, which only the failure side was covered for on a device. + val staged = File(context.cacheDir, "conversions") + assertArrayEquals( + "the destination did not receive what was staged", + RecordingPublisher.savedBytes, + context.contentResolver.openInputStream(destination!!)!!.use { it.readBytes() }, + ) + assertTrue("staging should be empty after a successful save", staged.listFiles().isNullOrEmpty()) + } + + /** + * Runs the conversion, leaving the screen on `Converted`. + * + * **The format is left at its default, and that is a constraint rather than laziness.** + * `ConverterScreen` registers `CreateDocument` with the *output's* MIME type, and + * [FixtureDocumentsProvider] advertises `Root.COLUMN_MIME_TYPES` of `video/mp4` — deliberately, + * so the picker's MIME filter has a mutation with a shape. DocumentsUI honours that on the save + * side too: choosing MP3 makes the destination type `audio/mpeg`, and the fixture root is then + * filtered out of the save dialog entirely. Measured, as *"the create-document dialog never + * showed LMC R38 fixtures"*. The default `MP4_H265` produces `video/mp4` and the root is + * offered. + * + * **The notification dialog is dismissed rather than pre-granted, and that is the honest + * version.** Convert never calls `convert()` directly — it launches `RequestPermission` for + * `POST_NOTIFICATIONS` and converts from the callback **whichever way the answer goes**. So the + * dialog only has to be got out of the way; denying it is a real user's path and the conversion + * still runs. Granting it programmatically was tried first and did not take — + * `GrantPermissionsActivity` appeared anyway, the click that followed went to it rather than to + * the app, and the screen sat in `Ready` with nothing enqueued. + * + * **Both taps scroll first.** On `Ready` the screen carries a file card, five pickers and then + * the button, so Convert is below the fold on a phone. `performClick` on an off-screen node + * dispatches at a position that hits nothing and throws nothing, and `assertIsEnabled` passes + * either way — the first version of this sat waiting for a `Converted` that could never come. + */ + private fun convertToTheDefaultFormat() { + composeRule.onNodeWithTag(TestTags.Converter.CONVERT) + .performScrollTo() + .assertIsEnabled() + .performClick() + + dismissThePermissionDialog() + awaitNode(TestTags.SAVE_FILE, CONVERSION_TIMEOUT_MS) + } + + /** + * Gets the `POST_NOTIFICATIONS` dialog out of the way, if this device shows one. + * + * Backing out of it is a denial, and a denial is fine here: the conversion starts either way, + * and what that costs the user is a progress notification confined to the Task Manager. Waiting + * only briefly, because on a device where the permission is already held no dialog appears at + * all and the conversion is already under way. + */ + private fun dismissThePermissionDialog() { + if (device.wait(Until.hasObject(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) != true) { + return + } + device.pressBack() + device.wait(Until.gone(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) + // And wait for the app to be in front again before anything asks Compose about it. + // Querying while another window still owns the screen raises "No compose hierarchies found + // in the app", which is what this test did on an API 35 leg: the back press had landed but + // the dialog had not finished going away. + // + // Asked of UiAutomator rather than through awaitAppFocus, which is the opposite of what the + // class KDoc argues for elsewhere and is right here: awaitAppFocus goes through + // composeRule.waitUntil, so it would raise the very error it is being used to avoid. + device.wait(Until.hasObject(By.pkg(context.packageName)), FOCUS_TIMEOUT_MS) + } + + /** + * Taps Save and drives the create-document dialog into the fixture root. + * + * Retried whole, for the reason [pickTheFixture] documents: a dialog that came up unreadable + * cannot be recovered from inside, and a fresh one is the only answer. + */ + private fun saveThroughTheSystemPicker() { + var missing: BySelector? = null + repeat(PICK_ATTEMPTS) { attempt -> + requireAReadableScreen() + composeRule.onNodeWithTag(TestTags.SAVE_FILE).performClick() + missing = walkTheSaveDialog( + if (attempt == 0) PICKER_TIMEOUT_MS else REOPENED_TIMEOUT_MS, + ) + if (missing == null) { + awaitNode(TestTags.Converter.CONVERT_ANOTHER, SAVE_TIMEOUT_MS) + return + } + dismissThePicker() + } + throw AssertionError( + "the create-document dialog never showed $missing, in $PICK_ATTEMPTS separate " + + "dialogs (the last one left ${device.currentPackageName} in front)", + ) + } + + /** Into the fixture root, then Save. Returns the selector never found, or null. */ + private fun walkTheSaveDialog(timeoutMs: Long): BySelector? { + val picker = By.pkg(DOCUMENTS_UI_PACKAGE) + val root = By.text(FixtureDocumentsProvider.ROOT_TITLE) + return when { + device.wait(Until.hasObject(picker), timeoutMs) != true -> picker + !tapPickerNode(root, timeoutMs, ifAbsent = ::openTheRootsDrawer) -> root + !tapPickerNode(SAVE_BUTTON, timeoutMs) -> SAVE_BUTTON + else -> null + } + } + private fun pickTheFixture() { var missing: BySelector? = null repeat(PICK_ATTEMPTS) { attempt -> @@ -795,8 +1038,8 @@ class SafPickerRoundTripTest { } } - private fun awaitNode(tag: String) { - composeRule.waitUntil("a node tagged $tag exists", APP_TIMEOUT_MS) { + private fun awaitNode(tag: String, timeoutMs: Long = APP_TIMEOUT_MS) { + composeRule.waitUntil("a node tagged $tag exists", timeoutMs) { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty() } } @@ -811,6 +1054,24 @@ class SafPickerRoundTripTest { const val PICKER_TIMEOUT_MS = 30_000L const val APP_TIMEOUT_MS = 30_000L + /** The runtime-permission dialog's package, so it can be recognised and dismissed. */ + const val PERMISSION_UI_PACKAGE = "com.google.android.permissioncontroller" + + /** Short: either the dialog is up almost immediately, or the permission was already held. */ + const val PERMISSION_DIALOG_MS = 5_000L + + /** A 3 s clip to MP3 on an emulator is about a second; this only bounds a hang. */ + const val CONVERSION_TIMEOUT_MS = 120_000L + + /** The copy is a few kilobytes, but it crosses a provider. */ + const val SAVE_TIMEOUT_MS = 30_000L + + /** + * DocumentsUI's save button. Case-insensitive because the label is "SAVE" on some images + * and "Save" on others, and the difference is not what this test is about. + */ + val SAVE_BUTTON: BySelector = By.text(Pattern.compile("save", Pattern.CASE_INSENSITIVE)) + /** * The same wait once a picker has already come and gone, and shorter for a reason. *