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.

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

Cannot verify module that doesn't produce snapshots #1692

Open
ansman opened this issue Nov 14, 2024 · 12 comments
Open

Cannot verify module that doesn't produce snapshots #1692

ansman opened this issue Nov 14, 2024 · 12 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ansman
Copy link
Contributor

ansman commented Nov 14, 2024

Description
We use paparazzi to verify compose previews, some of which produce snapshots while others don't.
When you run paparazzi on a module that only contains tests without snapshots the snapshot task fails because the snapshot directory doesn't exist.

The fix is quite simple, the input directory just needs to be a file collection instead:

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

Expected behavior
I expect the verification task to run and only require fail if a snapshot is missing.

Additional information:

  • Paparazzi Version: 1.3.5
  • Gradle Version: 8.11
@ansman ansman added the bug Something isn't working label Nov 14, 2024
@jrodbx
Copy link
Collaborator

jrodbx commented Nov 14, 2024

Please provide a repro sample, I suspect that your test tasks are being eagerly realized.

@jrodbx jrodbx added the waiting on user Waiting on information from OP label Nov 14, 2024
@ansman
Copy link
Contributor Author

ansman commented Nov 14, 2024

Here is a small sample repo, simply run ./gradlew :app:verifyPaparazziDebug

This happens when you try and verify in a module that doesn't have any snapshots (either because it has none recorded or because it has no paparazzi tests).

We are hitting this because we apply paparazzi in our convention plugin even if there are no paparazzi tests in that module.

PaparazziTest.zip

@geoff-powell geoff-powell self-assigned this Nov 15, 2024
@geoff-powell
Copy link
Collaborator

Thanks for the test repro, found the issue and have a pr. #1695

@geoff-powell geoff-powell added this to the 2.0.0-alpha01 milestone Nov 15, 2024
@ashley-figueira
Copy link

I've implemented the following workaround in my project in order to update and not have to wait for a new release.

import org.gradle.api.DefaultTask
import org.gradle.api.Project
import org.gradle.api.file.ProjectLayout
import org.gradle.api.file.RegularFileProperty
import org.gradle.api.model.ObjectFactory
import org.gradle.api.provider.Property
import org.gradle.api.tasks.CacheableTask
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.OutputDirectory
import org.gradle.api.tasks.TaskAction
import org.gradle.api.tasks.TaskProvider
import org.gradle.api.tasks.testing.Test
import org.gradle.kotlin.dsl.property
import org.gradle.kotlin.dsl.register
import org.gradle.kotlin.dsl.withType
import javax.inject.Inject

@CacheableTask
abstract class CreateSnapshotsDirTask @Inject constructor(
    private val layout: ProjectLayout,
    objects: ObjectFactory,
) : DefaultTask() {

    @Input
    val snapshotPath: Property<String> = objects.property<String>().convention("src/test/snapshots")

    @OutputDirectory
    val snapshotDir: RegularFileProperty = objects.fileProperty().convention {
        layout.projectDirectory.file(snapshotPath.get()).asFile
    }

    @TaskAction
    fun run() {
        snapshotDir.get().asFile.mkdirs()
    }

    companion object {
        const val NAME = "createSnapshotsDirTask"


        fun register(project: Project) {
            val createSnapshotsDirTask: TaskProvider<CreateSnapshotsDirTask> =
                project.tasks.register<CreateSnapshotsDirTask>(NAME)

            project.tasks.withType<Test>().configureEach {
                inputs.files(createSnapshotsDirTask)
            }

        }
    }

}

And then I apply this custom gradle task to my plugin like so:

CreateSnapshotsDirTask.register(project)

@geoff-powell
Copy link
Collaborator

Talked with @jrodbx and other folks about this and I think the correct fix is to error the build and suggest recording snapshots with recordPaparazziDebug there shouldn't be a usecase to run the verify task when the directory is empty or snapshots weren't recorded.

@ab-gnm
Copy link

ab-gnm commented Nov 20, 2024

Thanks @geoff-powell . Do you have a link for the discussion?

I think the reason this is currently blocking updates for us and others is that we apply the Paparazzi plugin as part of our convention plugin that sets up Compose. This means that any Gradle module that has Compose set up, will also have Paparazzi plugin applied.
Not all of these Gradle modules may actually have any Paparazzi tests in them.

Prior to 1.3.5, running verifyPaparazzi checked present tests against snapshots. If a module had paparazzi applied but no tests, then it just ignored that.
With 1.3.5, running verifyPaparazzi will fail on all those modules without any tests. As these modules don't have any tests, they're not expected to have a snapshots folder.

If there's a strong POV from @jrodbx or others that we should only apply the Paparazzi plugin to modules that have one or more tests in them, then that'd be a breaking change in our integrations.
@ashley-figueira 's solution above is one such option - create empty snapshot directory in every module that has the plugin applied. Though I'm not sure if this has any side effects elsewhere.

@geoff-powell
Copy link
Collaborator

We discussed this in a meeting so don't have any links of the conversation.

Thanks for providing your use case. I think as a way to unblock yourself, I think the convention plugin could check the snapshot directory and apply the plugin based on that.

Let me think through this some more and update my PR to perhaps mimic paparazzi 1.3.4 and older.

@ansman
Copy link
Contributor Author

ansman commented Nov 20, 2024

This breaks out use case too. We use paparazzi to verify composables without producing screenshots which would be unsupported.

What is the reason for not wanting to make this change?

@adityabhaskar
Copy link

adityabhaskar commented Nov 20, 2024

I think as a way to unblock yourself, I think the convention plugin could check the snapshot directory and apply the plugin based on that.

This would theoretically work. The directory at the moment is created by running the record task. To run the record task we need to apply the plugin. To apply the plugin we now need the directory.

In practice, it'd mean that a developer would have to create an empty directory, then sync gradle, and then write and run tests. If this is the only solution, then @ashley-figueira's suggestion of pre-emptively creating empty directories from the convention plug-in is a better approach.

(It's still me @ab-gnm above. Using a different account from personal machines 🙂 )

@geoff-powell
Copy link
Collaborator

geoff-powell commented Nov 20, 2024

ya thats a good point about needing the plugin for recording.

I'm a little confused. How does applying the plugin cause an issue at all? Isn't the issue when the verify task is run on a module that doesn't have a snapshot directory?

My thought process to fix this issue is to allow for a non-existent directory not fail the verify task, but log a warning then said snapshot directory doesn't exist.

see this test: https://github.com/cashapp/paparazzi/pull/1695/files#diff-8c52db63811b414fc18b3fe88650c819563e678a6cf71aa913ef4820e2c1cba4R209-R222

@TWiStErRob
Copy link
Contributor

We use paparazzi to verify composables without producing screenshots

@ansman Does this mean you're using in place of Robolectric?

My thought process to fix this issue is to allow for a non-existent directory not fail the verify task, but log a warning then said snapshot directory doesn't exist.

@geoff-powell Gradle has a facility for this! The task outcome NO-SOURCE would mean exactly this, isn't it?

@ansman
Copy link
Contributor Author

ansman commented Nov 20, 2024

Does this mean you're using in place of Robolectric?

Yes correct. We've built a framework on top of Paparazzi that verifies that all compose previews can be executed. You can optionally generate a screenshot for the preview by adding an animation. So some modules have compose previews but none of them have screenshot testing enabled which causes this failure.

@geoff-powell geoff-powell removed the waiting on user Waiting on information from OP label Nov 21, 2024
jonnyandrew added a commit to govuk-one-login/mobile-android-cri-orchestrator that referenced this issue Jan 31, 2025
SimonMarquis added a commit to SimonMarquis/paparazzi that referenced this issue Feb 8, 2025
…t present

This is the 2nd PR to finally close cashapp#1716.

Also relates to:
- cashapp#1692
- cashapp#1695

since this still requires the presence of the `src/test/snapshots/images` input to work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants