Testing slideshow deletion for remote files

PhilLab · PhilLab · commit 8ab58331ea90 · 2025-12-28T13:29:52.000+01:00
The remote + offline test case had to be ignored due to two reasons
1) Broken App behavior
- The UX is indeed broken, as from a user perspective, nothing happens
  with the file when deleting it. The offlineOperation is put on the worker
  stack, but the user doesn't see anything from it
- Even when coming back online, it is completely unreliable when the
  deletion will be finally done. It might happen 5 or 10 minutes later
2) Broken test mock
- The mocked connectivityService doesn't work as expected, because the
  OfflineOperationsWorker has its own service, and thus might still
  execute the deletion, but just at an unforseable time during the test
  execution - see problem 1).

Signed-off-by: Philipp Hasper &lt;vcs@hasper.info&gt;
diff --git a/app/src/androidTest/java/com/nextcloud/test/FileRemovedIdlingResource.kt b/app/src/androidTest/java/com/nextcloud/test/FileRemovedIdlingResource.kt
@@ -0,0 +1,51 @@
+/*
+ * Nextcloud - Android Client
+ *
+ * SPDX-FileCopyrightText: 2025 Philipp Hasper <vcs@hasper.info>
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+package com.nextcloud.test
+
+import androidx.test.espresso.IdlingResource
+import com.owncloud.android.datamodel.FileDataStorageManager
+import com.owncloud.android.datamodel.OCFile
+import java.util.concurrent.atomic.AtomicLong
+import java.util.concurrent.atomic.AtomicReference
+
+/**
+ * IdlingResource that can be reused to watch the removal of different file ids sequentially.
+ *
+ * Use setFileId(fileId) before triggering the deletion. The resource will call the Espresso callback
+ * once the file no longer exists. Call unregister from IdlingRegistry in @After.
+ */
+class FileRemovedIdlingResource(private val storageManager: FileDataStorageManager) : IdlingResource {
+    private var resourceCallback: IdlingResource.ResourceCallback? = null
+
+    // null means "no file set"
+    private var currentFile = AtomicReference<OCFile>(null)
+
+    override fun getName(): String = "${this::class.java.simpleName}"
+
+    override fun isIdleNow(): Boolean {
+        val file = currentFile.get()
+        // If no file set, consider idle. If file set, idle only if it doesn't exist.
+        val idle = file == null || (!storageManager.fileExists(file.fileId) && !file.exists())
+        if (idle && file != null) {
+            // if we detect it's already removed, notify and clear
+            resourceCallback?.onTransitionToIdle()
+            currentFile.set(null)
+        }
+        return idle
+    }
+
+    override fun registerIdleTransitionCallback(callback: IdlingResource.ResourceCallback) {
+        this.resourceCallback = callback
+    }
+
+    /**
+     * Start watching the given file. Call this right before performing the UI action that triggers deletion.
+     */
+    fun setFile(file: OCFile) {
+        currentFile.set(file)
+    }
+}
diff --git a/app/src/androidTest/java/com/owncloud/android/ui/preview/PreviewImageActivityIT.kt b/app/src/androidTest/java/com/owncloud/android/ui/preview/PreviewImageActivityIT.kt
@@ -22,20 +22,34 @@ import androidx.test.espresso.matcher.ViewMatchers.isRoot
 import androidx.test.espresso.matcher.ViewMatchers.withId
 import androidx.test.espresso.matcher.ViewMatchers.withText
 import com.nextcloud.test.ConnectivityServiceOfflineMock
+import com.nextcloud.test.FileRemovedIdlingResource
 import com.nextcloud.test.LoopFailureHandler
 import com.owncloud.android.AbstractOnServerIT
 import com.owncloud.android.R
 import com.owncloud.android.datamodel.OCFile
+import com.owncloud.android.db.OCUpload
+import com.owncloud.android.files.services.NameCollisionPolicy
+import com.owncloud.android.lib.resources.files.ExistenceCheckRemoteOperation
 import org.hamcrest.Matchers.allOf
+import org.junit.After
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Ignore
 import org.junit.Test
 import java.io.File
 
 class PreviewImageActivityIT : AbstractOnServerIT() {
-    lateinit var testFiles: List<OCFile>
+    companion object {
+        private const val REMOTE_FOLDER: String = "/PreviewImageActivityIT/"
+    }
+
+    var fileRemovedIdlingResource = FileRemovedIdlingResource(storageManager)
 
-    fun createMockedImageFiles(count: Int, localOnly: Boolean) {
+    @Suppress("SameParameterValue")
+    private fun createLocalMockedImageFiles(count: Int): List<OCFile> {
         val srcPngFile = getFile("imageFile.png")
-        testFiles = (0 until count).map { i ->
+        return (0 until count).map { i ->
             val pngFile = File(srcPngFile.parent ?: ".", "image$i.png")
             srcPngFile.copyTo(pngFile, overwrite = true)
 
@@ -44,27 +58,56 @@ class PreviewImageActivityIT : AbstractOnServerIT() {
                 mimeType = "image/png"
                 modificationTimestamp = 1000000
                 permissions = "D" // OCFile.PERMISSION_CAN_DELETE_OR_LEAVE_SHARE. Required for deletion button to show
-                remoteId = if (localOnly) null else "abc-mocked-remote-id" // mocking the file to be on the server
             }.also {
                 storageManager.saveNewFile(it)
             }
         }
     }
 
-    fun veryImageThenDelete(index: Int) {
-        val currentFileName = testFiles[index].fileName
+    /**
+     * Create image files and upload them to the connected server.
+     *
+     * This function relies on the images not existing beforehand, as AbstractOnServerIT#deleteAllFilesOnServer()
+     * should clean up. If it does fail, likely because that clean up didn't work and there are leftovers from
+     * a previous run
+     * @param count Number of files to create
+     * @param folder Parent folder to which to upload. Must start and end with a slash
+     */
+    private fun createAndUploadImageFiles(count: Int, folder: String = REMOTE_FOLDER): List<OCFile> {
+        val srcPngFile = getFile("imageFile.png")
+        return (0 until count).map { i ->
+            val pngFile = File(srcPngFile.parent ?: ".", "image$i.png")
+            srcPngFile.copyTo(pngFile, overwrite = true)
+
+            val ocUpload = OCUpload(
+                pngFile.absolutePath,
+                folder + pngFile.name,
+                account.name
+            ).apply {
+                nameCollisionPolicy = NameCollisionPolicy.OVERWRITE
+            }
+            uploadOCUpload(ocUpload)
+
+            fileDataStorageManager.getFileByDecryptedRemotePath(folder + pngFile.name)!!
+        }
+    }
+
+    private fun veryImageThenDelete(testFile: OCFile) {
         Espresso.setFailureHandler(
-            LoopFailureHandler(targetContext, "Test failed with image file index $index, $currentFileName")
+            LoopFailureHandler(targetContext, "Test failed with image file ${testFile.fileName}")
         )
 
+        assertTrue(testFile.exists())
+        assertTrue(testFile.fileExists())
+
         onView(withId(R.id.image))
             .check(matches(isDisplayed()))
 
         // Check that the Action Bar shows the file name as title
         onView(
             allOf(
                 isDescendantOfA(isAssignableFrom(ActionBarContainer::class.java)),
-                withText(currentFileName)
+                withText(testFile.fileName)
             )
         ).check(matches(isDisplayed()))
 
@@ -82,7 +125,7 @@ class PreviewImageActivityIT : AbstractOnServerIT() {
         onView(withText(R.string.common_remove)).perform(ViewActions.click())
 
         // Check confirmation dialog and then confirm the deletion by clicking the main button of the dialog
-        val expectedText = targetContext.getString(R.string.confirmation_remove_file_alert, currentFileName)
+        val expectedText = targetContext.getString(R.string.confirmation_remove_file_alert, testFile.fileName)
         onView(withId(android.R.id.message))
             .inRoot(isDialog())
             .check(matches(withText(expectedText)))
@@ -92,43 +135,80 @@ class PreviewImageActivityIT : AbstractOnServerIT() {
             .check(matches(withText(R.string.file_delete)))
             .perform(ViewActions.click())
 
+        // Register the idling resource to wait for successful deletion
+        fileRemovedIdlingResource.setFile(testFile)
+
+        // Wait for idle, then verify that the file is gone. Somehow waitForIdleSync() doesn't work and we need onIdle()
+        Espresso.onIdle()
+        assertFalse("test file still exists: ${testFile.fileName}", testFile.exists())
+
         Espresso.setFailureHandler(DefaultFailureHandler(targetContext))
     }
 
-    @Test
-    fun deleteFromSlideshow_localOnly_online() {
+    @Before
+    fun bringUp() {
+        IdlingRegistry.getInstance().register(fileRemovedIdlingResource)
+    }
+
+    @After
+    fun tearDown() {
+        IdlingRegistry.getInstance().unregister(fileRemovedIdlingResource)
+    }
+
+    private fun testDeleteFromSlideshow_impl(localOnly: Boolean, offline: Boolean) {
         // Prepare local test data
         val imageCount = 5
-        createMockedImageFiles(imageCount, localOnly = true)
+        val testFiles = if (localOnly) {
+            createLocalMockedImageFiles(
+                imageCount
+            )
+        } else {
+            createAndUploadImageFiles(imageCount)
+        }
 
         // Launch the activity with the first image
         val intent = PreviewImageActivity.previewFileIntent(targetContext, user, testFiles[0])
-        launchActivity<PreviewImageActivity>(intent).use {
+        launchActivity<PreviewImageActivity>(intent).use { scenario ->
+            if (offline) {
+                scenario.onActivity { activity ->
+                    activity.connectivityService = ConnectivityServiceOfflineMock()
+                }
+            }
             onView(isRoot()).check(matches(isDisplayed()))
 
-            for (i in 0 until imageCount) {
-                veryImageThenDelete(i)
+            for (testFile in testFiles) {
+                veryImageThenDelete(testFile)
+                assertTrue(
+                    "Test file still exists on the server: ${testFile.remotePath}",
+                    ExistenceCheckRemoteOperation(testFile.remotePath, true).execute(client).isSuccess
+                )
             }
         }
     }
 
+    @Test
+    fun deleteFromSlideshow_localOnly_online() {
+        testDeleteFromSlideshow_impl(localOnly = true, offline = false)
+    }
+
     @Test
     fun deleteFromSlideshow_localOnly_offline() {
-        // Prepare local test data
-        val imageCount = 5
-        createMockedImageFiles(imageCount, localOnly = true)
+        testDeleteFromSlideshow_impl(localOnly = true, offline = true)
+    }
 
-        // Launch the activity with the first image
-        val intent = PreviewImageActivity.previewFileIntent(targetContext, user, testFiles[0])
-        launchActivity<PreviewImageActivity>(intent).use { scenario ->
-            scenario.onActivity { activity ->
-                activity.connectivityService = ConnectivityServiceOfflineMock()
-            }
-            onView(isRoot()).check(matches(isDisplayed()))
+    @Test
+    fun deleteFromSlideshow_remote_online() {
+        testDeleteFromSlideshow_impl(localOnly = false, offline = false)
+    }
 
-            for (i in 0 until imageCount) {
-                veryImageThenDelete(i)
-            }
-        }
+    @Test
+    @Ignore(
+        "Offline deletion is following a different UX and it is also brittle: Deletion might happen 10 minutes later"
+    )
+    fun deleteFromSlideshow_remote_offline() {
+        // Note: the offline mock doesn't actually do what it is supposed to. The OfflineOperationsWorker uses its
+        // own connectivityService, which is online, and may still execute the server deletion.
+        // You'll need to address this, should you activate that test. Otherwise it might not catch all error cases
+        testDeleteFromSlideshow_impl(localOnly = false, offline = true)
     }
 }
