Add automatic preview screenshot testing with Paparazzi and Showkase

[alexvanyo]

Automatic @Preview screenshot testing utilizing Showkase, Paparazzi and TestParameterInjector.

Showkase aggregates all @Preview definitions, and collects them into a list of components via a KSP processor. We can then use these components as parameterized inputs to tests configured with TestParameterInjector, and then we use Paparazzi to render each preview and diff the output.

This CL configures Showkase to run on all UI modules, and then aggregates them all in a test-only preview-screenshots module. These tests run all previews in a matrix of configurations:

• Each @Preview
• Nexus 5, Pixel 5, Pixel C
• Font scale of 1.0 and 1.5

This could be expanded even further if desired (locale, theming, etc.)

Recording tests can be done with ./gradlew recordPaparazziDemoDebug, and then checked with ./gradlew verifyPaparazziDemoDebug. check is configured to depend on verifyPaparazziDemoDebug.

This PR currently does not do anything with Git LFS for storing the screenshots, as recommended here: https://github.com/cashapp/paparazzi#git-lfs

Change-Id: I13a64e187517c08a69487caca09c1db52c6e4c98

[JoseAlcerreca]

The new tests are going to be run through the test task, making it slower but not actually verifying screenshots, right? They should be excluded and the correct task added to the workflow.

[JoseAlcerreca]

Also, can we make the screenshots for components smaller? The PNGs shouldn't take much more space but the reports will.

[alexvanyo]

The new tests are going to be run through the test task, making it slower but not actually verifying screenshots, right? They should be excluded and the correct task added to the workflow.

Ah good catch, yep. I converted the testDebug to check in the workflow in https://github.com/android/nowinandroid/pull/101/commits/14b70a822ce9635ee0beb4bc865d95e0466df026

Also, can we make the screenshots for components smaller? The PNGs shouldn't take much more space but the reports will.

Hmm I don't know of a way for thebuild-outputs folder. Git LFS should help with the repo size, but we might need to add in a step to compress the PNG for the reports, or exclude them in some way?

[JoseAlcerreca]

Ah good catch, yep. I converted the testDebug to check in the workflow in 14b70a8

How about we run screenshot tests in their own step instead? I'd like to track how much time they take.

Hmm I don't know of a way for thebuild-outputs folder. Git LFS should help with the repo size, but we might need to add in a step to compress the PNG for the reports, or exclude them in some way?

I mean the actual window that is screenshot. For example, there's a lot of background here: https://github.com/android/nowinandroid/compare/main...av/paparazzi-showkase-preview-screenshot-testing#diff-dfed63bbea9eea2c761938aa17b5db0477ce26b28c433737401f8b19a9939059

[alexvanyo]

How about we run screenshot tests in their own step instead? I'd like to track how much time they take.

Good idea, updated to split those out in the latest set of commits. (Sorry for the force pushes here, I want to keep rebasing to ensure the screenshots are up to date with other changes being merged)

I mean the actual window that is screenshot. For example, there's a lot of background here:

We can configure Paparazzi to have a "undetermined" size, but I can't think of a rule that we could broadly apply to all previews that makes sense.

For previews of very small components, then we don't need all the background and we could render the intrinsic size of the component.

For the previews of entire screens (or anything else that will fill up all of the space applied), then we want to use the full device size to constrain the component.

A couple options:

• We could avoid using Showkase to collect all of the previews, and manually define tests completely separately. Then we could setup tests on a case-by-case basis so that they are applicable to what we are verifying.

• We could try to insert some post-screenshot, pre-saving to disk logic to adjust what the screenshots look like. We could use that to remove the background (if we can somehow detect that).

[alexvanyo]

Capture of latest run after splitting:

[JolandaVerhoef]

#72 implements a Preview strategy for Feature screens that already renders them on different sizes. Using Showkase, this will now explode as we will render each preview dimension on each device. We will need a better approach for that.

For the design system components, I'm planning to implement a lot of different Previews for each component (component types x font scale x dark/light etc) so this would again lead to duplicate effort if Paparazzi is configured to do the same.

[alexvanyo]

Currently, the setup only treats as input as the set of @Composable functions annotated with @Preview, as opposed to set of all @Preview annotations. Then, it applies the dimensions independently, without looking at the contents of @Preview.

Which isn't ideal, but does avoid the explosion at the cost of duplication. There's definitely room for tighter integration there, where there would be a 1:1 mapping between the previews that we see rendered in Android Studio itself (with all of the theming and font scale configuration) with the list of screenshots captured and verified against goldens.

[vinaygaba]

Fwiw, Showkase has a way of skipping some previews if we don't want them to be a part of the Showkase metadata - https://github.com/airbnb/Showkase/blob/master/showkase-annotation/src/main/java/com/airbnb/android/showkase/annotation/ShowkaseComposable.kt#L67

This flag allows you to continue showing previews in AS while skipping them from the Showkase browser/screenshot tests

@ShowkaseComposable(..., skip = true)
@Preview(...)
fun MyComponent() {

}

This will ensure that you continue seeing the previews in AS, while limiting what you end up screenshot testing. I'm also keen on learning more about how you are thinking about it and would be happy to make necessary modifications to Showkase if they make sense!

[JolandaVerhoef]

Some more questions:

• Do you know how Showkase works with Multipreview annotated previews?
• Could we setup Showkase differently for the different modules? That way we could use different preview config for Design System vs feature modules.

[alexvanyo]

Do you know how Showkase works with Multipreview annotated previews?

They currently aren't supported: https://github.com/airbnb/Showkase/issues/233

Could we setup Showkase differently for the different modules? That way we could use different preview config for Design System vs feature modules.

Yes, definitely. Right now, everything is being collected into the single preview-screenshots module, but we could collect them in each module independently and apply rules or configuration on a case-by-case basis.

[vinaygaba]

They currently aren't supported: https://github.com/airbnb/Showkase/issues/233

@alexvanyo I can fix that issue that you pointed to relatively easily. What's stopping me is some nuance on how the multi previews should be handled as the primary motivation is to render it across different devices (which Showkase obviously doesn't need to do since it runs on a single device)

Could we setup Showkase differently for the different modules? That way we could use different preview config for Design System vs feature modules.

Like Alex pointed out, this is absolutely possible. You just add Showkase to the modules that you want Showkase to generate code for. In your example, you can set it up just for the design system modules.

[JolandaVerhoef]

What's stopping me is some nuance on how the multi previews should be handled as the primary motivation is to render it across different devices (which Showkase obviously doesn't need to do since it runs on a single device)

Multipreview is not specifically meant to render on multiple devices. You can combine each type of preview parameters, e.g. font scales, dark & light mode, etc etc.

I'd expect each composable with either a Preview or a multipreview annotation to be included in the set that Showkase uses.

[vinaygaba]

@JolandaVerhoef That's fair! I'm going to take a stab at adding support for multipreview this week. Will keep y'all posted ✌️

[JoseAlcerreca]

I couldn't find the test reports in the artifacts. It might be a bigger config problem though.

[JolandaVerhoef]

About the repo size: does this PR already configure Git LFS? Let's make sure we fix this before merging.

[alexvanyo]

One issue with Git LFS: I haven't set it up myself, but my understanding via https://github.com/git-lfs/git-lfs/discussions/4644 is that it is an extra tool that needs to be installed locally by any user cloning the repository, or else the clone would fail. I'm wondering if that would be too much friction for getting the project up and running?

[JoseAlcerreca]

git-lfs/git-lfs#4644 is that it is an extra tool that needs to be installed locally by any user cloning the repository, or else the clone would fail.

That thread is ambiguous. We need to do our own testing making sure we use different machines or VMs, as config files left by lfs might be the problem.

[dturner]

Just had a chat with @alexvanyo about this. To move this forward we need to do one of the following options:

• Don't merge (just an exploration)
• Merge just Paparazzi, setup manual screenshot tests for everything we want to test
• Merge Paparazzi and showcase - would automatically setup screenshot tests for everything in the app. Risk: both are external libs, what are actually recommending as best practices?
• Could do a mix of the above

Other points:

• Showcase crashes with Compose previews (reason not to merge showcase)
• If we want to target API33 we need a different version of Paparazzi - fragile dependencies.

The value here is huge though, so we should seriously consider merging this.

[vinaygaba]

@dturner We've started the work to have multi-preview support in Showkase (which was the missing piece). Is that the crash you were referring to?

[alexvanyo]

@vinaygaba yes, I saw that's being worked on! I think the current PR I saw will make sure it won't crash, but I think we'd also want to have any composables that are annotated with @Preview or a custom multi-preview annotation picked up for the automatic screenshots for preview support with Showkase.

[vinaygaba]

@alexvanyo Yes, it's being worked on. This is the second PR with the goal to have full multi preview support - https://github.com/airbnb/Showkase/pull/259/

The previous PR ensured that there are no crashes anymore - https://github.com/airbnb/Showkase/pull/255

We'd need 1 more PR after this to allow custom annotations to be used as previews

[alexvanyo]

Also investigated what using Git LFS implies using https://github.com/google/horologist (which has Git LFS set up)

If Git LFS is not installed, the repo can still be cloned, but all of the images will remain in the Git LFS pointer format.

What that means is that there will still be a some_screenshot.png file locally, but it won't actually be a real .png: it will be a text file with the pointer information.

That means that the app will build normally to run, but running screenshots tests will fail because the real .png files won't be downloaded.

For any contributor uploading a PR (where .png files are affected), Git LFS would have to be set up, so that the pointer files are correctly created and updated.

I think that means we can use Git LFS, as it doesn't add an additional step for cloning and running the project directly, with a couple of warnings about it:

• [ ] Mention in the README that screenshot tests will fail unless Git LFS is set up (and also explain why we are using Git LFS)
• [ ] Mention in the contributing guide to set up Git LFS
• [ ] Add a CI step where we verify that all of the screenshot .png files we expect are actually in the Git LFS pointer file format. That should ensure that actual .png files don't get merged (that were created without Git LFS installed)

We would also need to ensure Git LFS works with our internal setup.

[mmoczkowski]

@alexvanyo Alex, looks like last activity on this PR was some time ago. Do you think we can close it?

[mmoczkowski]

Closing due to inactivity