Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix non-existent snapshot directory when running verifyPaparazziDebug #1695

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix non-existent snapshot directory when running verifyPaparazziDebug #1695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
072a7ec
Fix non-existent snapshot directory when running verifyPaparazziDebug
geoff-powell Nov 15, 2024
7608ffb
Use file property instead of directory
geoff-powell Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ public class PaparazziPlugin @Inject constructor(
test.inputs.dir(
isVerifyRun.flatMap {
project.objects.directoryProperty().apply {
set(if (it) snapshotOutputDir else null)
set(if (it && snapshotOutputDir.asFile.exists()) snapshotOutputDir else null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use a file collection instead you won't have this issue. This works but creating that directory will cause a configuration cache miss when the directory is later created I believe.

Copy link
Collaborator Author

@geoff-powell geoff-powell Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I noticed that in the gradle docs but didn't go that way. Let me check that.

Updated 👍🏽

Copy link
Contributor

@ansman ansman Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to this should work as a drop in replacement:

inputs.files(isVerifyRun.map { if (it) listOf(snapshotOutputDir) else emptyList() })
    .withPropertyName("paparazzi.snapshots")
    .withPathSensitivity(PathSensitivity.RELATIVE)
    .optional()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works because calling input.files with an empty directory simply adds no input files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this works but we don't want to cache based on the snapshot files themselves and we really want to rely on the snapshot path alone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make caching work (and predictive test selection) the files themselves need to be included in the cache key so using a file property is not a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep makes sense, I opted to instead keep the directory property I originally had.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue with this approach is that the outcome can depend on when the task is configured. I also want to make clear that a directory property will still include the contents of the directory in the cache key for the task so you're still going to be checksumming all the snapshots (which is a good thing)

}
}
).withPropertyName("paparazzi.snapshot.input.dir")
Expand Down
12 changes: 12 additions & 0 deletions paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,18 @@ class PaparazziPluginTest {
assertThat(snapshotsDir.exists()).isTrue()
}

@Test
fun onlyJunitTest() {
val fixtureRoot = File("src/test/projects/only-junit-test")

val result = gradleRunner
.withArguments("verifyPaparazziDebug", "--stacktrace")
.forwardOutput()
.runFixture(fixtureRoot) { build() }

assertThat(result.task(":verifyPaparazziDebug")?.outcome).isEqualTo(SUCCESS)
}

@Test
fun invalidChars() {
val fixtureRoot = File("src/test/projects/invalid-chars")
Expand Down
20 changes: 20 additions & 0 deletions paparazzi-gradle-plugin/src/test/projects/only-junit-test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
plugins {
id 'com.android.library'
id 'kotlin-android'
id 'app.cash.paparazzi'
}

android {
namespace 'app.cash.paparazzi.plugin.test'
compileSdk libs.versions.compileSdk.get() as int
defaultConfig {
minSdk libs.versions.minSdk.get() as int
}
compileOptions {
sourceCompatibility = libs.versions.javaTarget.get()
targetCompatibility = libs.versions.javaTarget.get()
}
kotlinOptions {
jvmTarget = libs.versions.javaTarget.get()
}
}
8 changes: 8 additions & 0 deletions ...gin/src/test/projects/only-junit-test/src/test/java/app/cash/paparazzi/test/SampleTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package app.cash.paparazzi.test

import org.junit.Test

class SampleTest {
@Test
fun test() = Unit
}