-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support DelayedPreviews (not tested, just initial push) #633
base: main
Are you sure you want to change the base?
Conversation
👀 |
The build failed because of a setting in the root build.gradle file:
I've addressed this by excluding the "roborazzi-annotations" module from this setting. |
I understand this is difficult, especially regarding ComposeRule. I'm exploring solutions. Please bear with me. |
@sergio-sastre |
I think we might have multiple activities for each capture. I'm looking a little further into this. |
Great work! |
Thanks! I'm considering whether the name "DelayedPreview" is appropriate. |
I've pushed a preview with the DelayedPreview annotation and the current implementation does not seem to work. |
@@ -143,18 +197,28 @@ interface ComposePreviewTester<T : Any> { | |||
|
|||
@ExperimentalRoborazziApi | |||
class AndroidComposePreviewTester : ComposePreviewTester<AndroidPreviewInfo> { | |||
private val composeTestRule by lazy { createAndroidComposeRule<RoborazziActivity>() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takahirom
Actually, it does not matter whether lazy or not since it is called in EVERY test.
I was afraid that using this TestRule in every test would increase the execution time, but I haven't noticed any significant increase though... I believe that is different from Activity Scenario, which launches an Activity and that does add execution time indeed...
It could also be that my laptop (Mac M2) is very fast and the amount of preview tests is not that high (less than 15...). I'm unsure about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe test isolation is important. I think it's good to have a separate Activity for each test, as we do now. Of course, it depends on how slow it is, but I'm fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that too. However, even with this lazy, the ComposeTestRule is still being created in EVERY test, because it is called in other parts of the AndroidPreviewTester ( I’ve debugged it).
I’d also like it to be created when necessary … any ideas on how to avoid that?
@SuppressLint("VisibleForTests") | ||
val viewRootForTest = composeView.getChildAt(0) as ViewRootForTest | ||
doBeforeCapture() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takahirom
I'm open to other solutions, but it does have to happen at this point. That was the reason why it was not working previously: the advanceTimeBy()
was being before the Composable was set as content in the Activity
// TODO -> maybe add also parameter for ignoreFrames, as used in mainClock.advanceTime() | ||
// TODO -> Make BINARY annotation only applicable to Methods | ||
// TODO -> Docu: mention about the 16ms frame in Android | ||
annotation class DelayCaptureRoboImage(val delayInMillis: Long) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takahirom
Feel free to give feedback on the name ;)
I'm thinking whether it'd also make sense to add the capability to take screenshots after a set of time intervals... what could be interesting to verify animations.
I think in that case, we could create a different annotation for that like
IntervalCaptureRoboImage(intervalsInMillis: arrayOf(500, 100, 200))
If you like the idea, we could make it either in this PR or afterwards, up to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think the name CaptureRoboImage
is a little misleading when used for DelayCaptureRoboImage
, because we filter Previews by @Preview
and not by @DelayCaptureRoboImage
. So I've renamed it to RoboManualAdvance
.
As for IntervalCaptureRoboImage
, could we perhaps have a repeatable annotation?
When considering IntervalCaptureRoboImage
, the use of CaptureRoboImage
name is understandable though.
@RoboManualAdvance(advanceTimeMillis = 100)
@RoboManualAdvance(advanceTimeMillis = 200)
@RoboManualAdvance(advanceTimeMillis = 500)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
I am also not very convinced about RoboManualAdvance, I think it does not tell its purpose clearly.
Its purpose is to advance the clock before capturing a screenshot.
I think something similar to
AdvanceClockBeforeCapture
or the like makes more sense.
as of repeatable annotations, ComposablePreviewScanner still does not support them as stated in the corresponding method:
That lies on me since it is a bit tricky 😅.
However, I agree that is a better solution.
I can create an issue and try to address it to support that in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I‘m planning to release support for repeated annotations in ComposablePreviewScanner 0.5.1.
However, what would be your expectation?
Would „duplicate“ previews for each repeated annotation, or would be more like a getRepeatedAnnotation()
and get a list with all annotations?
I‘m more inclined to the last one, but I‘m wondering how we could integrate it in this case 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It might be good to consider when we should add another annotation.
@RoboManualAdvance(advanceTimeMillis = 100)
@RoboManualAdvance(advanceTimeMillis = 200)
@RoboFoo(true)
@RoboFoo(false)
We might want to take a screenshot when RoboFoo
is true and RoboManualAdvance
is 100. So it might be good to have one annotation like @RoboComposeOption()
? For example, we could use @RoboComposeOption(autoAdvance = false, advanceTimeMillis = 100, foo = true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I was able to implement a workaround.
This is unrelated, but one area I'd like to investigate is whether we can incorporate users' custom ComposeRule or similar components in this implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the options()
override from AndroidComposePreviewTester and added ComposeRuleFactory to JUnit4TestLifecycleOptions. While this approach helps address the challenges of integrating custom ComposeRules, it introduces significant breaking changes that users need to be aware of during migration.
2056650
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is whether the KMP Preview support is currently functioning properly. As we currently lack test cases, I would like to establish comprehensive test cases to verify its functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added KMP Preview support sample, and it works great.
However, I believe we need to significantly refactor this code. The main issue is that we currently cannot capture multiple screenshots using different manualClockOptions because captureRoboImage{} closes the ActivityScenario.
I suggest creating separate tests for each screenshot to make the separation clearer, which would also require introducing a breaking change to the previews() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it and will review my code. 👀
1d3cade
… for one preview function
f93e261
to
ae1b052
Compare
This is going to be a big release for users using Compose Preview Support. I'm thinking releasing this as 1.44.0-alpha01 to gather feedback. Breaking changes for Experimental Compose Preview Support usersKey interface changes
Migration steps
// Before
class CustomTester : ComposePreviewTester<AndroidPreviewInfo> {
override fun previews(): ComposablePreview<AndroidPreviewInfo> = AndroidComposablePreviewScanner().scanPackageTrees(...)
override fun test(preview: ComposablePreview<AndroidPreviewInfo>)
}
// After
class CustomTester : ComposePreviewTester<AndroidPreviewJUnit4TestParameter> {
override fun testParameters(): AndroidPreviewJUnit4TestParameter = AndroidComposablePreviewScanner().scanPackageTrees(...)
override fun test(preview: AndroidPreviewJUnit4TestParameter)
}
implementation("io.github.takahirom.roborazzi:roborazzi-annotations:")
@RoboComposePreviewOptions(
manualClockOptions = [ManualClockOptions(advanceTimeMillis = 500L)]
)
@Preview
@Composable
fun DelayedPreview() {
// Content with animations/timed state changes
} Sample implementations[TODO: Update link]
These changes enable annotation-driven screenshot control while maintaining backward compatibility for most preview configurations. |
No description provided.