Open realdadfish opened 3 months ago
takahirom
commented
3 months ago Thanks. I'm interested in understanding more about your use case for managing screenshots in Roborazzi tests. Specifically, in what situations would you prefer setting recordEnabled = false instead of not applying the plugin at all? Could you elaborate on the scenarios or reasoning behind this choice?
takahirom
commented
3 months ago Please take a look at the test report
FAILURE: Build failed with an exception.
* Where:
Build file '/private/var/folders/n2/pt_35rc53tdgkld9531s2tfh0000gn/T/junit4772681132119589584/app/build.gradle.kts' line: 83
* What went wrong:
Script compilation error:
Line 83: }roborazzi {
^ Unresolved reference. None of the following candidates is applicable because of receiver type mismatch:
public fun Project.roborazzi(configure: Action<RoborazziExtension>): Unit defined in org.gradle.kotlin.dsl
1 error
* Try:
> Run with --debug option to get more log output.
> Run with --scan to get full insights.
realdadfish
commented
3 months ago Please take a look at the test report
FAILURE: Build failed with an exception. * Where: Build file '/private/var/folders/n2/pt_35rc53tdgkld9531s2tfh0000gn/T/junit4772681132119589584/app/build.gradle.kts' line: 83 * What went wrong: Script compilation error: Line 83: }roborazzi { ^ Unresolved reference. None of the following candidates is applicable because of receiver type mismatch: public fun Project.roborazzi(configure: Action<RoborazziExtension>): Unit defined in org.gradle.kotlin.dsl 1 error * Try: > Run with --debug option to get more log output. > Run with --scan to get full insights.
Ah, I was unable to run the tests locally, so I trusted on the CI, fixed the wrapping issue.
realdadfish
commented
3 months ago Thanks. I'm interested in understanding more about your use case for managing screenshots in Roborazzi tests. Specifically, in what situations would you prefer setting recordEnabled = false instead of not applying the plugin at all? Could you elaborate on the scenarios or reasoning behind this choice?
Well, this is a good question. IMHO whenever the plugin is applied it should actually record screenshots by default, because this is the foundation of what the plugin always does, collect the individual screenshots that were taken in tests and create an overall report for it.
The reason why I introduced a boolean property here is mainly backwards compatibility. Users of the plugin are used to the fact that they have to specifically tell the plugin to do anything, and I did not want to break that behavior. Also, since running these tasks in addition to what the test execution does there is surely a bit overhead that people eventually not want to pay each time. I'd expect that screenshot and non-screenshot tests live side-by-side in a module and most of the time the non-screenshot tests don't need any tasks to be run to collect data from them (or even let the screenshot code in the tests be executed).
realdadfish
commented
3 months ago Ok, I figured I miss something still. Even though screenshot recording and report generation works with this change, during test execution this branch is executed anyways:
if (taskType.isEnabled() && !roborazziSystemPropertyTaskType().isEnabled()) {
println(
"Roborazzi Warning:\n" +
"You have specified '$taskType' without the necessary plugin configuration like roborazzi.test.record=true or ./gradlew recordRoborazziDebug.\n" +
"This may complicate your screenshot testing process because the behavior is not changeable. And it doesn't allow Roborazzi plugin to generate test report.\n" +
"Please ensure proper setup in gradle.properties or via Gradle tasks for optimal functionality."
)
}I originally thought I would tackle this by this here https://github.com/takahirom/roborazzi/pull/293/files#diff-dc9dacb4aa08c2bfedbc7cbce3ce02898ab79e36f1dad6a7d5c314f819f3cb5bR322, however, that does not seem to work. Would you happen to know if I'm missing something else?
realdadfish
commented
3 months ago Ah ok, I think I see the culprit, fixed in revision 1a52494
takahirom
commented
3 months ago If users are looking to use Roborazzi for screenshot tests, they'll likely need to run the verify or compare tasks. As I understand it from this PR, setting recordEnabled = true means recording will always occur, which could be somewhat confusing. We should consider the options' priority and relevant use cases more carefully. I'll need some time to think over these changes.
realdadfish
commented
2 months ago Hi @takahirom, have you thought about how to continue with this?
takahirom
commented
1 month ago Thanks for the updates. While I appreciate the convenience this could provide, I believe we need to consider the overall API design more comprehensively.
Currently, we have several options for enabling recording:
roborazzi.test.record=true in gradle.propertiesrecordRoborazziDebug Gradle taskRoborazziOptions(taskType = RoborazziTaskType.Record) in codeThis PR introduces recordEnable = true within the Roborazzi DSL, which diverges slightly by using the term "enable." This might cause confusion or overlap with the existing options, and it could potentially limit us to only "record" actions, which might not be clear.
Perhaps we could consider something like roborazzi { taskType = RoborazziTaskType.Record } to align with existing configurations. Additionally, establishing a clear priority order—gradle.property < Gradle DSL < task < RoborazziOptions.taskType—and documenting this could help clarify their usage and justify this addition.
realdadfish
commented
1 month ago @takahirom I made some changes to my original PR - was it this what you imagined here? Where would be a proper place to document the behaviour?
takahirom
commented
2 weeks ago Sorry for being super late. I think it looks great, and I would like to merge this. However, I would like to have a test that checks the priority of the options with tasks and gradle.properties. For the documentation, you can add it here. Perhaps we can include this <details><summary>Gradle extension setting for the default behavior of Roborazzi for the Gradle module</summary> The explanation would be here </details> https://github.com/takahirom/roborazzi/blob/main/docs/topics/build_setup.md
realdadfish
commented
2 weeks ago Sorry for being super late. I think it looks great, and I would like to merge this. However, I would like to have a test that checks the priority of the options with tasks and gradle.properties.
This is where I stumble - because frankly I didn't fully understood the current way it is designed right now. From what I read in the code there are warnings if the task is executed, but the property is not present (see processOutputImageAndReport.kt:46ff), if the current work mode is already that tasks override gradle.properties settings, this seems to be in conflict with this warning. I also don't see an existing test that would test tasks overriding gradle.properties settings, this is all a bit confusing :-/.
I know my change adds even more complexity to an already complex implementation, and I start to feel bad about this.
For the documentation, you can add it here. Perhaps we can include this
<details><summary>Gradle extension setting for the default behavior of Roborazzi for the Gradle module</summary> The explanation would be here </details>https://github.com/takahirom/roborazzi/blob/main/docs/topics/build_setup.md
I added preliminary documentation, added it however to the table, and added a small preface that explains the concept behind.
github-actions[bot]
commented
1 week ago 
I have an Android Monorepo with about a dozen components and hundreds of modules, including many Robolectric UI tests. To avoid having to "enable" screenshot recording and report generation on a per
gradle.propertiesbasis I looked for alternatives to run the report generation of screenshots by default, but could not find any suitable. I make heavy usage of build convention plugins, so the easiest was to have a custom "my.plugin.test.screenshots" build convention that would apply your plugin and, at the same time, enable screenshot reporting by default via the plugin's extension for the respective test tasks.I figured it makes no sense to have such a configuration for the verify and compare tasks; my primary use case really is to automatically see what's wrong when a Robolectric UI test fails with some assertion, to aid debugging the problem. While one can omit the usage of the Gradle plugin altogether and just use the core library (together with a
System.setProperty("roborazzi.test.record", "enabled")somewhere), I found it useful to re-use the HTML reporting capability that the plugin provides.From what I see Roborazzi seems very feature-rich, eventually in the future things could get a bit more separated, so one thing does not require the existence of another.