Running paparazzi test from Android Studio/IDE doesn't fail but cmd line verifyPaparazziDebug task does

[kenyee]

Description It's not intuitive that running a Paparazzi test from the IDE doesn't fail but it does if you run ./gradlew module:verifyPaparazziDebug I noticed this when upgrading to Paparazzi 1.2.0 where the snapshot bitmaps were a different size.

Steps to Reproduce Create a project with Paparazzi 1.1.0 and a Paparazzi test Upgrade project to Paparazzi 1.2.0 Go into IDE and run your Paparazzi test and notice that it passes Use the cmd line to run verifyPaparazziDebug on the module and notice it fails

Expected behavior IDE run behavior should be the same as cmd line when running a Paparazzi test. I'd also expect an IDE run w/ a missing snapshot image to autocreate the image in the snapshot folder.

Additional information:

• Paparazzi Version: 1.2.0
• OS: OSX Ventura
• Compile SDK: 33
• Gradle Version: 7.5
• Android Gradle Plugin Version: 7.2.0

Screenshots N.A.

[TWiStErRob]

Silly question: did you sync the IDE after changing the version? Is it possible it was running with the classpath of the previous version?

[kenyee]

Yep. I just tried again:

• update version in libs.version.toml (from the latest Chris Banes fork to official 1.2.0)
• do a gradle sync in AS (Flamingo)
• run all Paparazzi tests in our UI components module by right clicking package name in AS IDE and choosing Run, and all passed
• run verifyPaparazziDebug on module via command line and tests fail

FWIW, this is how we set up the Paparazzi rule:

    @get:Rule
    val paparazzi = Paparazzi(
        deviceConfig = DeviceConfig.NEXUS_5.copy(softButtons = false, screenHeight = 0, screenWidth = 0),
        renderingMode = SessionParams.RenderingMode.FULL_EXPAND,
        maxPercentDifference = 0.01,
    )

[jrodbx]

Expected behavior IDE run behavior should be the same as cmd line when running a Paparazzi test. I'd also expect an IDE run w/ a missing snapshot image to autocreate the image in the snapshot folder.

The IDE test plugins run test${VARIANT} instead of verifyPaparazzi${VARIANT}, so the expected behavior is inaccurate. The version change isn't the root cause; it's intended design. There has been conversation in the past around whether test should default to verification or its current behavior of generating the HTML report in the build dir -- the stance remains that test does not -- by design -- run verification because a record needs to be ran first, forcing the tester to update the golden images prior to a verification. This in short means the IDE behavior doesn't work as is.

Some nice ways to workaround this:

• A Paparazzi IDE plugin would help here
• setting the current system property directly to force Paparazzi into verification mode
• providing a new system property that changes the default behavior of the test run, defaulting to its current behavior if not set.

Closing this for now as works as expected

[kenyee]

whether test should default to verification or its current behavior of generating the HTML report in the build dir -- the stance remains that test does not -- by design -- run verification because a record needs to be ran first, forcing the tester to update the golden images prior to a verification

I'd say my expectation is if the record was never done, the test from an IDE would fail with an exception telling the user that a record is needed. Having it generate an HTML report is definitely not what I'd expect by default :-)

setting the current system property directly to force Paparazzi into verification mode

What's the current system property that forces verification mode? I didn't see mention of it in the repo or the docs.

A Paparazzi IDE plugin would help here

+1 would be nice if a context menu popped up that showed "record" and "verify" options.

[TWiStErRob]

setting the current system property directly to force Paparazzi into verification mode

What's the current system property that forces verification mode? I didn't see mention of it in the repo or the docs.

@kenyee, @jrodbx is talking about these two:

https://github.com/cashapp/paparazzi/blob/71bfd7f92fa667cdccb44a1170b74c13842dca51/paparazzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt#L173-L174

they're internal to Paparazzi and might change in the future.

That said, you can pretty safely use the workaround John suggested, because even when it doesn't work, you'll know straight away: the workaround will stop working. Here's a way to do this (untested):

tasks.withType<Test>().configureEach {
    systemProperties["paparazzi.test.record"] = false
    systemProperties["paparazzi.test.verify"] = true
}

This above will set the properties, and the Paparazzi plugin will overwrite when necessary (record/verify tasks executed), so you're only changing the defaults.

You can spice it up even more by only changing the IDE behavior, and not applying this in command line.

providing a new system property that changes the default behavior of the test run, defaulting to its current behavior if not set.

If anyone wants to contribute, this mentioned workaround is pretty much the same as above, with the exception of an if around it, and it existing in the Paparazzi codebase.

[kenyee]

ahh..nice. Thanks @TWiStErRob. We can add this to one of our gradle plugins 😸

[kenyee]

@TWiStErRob does this workaround work with the config cache?

The code looks like this:

    fun paparazzi() {
        with(project) {
            with(pluginManager) {
                apply("app.cash.paparazzi")
            }
            // This makes running a test in the IDE do a Paparazzi verify instead of generating an HTML report
            if (System.getProperty("idea.active").toBoolean()) {
                tasks.withType<Test>().configureEach {
                    println("******* activating paparazzi verify mode on ${this.name} *******")
                    systemProperties["paparazzi.test.record"] = false
                    systemProperties["paparazzi.test.verify"] = true
                }
            }
        }
    }

and I verified that "****" does get printed during config time. But when I modify one of the screenshot files, it still doesn't do a verify in the IDE when I right click the test and run it.
Running command line fails the test as expected.

The only thing I can think of is the config cache is interfering with it somehow 🤔

[TWiStErRob]

Turn it off and try that way, just set it to off in gradle.properties, make sure it works consistently as you expect, and then put config cache on top of it. I'm not sure if System.getProperty is valid in config cache.

Also double-check the PaparazziPlugin code, maybe there's more setup needed.

[kenyee]

I forgot to mention I also tried removing the System.getProperty conditional (set it to true). What's weird is I added these to the project's gradle.properties: paparazzi.test.record=false paparazzi.test.verify=true And it's the same behavior...IDE run of test doesn't do the verify and just generates the html report.

[TWiStErRob]

That's a totally different level, you need to make sure that in the runtime of the test the system properties exists. Gradle build won't (I really hope) automatically expose props. Add a println in your test to see if it's picked up. I'm really not sure what else could be missing, as I don't know how Paparazzi internals handle this. Your reading of code will be as good as mine ;)

[kenyee]

FYI, I tried disabling the config cache in gradle.properties and that didn't help (verified that it did print out that it added the sysprops on the testDebugUnitTest task when gradle syncing).

Also added this bit in the test and those sysprops are definitely missing in the test output:

        val sysProp = System.getProperty("paparazzi.test.verify")
        println("***** verify set to $sysProp *****\nSystem props: ${System.getProperties()}")

I don't think the

tasks.withType<Test>().configureEach {

is passing the system props to the Java JVM when running the tests (even through it should from everything I've dug up).

[TWiStErRob]

Silly question, but: is it in the right submodule?

[kenyee]

Silly question, but: is it in the right submodule?

Fair question. The code above is run in a DSL that is used in that module, but I also dumped this into that module's build.gradle.kts directly just in case our DSL code was incorrect:

tasks.withType<Test>().configureEach {
    println("******* double activating paparazzi verify mode on ${this.name} in ${this.project.name} *******")
    systemProperties["paparazzi.test.record"] = "false"
    systemProperties["paparazzi.test.verify"] = "true"
}

and verified the module name and task was testDebugUnitTest.

[kenyee]

Asked a coworker to sanity check me and found that adding this:

systemProperties["random.value.for.testing"] = "bizarro"

is a value that makes it to the tests...printing out System.getProperties() includes this "random.value.for.testing" value, but not the paparazzi ones. Time to look for paparazzi code that clears these values... 🤔

[kenyee]

Ahh...the code setting it is here: https://github.com/cashapp/paparazzi/blob/master/paparazzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt#L173

It no longer looks at a system property, but it sets it via this:

isVerifyRun.set(verifyTaskProvider.map { graph.hasTask(it) })

which just checks that a user has specified the verifyPaparazzi task in the command line, which of course the IDE isn't doing.

This plugin have to be modified to look at a system prop to be able to do what I'd want...something like:

val shouldIdeVerify = test.systemProperties["paparazzi.ide.verify"] ?: false 
isVerifyRun.set(verifyTaskProvider.map { graph.hasTask(it) } || shouldIdeVerify)

Given previous comments, it seems like this is verboten so a PR would not be accepted?

[TWiStErRob]

Ah, the order! Paparazzi's doFirst overwrites it... Sorry 🤦‍♂️ Then I'm not sure what could be done other than first party flag as you said.

[kozaxinan]

Could you check if system property exists before setting it in doFirst? if it is set by the call itself, plugins doesnt have to override the values.