Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add localInspectionMode property to RoborazziComposeOptions #588

Merged
merged 4 commits into from
Dec 12, 2024
Merged

add localInspectionMode property to RoborazziComposeOptions #588

merged 4 commits into from
Dec 12, 2024
+33 −1

Conversation

YusukeMoriJapan
Copy link
Contributor

@YusukeMoriJapan YusukeMoriJapan commented Dec 5, 2024 •
edited
Loading

fix this issue #589

@YusukeMoriJapan YusukeMoriJapan force-pushed the feature/local-inspection-true branch from 7ca46e1 to f62d182 Compare December 5, 2024 07:59
@YusukeMoriJapan YusukeMoriJapan mentioned this pull request Dec 5, 2024
Closed
@YusukeMoriJapan YusukeMoriJapan marked this pull request as ready for review December 5, 2024 08:00
YusukeMoriJapan
YusukeMoriJapan commented Dec 5, 2024
View reviewed changes
...anner-support/src/main/java/com/github/takahirom/roborazzi/RoborazziPreviewScannerSupport.kt Outdated
Comment on lines 21 to 22
composablePreview()
CompositionLocalProvider(LocalInspectionMode provides true) {
composablePreview()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running the Preview Screenshot Test, it is normally expected to produce the same result as during the Preview rendering, so there is no option to choose between true or false. If there is a need to make it selectable, I will modify it accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@takahirom
I’m actually thinking whether it’d make sense to provide this option also via gradle plugin?
Maybe in a future version.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have something like roborazz.compose.inspectionmode = true. The default would be null. However, this might be a little confusing because we can't inject the setting if you use your activity.

Copy link
Contributor

@sergio-sastre sergio-sastre Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then better to leave it as ComposeOption only

@takahirom
Copy link
Owner

takahirom commented Dec 5, 2024 •
edited
Loading

Thanks. I think there is a trade-off when we use LocalInspectionMode provides true. It is important to maintain fidelity (how close it is to production) in tests, and if we use it, the code we run will be altered. Therefore, I don't want to make it the default option. How about adding a RoborazziComposeConfig that enables and disables the inspection mode so users can configure it?

@YusukeMoriJapan
Copy link
Contributor Author

Thanks. I think there is a trade-off when we use LocalInspectionMode provides true. It is important to maintain fidelity (how close it is to production) in tests, and if we use it, the code we run will be altered. Therefore, I don't want to make it the default option. How about adding a RoborazziComposeConfig that enables and disables the inspection mode so users can configure it?

OK👍
I will try it now.

@YusukeMoriJapan YusukeMoriJapan changed the title Fix Compose Preview screenshot tests, rendering fails add localInspectionMode property to RoborazziOptions Dec 5, 2024
@YusukeMoriJapan YusukeMoriJapan force-pushed the feature/local-inspection-true branch from 16a5061 to f62d182 Compare December 5, 2024 09:45
@takahirom
Copy link
Owner

Sorry, I intended to add the configuration to RoborazziComposeConfig instead of RoborazziOptions. RoborazziOptions is a option for all images not for Compose. RoborazziComposeConfig is a new feature that hasn’t been released yet, and the API is still under consideration.

I suggest rebasing this PR onto the 12-05-refactor_roborazzicomposeconfigbuilder branch to use the new RoborazziComposeConfig.Builder.

@YusukeMoriJapan
Copy link
Contributor Author

Sorry, I intended to add the configuration to RoborazziComposeConfig instead of RoborazziOptions. RoborazziOptions is a option for all images not for Compose. RoborazziComposeConfig is a new feature that hasn’t been released yet, and the API is still under consideration.

I suggest rebasing this PR onto the 12-05-refactor_roborazzicomposeconfigbuilder branch to use the new RoborazziComposeConfig.Builder.

I just realized the same thing. Thank you for letting me know.
I will rebase and try again.

@takahirom
Copy link
Owner

Sorry, I'm going to change the APIs a little bit more. The timing is a bit unfortunate 🙇. I suggest you try this tomorrow.

@YusukeMoriJapan YusukeMoriJapan changed the title add localInspectionMode property to RoborazziOptions Fix Compose Preview screenshot tests, rendering fails Dec 5, 2024
@takahirom
Copy link
Owner

Thank you for your patience. I believe you can work on this now.

@YusukeMoriJapan YusukeMoriJapan marked this pull request as draft December 9, 2024 05:53
@YusukeMoriJapan YusukeMoriJapan force-pushed the feature/local-inspection-true branch from f62d182 to 2f972ac Compare December 11, 2024 14:52
@YusukeMoriJapan YusukeMoriJapan changed the title Fix Compose Preview screenshot tests, rendering fails add localInspectionMode property to RoborazziComposeOptions Dec 11, 2024
@YusukeMoriJapan YusukeMoriJapan marked this pull request as ready for review December 11, 2024 14:53
YusukeMoriJapan
YusukeMoriJapan commented Dec 11, 2024
View reviewed changes
roborazzi-compose/src/main/java/com/github/takahirom/roborazzi/RoborazziComposeOptions.kt
Comment on lines +253 to +263
data class RoborazziComposeLocalInspectionModeOption(private val localInspectionMode: Boolean) :
RoborazziComposeComposableOption {
override fun configureWithComposable(
content: @Composable () -> Unit
): @Composable () -> Unit = {
CompositionLocalProvider(LocalInspectionMode provides localInspectionMode) {
content()
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@takahirom
I added the option to configure localInspectionMode in RoborazziComposeOptions.

takahirom
takahirom reviewed Dec 12, 2024
View reviewed changes
sample-android/src/test/java/com/github/takahirom/roborazzi/sample/ManualTest.kt Outdated
Comment on lines 181 to 183
/* The default value is false, but we can set it to true,
if you want to use the logic for Preview in composable functions. */
localInspectionMode(false)
Copy link
Owner

@takahirom takahirom Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I don't think we need to apply localInspectionMode explicitly. How about making it a comment?

Suggested change
/* The default value is false, but we can set it to true,
if you want to use the logic for Preview in composable functions. */
localInspectionMode(false)
/*
We don't specify `localInspectionMode` by default. The default value for `localInspectionMode` in Compose is `false`. This is to maintain higher fidelity in tests.
If you encounter issues integrating the library, you can set `localInspectionMode` to `true`.
localInspectionMode(true)
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@takahirom
OK, I have incorporated your feedback into the comment.
4f6007a

takahirom
takahirom approved these changes Dec 12, 2024
View reviewed changes
Copy link
Owner

@takahirom takahirom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you for your contribution!

@takahirom takahirom merged commit 02590f2 into takahirom:main Dec 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergio-sastre sergio-sastre sergio-sastre left review comments

@takahirom takahirom takahirom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@YusukeMoriJapan @takahirom @sergio-sastre