From 73b3282939b57c846e2a850853f1a3732594c9ec Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 1 Oct 2026 15:44:26 +0200 Subject: [PATCH 1/2] Stop rewriting screenshots that did not meaningfully change Recording rewrote every screenshot unconditionally, so renderer noise far below the verification tolerance produced large "Update screenshots" commits. Use `app.cash.paparazzi.overwriteOnMaxPercentDifference` to keep screenshots that are within tolerance. Existing screenshots are no longer deleted before recording; obsolete ones are pruned afterwards, only when their preview produced no screenshot at all. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../workflows/scripts/recordScreenshots.sh | 7 +- build.gradle.kts | 84 +++++++++++++++++-- gradle.properties | 6 ++ 3 files changed, 89 insertions(+), 8 deletions(-) diff --git a/.github/workflows/scripts/recordScreenshots.sh b/.github/workflows/scripts/recordScreenshots.sh index d4122c22bf..952c552ce4 100755 --- a/.github/workflows/scripts/recordScreenshots.sh +++ b/.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 diff --git a/build.gradle.kts b/build.gradle.kts index e7a03b403d..ef8cd772c8 100644 --- a/build.gradle.kts +++ b/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 (see LayoutLibErrorFilterStatement). 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/.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 ".#" and the screenshot file is named + // "__.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 { diff --git a/gradle.properties b/gradle.properties index 26ebbabf4d..27f8a43117 100644 --- a/gradle.properties +++ b/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 From 56f8cffe3493b5c171498872d0059dc682a39f51 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 1 Oct 2026 15:58:52 +0200 Subject: [PATCH 2/2] Remove LayoutLibErrorFilterStatement Paparazzi 2.0.0-alpha05.1 ships layoutlib 16.2.3, which fixes the errors this rule was swallowing. All screenshot tests pass without it. Co-Authored-By: Claude Opus 5.5 (1M context) --- build.gradle.kts | 2 +- .../ui/LayoutLibErrorFilterStatement.kt | 40 ------------------- .../src/test/kotlin/ui/PreviewA11yTest.kt | 5 +-- .../src/test/kotlin/ui/PreviewShard1Test.kt | 5 +-- .../src/test/kotlin/ui/PreviewShard2Test.kt | 5 +-- .../src/test/kotlin/ui/PreviewShard3Test.kt | 5 +-- .../src/test/kotlin/ui/PreviewShard4Test.kt | 5 +-- 7 files changed, 6 insertions(+), 61 deletions(-) delete mode 100644 tests/uitests/src/test/kotlin/ui/LayoutLibErrorFilterStatement.kt diff --git a/build.gradle.kts b/build.gradle.kts index ef8cd772c8..3c9d907586 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -237,7 +237,7 @@ subprojects { // // 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 (see LayoutLibErrorFilterStatement). Matching whole previews means such +// 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` diff --git a/tests/uitests/src/test/kotlin/ui/LayoutLibErrorFilterStatement.kt b/tests/uitests/src/test/kotlin/ui/LayoutLibErrorFilterStatement.kt deleted file mode 100644 index 9f91f30816..0000000000 --- a/tests/uitests/src/test/kotlin/ui/LayoutLibErrorFilterStatement.kt +++ /dev/null @@ -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 - } - } - } - } -} diff --git a/tests/uitests/src/test/kotlin/ui/PreviewA11yTest.kt b/tests/uitests/src/test/kotlin/ui/PreviewA11yTest.kt index f420110f31..6f8fe2f506 100644 --- a/tests/uitests/src/test/kotlin/ui/PreviewA11yTest.kt +++ b/tests/uitests/src/test/kotlin/ui/PreviewA11yTest.kt @@ -29,10 +29,7 @@ class PreviewA11yTest( @TestParameter(valuesProvider = ComposableA11yPreviewProvider::class) val preview: ComposablePreview, ) { - @get:Rule(order = 0) - val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement() - - @get:Rule(order = 1) + @get:Rule val paparazziRule = PaparazziPreviewRule.createFor( preview = preview, locale = "en", diff --git a/tests/uitests/src/test/kotlin/ui/PreviewShard1Test.kt b/tests/uitests/src/test/kotlin/ui/PreviewShard1Test.kt index 17632c212d..efb1a82cba 100644 --- a/tests/uitests/src/test/kotlin/ui/PreviewShard1Test.kt +++ b/tests/uitests/src/test/kotlin/ui/PreviewShard1Test.kt @@ -28,10 +28,7 @@ class PreviewShard1Test( @TestParameter(valuesProvider = Shard1ComposablePreviewProvider::class) val preview: ComposablePreview, ) { - @get:Rule(order = 0) - val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement() - - @get:Rule(order = 1) + @get:Rule val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en") @Test diff --git a/tests/uitests/src/test/kotlin/ui/PreviewShard2Test.kt b/tests/uitests/src/test/kotlin/ui/PreviewShard2Test.kt index cec2b028ac..c3312bad3b 100644 --- a/tests/uitests/src/test/kotlin/ui/PreviewShard2Test.kt +++ b/tests/uitests/src/test/kotlin/ui/PreviewShard2Test.kt @@ -28,10 +28,7 @@ class PreviewShard2Test( @TestParameter(valuesProvider = Shard2ComposablePreviewProvider::class) val preview: ComposablePreview, ) { - @get:Rule(order = 0) - val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement() - - @get:Rule(order = 1) + @get:Rule val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en") @Test diff --git a/tests/uitests/src/test/kotlin/ui/PreviewShard3Test.kt b/tests/uitests/src/test/kotlin/ui/PreviewShard3Test.kt index c752d0b321..d4508fb821 100644 --- a/tests/uitests/src/test/kotlin/ui/PreviewShard3Test.kt +++ b/tests/uitests/src/test/kotlin/ui/PreviewShard3Test.kt @@ -28,10 +28,7 @@ class PreviewShard3Test( @TestParameter(valuesProvider = Shard3ComposablePreviewProvider::class) val preview: ComposablePreview, ) { - @get:Rule(order = 0) - val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement() - - @get:Rule(order = 1) + @get:Rule val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en") @Test diff --git a/tests/uitests/src/test/kotlin/ui/PreviewShard4Test.kt b/tests/uitests/src/test/kotlin/ui/PreviewShard4Test.kt index 55881de079..c0e3b807b7 100644 --- a/tests/uitests/src/test/kotlin/ui/PreviewShard4Test.kt +++ b/tests/uitests/src/test/kotlin/ui/PreviewShard4Test.kt @@ -28,10 +28,7 @@ class PreviewShard4Test( @TestParameter(valuesProvider = Shard4ComposablePreviewProvider::class) val preview: ComposablePreview, ) { - @get:Rule(order = 0) - val layoutLibErrorFilterStatement = LayoutLibErrorFilterStatement() - - @get:Rule(order = 1) + @get:Rule val paparazziRule = PaparazziPreviewRule.createFor(preview, locale = "en") @Test