Merge pull request #7831 from element-hq/feature/bma/fixScreenshotFlakiness

Stop rewriting screenshots that did not meaningfully change

.github/workflows/scripts/recordScreenshots.sh

@@ -51,9 +51,10 @@ if [[ -z ${REPO} ]]; then
   exit 1
 fi
 
-echo "Deleting previous screenshots"
-./gradlew removeOldSnapshots --stacktrace --warn $GRADLE_ARGS
-
+# Note: the previous screenshots are deliberately kept, so that Paparazzi can leave the ones that only
+# differ by less than `maxPercentDifference` untouched instead of rewriting their Git LFS blob. The
+# screenshots of previews that no longer exist are pruned afterwards by `pruneObsoleteSnapshots`, which
+# `recordPaparazziDebug` is finalized by.
 echo "Record screenshots"
 ./gradlew recordPaparazziDebug --stacktrace $GRADLE_ARGS
 

build.gradle.kts

@@ -211,19 +211,93 @@ tasks.register("generateDocsToc", Exec::class.java) {
     commandLine("python3", "tools/docs/generate_toc.py", *inputs.files.map { it.path }.toTypedArray())
 }
 
-// Make sure to delete old screenshots before recording new ones
+// Delete all the screenshots. Not hooked to the record tasks on purpose, see `pruneObsoleteSnapshots`
+// below. Run it by hand (or use Paparazzi's own `cleanRecordPaparazziDebug`) to record from scratch.
 subprojects {
     val snapshotsDir = File("${project.projectDir}/src/test/snapshots")
-    val removeOldScreenshotsTask = tasks.register("removeOldSnapshots") {
+    tasks.register("removeOldSnapshots") {
         onlyIf { snapshotsDir.exists() }
         doFirst {
             println("Delete previous screenshots located at $snapshotsDir\n")
             snapshotsDir.deleteRecursively()
         }
     }
-    tasks.findByName("recordPaparazzi")?.dependsOn(removeOldScreenshotsTask)
-    tasks.findByName("recordPaparazziDebug")?.dependsOn(removeOldScreenshotsTask)
-    tasks.findByName("recordPaparazziRelease")?.dependsOn(removeOldScreenshotsTask)
+}
+
+// Delete the screenshots of previews that no longer exist, *after* recording.
+//
+// Wiping the whole snapshots directory before recording used to be how stale screenshots were removed,
+// but it also removes the reference image Paparazzi needs to honour
+// `app.cash.paparazzi.overwriteOnMaxPercentDifference` (see gradle.properties): with no existing file to
+// compare against, every screenshot is rewritten unconditionally, so renderer noise far below the
+// threshold `verifyPaparazziDebug` accepts still rewrites the Git LFS blob of every affected screenshot.
+//
+// Instead, let the record task keep the existing screenshots, then prune the ones belonging to a
+// preview that Paparazzi did not render at all.
+//
+// Pruning per preview, and not per screenshot, is deliberate: a preview that fails to render is absent
+// from the run report in exactly the same way as a preview that was deleted, and LayoutLib does fail to
+// render a preview now and then. Matching whole previews means such
+// a failure leaves the other screenshots of that preview in place, so it cannot silently delete a
+// screenshot that is still in use. The cost is that dropping one value from a PreviewParameterProvider
+// leaves its last screenshot behind, since the preview itself still renders. Run `removeOldSnapshots`
+// and record again to clean those up.
+subprojects {
+    val projectDir = project.projectDir
+    val pruneObsoleteSnapshots = tasks.register("pruneObsoleteSnapshots") {
+        description = "Delete the recorded screenshots of previews that no longer exist"
+        val imagesDir = File(projectDir, "src/test/snapshots/images")
+        val reportsDir = File(projectDir, "build/reports/paparazzi")
+        onlyIf { imagesDir.exists() }
+        doLast {
+            // One `runs/<runName>.js` per forked test JVM, each holding `window.runs[...] = [ ...snapshots... ];`.
+            val runFiles = reportsDir.walkTopDown()
+                .filter { it.isFile && it.extension == "js" && it.parentFile.name == "runs" }
+                .toList()
+            check(runFiles.isNotEmpty()) {
+                "No Paparazzi run report found under $reportsDir, so there is no way to tell which " +
+                    "screenshots are still in use. Refusing to delete anything."
+            }
+            // `testName` is serialised as "<package>.<class>#<method>" and the screenshot file is named
+            // "<package>_<class>_<method>.png". Only the package and the class are used here, they identify
+            // the preview. Do not be tempted to rebuild the whole file name from the method: it has to be
+            // normalised exactly as `Snapshot.toFileName` does, which replaces whitespace with the
+            // delimiter, so a preview named "List item - Simple" does not read as its own file name.
+            val testNameRegex = """"testName"\s*:\s*"(.*)\.([^.]*)#""".toRegex()
+            val renderedPreviews = runFiles.flatMap { runFile ->
+                testNameRegex.findAll(runFile.readText()).map { match ->
+                    val (packageName, className) = match.destructured
+                    // Keep the trailing delimiter, so that `Foo_` does not also match `FooBar_…`.
+                    "${packageName}_${className}_"
+                }
+            }.toSet()
+            check(renderedPreviews.isNotEmpty()) {
+                "Could not read any snapshot name out of the Paparazzi run reports in $reportsDir. " +
+                    "The report format has probably changed. Refusing to delete anything."
+            }
+
+            val existing = imagesDir.listFiles { file -> file.extension == "png" }.orEmpty()
+            val obsolete = existing.filter { file -> renderedPreviews.none { file.name.startsWith(it) } }
+            // Deleting a screenshot is a tracked file deletion, and a bad `renderedPreviews` set would wipe
+            // thousands of them. Anything beyond a trickle means something is wrong rather than a few
+            // deleted previews.
+            check(obsolete.size <= existing.size / 10) {
+                "pruneObsoleteSnapshots would delete ${obsolete.size} of ${existing.size} screenshots in " +
+                    "$imagesDir, which looks like a bug rather than deleted previews. Refusing to delete " +
+                    "anything. If the deletions are expected, run `removeOldSnapshots` and record again."
+            }
+            obsolete.forEach {
+                println("Delete obsolete screenshot ${it.name}")
+                it.delete()
+            }
+            println("Pruned ${obsolete.size} obsolete screenshot(s), kept ${existing.size - obsolete.size}.\n")
+        }
+    }
+    // `configureEach` on a live filtered collection, and not `findByName`, because the Paparazzi plugin
+    // registers its record tasks per variant, long after this block runs.
+    tasks.matching { it.name.startsWith("recordPaparazzi") }.configureEach {
+        finalizedBy(pruneObsoleteSnapshots)
+    }
 }
 
 subprojects {

gradle.properties

@@ -65,3 +65,9 @@ android.r8.gradual.support=true
 
 # Enabled parallel sync for Gradle 9.4+
 org.gradle.tooling.parallel=true
+
+# When re-recording, keep the existing screenshot if it differs by less than `maxPercentDifference`
+# (see PaparazziPreviewRule in tests/uitests/.../base/ScreenshotTest.kt). Without this, recording has
+# no tolerance at all while verification has one, so renderer noise far below the threshold that
+# `verifyPaparazziDebug` accepts still rewrites the Git LFS blob of every affected screenshot.
+app.cash.paparazzi.overwriteOnMaxPercentDifference=true

tests/uitests/src/test/kotlin/ui/LayoutLibErrorFilterStatement.kt

@@ -1,40 +0,0 @@
-/*
- * Copyright (c) 2026 Element Creations Ltd.
- *
- * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial.
- * Please see LICENSE files in the repository root for full details.
- */
-
-package ui
-
-import org.junit.rules.TestRule
-import org.junit.runner.Description
-import org.junit.runners.model.Statement
-
-/**
- * Workaround for Paparazzi 2.0.0-alpha05 / LayoutLib 16.2.1 bug where
- * HandlerThread_Delegate calls Thread.setPosixNicenessInternal which doesn't exist on JVM.
- * Tracked in https://github.com/cashapp/paparazzi/issues/2342 — fixed in layoutlib 16.2.3.
- * Remove this workaround once Paparazzi ships with layoutlib >= 16.2.3.
- *
- * Paparazzi's PaparazziLogger collects background thread errors and re-throws them via
- * assertNoErrors(). This Statement wrapper catches the known NoSuchMethodError so the test
- * can pass despite the LayoutLib bug.
- */
-class LayoutLibErrorFilterStatement : TestRule {
-    override fun apply(base: Statement, description: Description): Statement {
-        return object : Statement() {
-            @Throws(Throwable::class)
-            override fun evaluate() {
-                try {
-                    base.evaluate()
-                } catch (e: NoSuchMethodError) {
-                    if (e.message?.contains("setPosixNicenessInternal") != true) throw e
-                } catch (npe: NullPointerException) {
-                    // Also catch this error on `HorizontalFloatingToolbarPreview` and `HorizontalFloatingToolbarNoFabPreview`
-                    if (npe.message?.contains("""Cannot invoke "java.util.ArrayList.size()" because "childrenList" is null""") != true) throw npe
-                }
-            }
-        }
-    }
-}

tests/uitests/src/test/kotlin/ui/PreviewA11yTest.kt

@@ -29,10 +29,7 @@ class PreviewA11yTest(
     @TestParameter(valuesProvider = ComposableA11yPreviewProvider::class)
     val preview: ComposablePreview<AndroidPreviewInfo>,
 ) {
-    @get:Rule(order = 0)
-    val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement()
-
-    @get:Rule(order = 1)
+    @get:Rule
     val paparazziRule = PaparazziPreviewRule.createFor(
         preview = preview,
         locale = "en",

tests/uitests/src/test/kotlin/ui/PreviewShard1Test.kt

@@ -28,10 +28,7 @@ class PreviewShard1Test(
     @TestParameter(valuesProvider = Shard1ComposablePreviewProvider::class)
     val preview: ComposablePreview<AndroidPreviewInfo>,
 ) {
-    @get:Rule(order = 0)
-    val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement()
-
-    @get:Rule(order = 1)
+    @get:Rule
     val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en")
 
     @Test

tests/uitests/src/test/kotlin/ui/PreviewShard2Test.kt

@@ -28,10 +28,7 @@ class PreviewShard2Test(
     @TestParameter(valuesProvider = Shard2ComposablePreviewProvider::class)
     val preview: ComposablePreview<AndroidPreviewInfo>,
 ) {
-    @get:Rule(order = 0)
-    val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement()
-
-    @get:Rule(order = 1)
+    @get:Rule
     val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en")
 
     @Test

tests/uitests/src/test/kotlin/ui/PreviewShard3Test.kt

@@ -28,10 +28,7 @@ class PreviewShard3Test(
     @TestParameter(valuesProvider = Shard3ComposablePreviewProvider::class)
     val preview: ComposablePreview<AndroidPreviewInfo>,
 ) {
-    @get:Rule(order = 0)
-    val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement()
-
-    @get:Rule(order = 1)
+    @get:Rule
     val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en")
 
     @Test

tests/uitests/src/test/kotlin/ui/PreviewShard4Test.kt

@@ -28,10 +28,7 @@ class PreviewShard4Test(
     @TestParameter(valuesProvider = Shard4ComposablePreviewProvider::class)
     val preview: ComposablePreview<AndroidPreviewInfo>,
 ) {
-    @get:Rule(order = 0)
-    val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement()
-
-    @get:Rule(order = 1)
+    @get:Rule
     val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en")
 
     @Test