takahirom / roborazzi

Make JVM Android integration test visible 🤖📸
https://takahirom.github.io/roborazzi/
Apache License 2.0
649 stars 24 forks source link

Integrate ComposablePreviewScanner #415

Open yschimke opened 1 week ago

yschimke commented 1 week ago

https://github.com/sergio-sastre/ComposablePreviewScanner requires apps do the integration with the preferred screenshot library

Example https://github.com/sergio-sastre/ComposablePreviewScanner?tab=readme-ov-file#roborazzi

But it could be a great experience if the roborazzi plugin had an option to enable screenshot testing of previews with no extra code or config.

Theoretical developer Steps

  1. Use previews in a gradle project
  2. Add io.github.takahirom.roborazzi plugin to build
  3. Configure via RoborazziExtension
roborazzi {
  automaticPreviewScreenshots = true
}

This would cover maybe 80% of a typical apps screenshots, while promoting routine use of Previews for development, and allowing unit tests for the other 20%.

takahirom commented 1 week ago

Thank you for your proposal! I think it is great to have APIs that users can set up easily, while also having basic APIs for advanced usages. For example, we can add dependencies like Robolectric and options like hardware rendering when using the automatic option.

takahirom commented 1 week ago

One challenge is automatically adding a test. Here's how I think we can add a test:

https://github.com/takahirom/roborazzi/blob/main/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt

        val generateTestsTask = project.tasks.register<GeneratePreviewScreenshotTestsTask>("generate${variant.name.capitalize()}PreviewScreenshotTests") {
          outputDir.set(project.layout.buildDirectory.dir("generated-tests/preview-screenshot"))
        }
        variant.sources.kotlin?.addGeneratedSourceDirectory(
          generateTestsTask,
          GeneratePreviewScreenshotTestsTask::outputDir
        )
....

abstract class GeneratePreviewScreenshotTestsTask : DefaultTask() {
    @get:OutputDirectory
    abstract val outputDir: DirectoryProperty

    @TaskAction
    fun generateTests() {
        val testDir = outputDir.get().asFile
        testDir.mkdirs()

        File(testDir, "GeneratedPreviewScreenshotTest.kt").writeText("""
            import org.junit.Test

            class GeneratedPreviewScreenshotTest {
                @Test
                fun testPreviewScreenshot() {
                }
            }
        """.trimIndent())
    }
}
takahirom commented 1 week ago

I think we can implement this feature by following these steps:

  1. Add the automaticPreviewScreenshots Gradle extension property.
  2. Add dependencies like Robolectric and ComposablePreviewScanner if automaticPreviewScreenshots exists.
  3. Add the GeneratePreviewScreenshotTestsTask and set it up if automaticPreviewScreenshots exists.
sergio-sastre commented 1 week ago

I think it is important to mention that ComposablePreviewScanner is based on ClassGraph which finds the Previews while running the jvm tests (so no code generation or ksp involved), and such tests run at build-time as stated here

Theoretically it should be possible, but I am a little bit unsure how good it works inside a Gradle plugin, and I think the first step would be to try it out and see if it finds some Composables in a very basic gradle plugin that does nothing else but that.

That would be the very first step I’d try.

Regarding the mapping between the Previews and Robolecteic/Roborazzi, I can help with that once I am back from Droidcon Berlin 😊

takahirom commented 1 week ago

Thanks. I'm thinking of adding a generated test, which uses ComposablePreviewScanner, to the test Gradle task to generate screenshots instead of loading Previews in the Gradle Plugin. This approach is easier and would allow users to migrate their own customized tests. If you have any opinions on this, please let me know.

Sadly, I don't have time to implement this now, so I added the contributions welcome label. I'll be available in late September.

sergio-sastre commented 1 week ago

Oh I see, that should definitely work without troubles

takahirom commented 1 week ago

With just adding one hundred lines of code, it seems to have worked 🎉 https://github.com/takahirom/roborazzi/pull/416/files#diff-2ec7175e0bee85fe7e7d1d5d40dbf060b59731fcbd6dac9cc2db230a9ed84876R22 All users have to do is add this.

plugins {
  id("io.github.takahirom.roborazzi")
}

roborazzi {
  automaticPreviewScreenshots {
    enabled = true
    scanPackages = listOf("com.github.takahirom.sample")
  }
}
takahirom commented 1 week ago

~I have a small issue: I'm not sure how we can access the version catalog during compile time from the plugin.~

https://github.com/takahirom/roborazzi/blob/8d68dea6f3a9372d931d73451d75c3a1a5dffd17/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/PreviewScreenshots.kt#L49-L63

Fixed in https://github.com/takahirom/roborazzi/pull/416/commits/6c115fe6a2ee83396bc41240d7789d3679c9f599

yschimke commented 1 week ago

Is naming of multipreviews a follow up? It wasn't clear it's handled.

But amazing, ship it!

sergio-sastre commented 1 week ago

Wow! Super fast 🏎️ I’ll try to do a code review today in the evening (CET time)

sergio-sastre commented 1 week ago

Wow! Super fast 🏎️ I’ll try to do a code review today in the evening (CET time)

@takahirom Done! I believe my comment regarding the screenshot file name is the most important to ensure it does not override files for preview with the same method names but in different classes as well as extra previews generated due to @PreviewParameters.

The other comments relate to niceties we could add in future versions, although setting the most relevant PreviewInfos (e.g. locale, fontScale, etc. ) in Robolectric is sth I'd expect in the very first version as well.

Your call ;)

takahirom commented 6 days ago

I’m considering making the DSL more flexible and would appreciate your feedback.

Current Configuration:

roborazzi {
  automaticPreviewScreenshots {
    enabled = true
    scanPackageTrees = listOf("com.github.takahirom.sample")
  }
}

Proposal: Minimum setup for preview screenshot tests:

roborazzi {
  androidSetup {
    enable = true
    generatePreviewTests {
      scanPackages = listOf("com.github.takahirom.sample")
    }
  }
}

You can disable or enable all setups:

roborazzi {
  androidSetup {
    enable = true
    generatePreviewTests {
      enable = true // Inherits from setupOptions if not specified
      scanPackages = listOf("com.github.takahirom.sample")
    }
    libraryDependencies {
      enable = true
      robolectricVersion = "1.xxx"
    }
    testConfig {
      enable = false
      includeAndroidResources = true
      roborazziFilePathStrategy = "relativeFromOutputDir"
      robolectricRenderMode = "hardware"
    }
  }
}
takahirom commented 6 days ago

These changes allow users to copy and reuse GeneratedPreview tests, disable specific settings like dependencies, and easily set up screenshot tests even if they opt not to use Preview Screenshot tests. We can also introduce new settings with defaults set to false. Additionally, naming the setup as androidSetup leaves room for possible expansion to Kotlin Multiplatform.

sergio-sastre commented 6 days ago

I’m considering making the DSL more flexible and would appreciate your feedback.

Current Configuration:

roborazzi {
  automaticPreviewScreenshots {
    enabled = true
    scanPackageTrees = listOf("com.github.takahirom.sample")
  }
}

Proposal: Minimum setup for preview screenshot tests:

roborazzi {
  androidSetup {
    enable = true
    generatePreviewTests {
      scanPackages = listOf("com.github.takahirom.sample")
    }
  }
}

You can disable or enable all setups:

roborazzi {
  androidSetup {
    enable = true
    generatePreviewTests {
      enable = true // Inherits from setupOptions if not specified
      scanPackages = listOf("com.github.takahirom.sample")
    }
    libraryDependencies {
      enable = true
      robolectricVersion = "1.xxx"
    }
    testConfig {
      enable = false
      includeAndroidResources = true
      roborazziFilePathStrategy = "relativeFromOutputDir"
      robolectricRenderMode = "hardware"
    }
  }
}

Looks good, I just have one concern. There are things that could be set in the plugin as well as dependency or part of the gradle file. For instance, the testConfig. So what would happen if one sets other values in the gradle file than inside the roborazzi plugin?

Also for the library versions, since it is not inside generatePreviewTests block, it applies to roborazzi in general, so could be confusing how it behaves if such dependencies are also defined outside.

IMHO, Although I like the idea, I’m afraid it could confuse users, and for now I’d just scope those things inside the generatePreviewTests, maybe inside the androidSetup to support multiplatform options later as you mentioned.

I’d also name it scanPackageTrees like in the lib for consistency (I think I named it wrong, and should be scanTreePackages 🤔) I prefer it to just packages, since it also works for subpackages inside those packages

takahirom commented 6 days ago

I appreciate your feedback. I also find it confusing. I'm considering adding an additional abstraction layer for users.

roborazzi {
  baseSetupConfig(
    AndroidAutomaticPreviewScreenshots(listOf("com.github.takahirom.sample"))
  )
  advancedAndroidSetup {
    libraryDependencies.junitVersion = "4.13.2"
  }
}
open class RoborazziExtension @Inject constructor(objects: ObjectFactory) {
  val outputDir: DirectoryProperty = objects.directoryProperty()

  // UseCase based APIs
  sealed interface BaseSetupConfig {
    fun setup(advancedAndroidSetupExtension: AdvancedAndroidSetupExtension)
    fun name(): String
    data class BestPractice(val previewScanPackages: List<String>) :
      BaseSetupConfig by AndroidAutomaticPreviewScreenshots(previewScanPackages)

    data class AndroidAutomaticPreviewScreenshots(val scanPackages: List<String>) :
      BaseSetupConfig {
      override fun name(): String {
        return "AndroidAutomaticPreviewScreenshots"
      }

      override fun setup(advancedAndroidSetupExtension: AdvancedAndroidSetupExtension) {
        advancedAndroidSetupExtension.enable.set(true)
        advancedAndroidSetupExtension.generatePreviewTests {
          scanPackages.set(this@AndroidAutomaticPreviewScreenshots.scanPackages)
        }
      }
    }
  }

  @ExperimentalRoborazziApi
  fun baseSetupConfig(baseSetupConfig: BaseSetupConfig) {
    baseSetupConfig.setup(advancedAndroidSetup)
  }

  // Configuration based APIs
  @ExperimentalRoborazziApi
  val advancedAndroidSetup: AdvancedAndroidSetupExtension =
    objects.newInstance(AdvancedAndroidSetupExtension::class.java)

  @ExperimentalRoborazziApi
  fun advancedAndroidSetup(action: AdvancedAndroidSetupExtension.() -> Unit) {
    action(advancedAndroidSetup)
  }
}
yschimke commented 6 days ago

I don't think you should get involved in adding dependencies or configuring junit options. I think the pattern from Hilt, Compose and gRPC (IIRC) is to fail the build if they are unmet, with a clear error, and often a way to suppress.

So I'd suggest minimising the changes to the project structure and dependencies, instead executing the code paths you planned and explaining the requirements to the user through a simple clear actionable error.

takahirom commented 4 days ago

That's a good point. I've been considering what should and shouldn't be included in our scope. For example, I was wondering about the differences between setting dependencies and adding new tasks in the plugin. Adding tasks and properties means they become the single source of configuration, but changing something users can already configure can lead to losing that single source of truth.

I've come up with the following API. This setup checks dependencies and configurations and generates necessary source codes without altering user settings:

roborazzi {
  generateRobolectricPreviewTests {
      enable = true
      pacakges = listOf("com.github.takahirom.sample")
  }
}

(I'm still considering the naming of generateRobolectricPreviewTests.)

yschimke commented 4 days ago

Looks good. Typo in packages aside.

takahirom commented 2 days ago

Thank you. I have re-implemented the PR based on the feedback. https://github.com/takahirom/roborazzi/pull/416

yschimke commented 2 days ago

That looks great to me. Keen to try it.

sergio-sastre commented 2 days ago

@takahirom Droidcon Berlin starts tomorrow. I‘ll tske a look at the PR whenever I find time in the next days 🙏