From 73b3282939b57c846e2a850853f1a3732594c9ec Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 1 Oct 2026 15:44:26 +0200 Subject: [PATCH] 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