cashapp / paparazzi

Render your Android screens without a physical device or emulator
https://cashapp.github.io/paparazzi/
Apache License 2.0
2.24k stars 212 forks source link

Multiple paparazzi rules in the same test run give NullPointerException #1340

Open eboudrant opened 4 months ago

eboudrant commented 4 months ago

Description We'd like to create our own rule to wrap some boiler plate code and help adopting some good practice by our users. One of it is to build snapshot test that cover font scale on device. Since the fontScale can only be set on the DeviceConfig I ended with a custom test rule that delegate to 2 Paparazzi rules (see code). When the test run we get a NPE because of initialization code.

app.cash.paparazzi.sample.ScaledTextTest > compositeItems FAILED
    java.lang.NullPointerException
        at app.cash.paparazzi.Paparazzi.takeSnapshots(Paparazzi.kt:285)
        at app.cash.paparazzi.Paparazzi.snapshot(Paparazzi.kt:216)
        at app.cash.paparazzi.Paparazzi.snapshot$default(Paparazzi.kt:215)
        at app.cash.paparazzi.Paparazzi.snapshot(Paparazzi.kt:211)
        at app.cash.paparazzi.sample.MyPaparazziRule.snapshot(MyPaparazziRule.kt:39)
        at app.cash.paparazzi.sample.MyPaparazziRule.snapshot$default(MyPaparazziRule.kt:34)
        at app.cash.paparazzi.sample.ScaledTextTest.compositeItems(ScaledTextTest.kt:14)

isInitialized is true for the second Paparazzi rule giving it an invalid state.

Also a side question I guess would be, how would you recommend to approach testing on variants that are at the DeviceConfig level. Ultimately we want our user to write the test once and have multiple snapshot per DeviceConfig property we care about.

Steps to Reproduce This branch https://github.com/eboudrant/paparazzi/tree/multiple_paparazzi contains a new test app.cash.paparazzi.sample.ScaledTextTest

Expected behavior Not crash, the test output would be 2 images

Additional information:

eboudrant commented 4 months ago

Btw, I just realized to solve the fontScale (or any attribute from the DeviceFactor) you can use @TestParameter at the test class level (see https://github.com/google/TestParameterInjector).

So paparazzi might just need a better error message than the NullPointerException noted here.

jrodbx commented 4 months ago

Btw, I just realized ... you can use @TestParameter at the test class level...

Correct! We have an example for that here.

So paparazzi might just need a better error message than the NullPointerException noted here.

Agreed. A second instance of Paparazzi rule is invalid so that sounds like the appropriate fix here. Wanna submit a PR?