-
Notifications
You must be signed in to change notification settings - Fork 4
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
ANDROID-13871 Add screenshot tests for all types of buttons #308
ANDROID-13871 Add screenshot tests for all types of buttons #308
Conversation
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
<?xml version="1.0" encoding="utf-8"?> | ||
<resources> | ||
|
||
<attr name="defaultButtonTest_test" format="reference"/> |
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 need a reference of type R.attr.xxx to init the button programmatically with this style (Button(activity, null, R.attr.xxx)). Maybe there is a simpler way
android:label="Text Input Activity" | ||
android:exported="true" | ||
android:theme="@style/MisticaTheme.Movistar"> | ||
android:label="Activity for testing Mistica xml elements" |
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.
Yup, much better
fun getAllButtonStyles() = listOf( | ||
ButtonStyle.PRIMARY, ButtonStyle.PRIMARY_SMALL, ButtonStyle.SECONDARY, ButtonStyle.SECONDARY_SMALL, ButtonStyle.DANGER, | ||
ButtonStyle.DANGER_SMALL, ButtonStyle.LINK, ButtonStyle.PRIMARY_INVERSE, ButtonStyle.PRIMARY_SMALL_INVERSE, ButtonStyle.SECONDARY_INVERSE, | ||
ButtonStyle.SECONDARY_SMALL_INVERSE, ButtonStyle.LINK_INVERSE | ||
) |
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 think this should be the same as: ButtonStyle.values().toList()
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.
Done
"dark".takeIf { darkTheme } | ||
).joinToString(separator = "_") | ||
|
||
return """screenshots/$nonNullParams""".plus(".png").replace("\\s+".toRegex(), "") |
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 see no need in here for the multiline string in here and also to add the png out of the string template:
return "screenshots/$nonNullParams.png".replace("\\s+".toRegex(), "")
By the way, nice usage of listOfNotNull(), I like that 🚀
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.
Done
private fun `then screenshot is OK`(brand: Brand, darkTheme: Boolean) { | ||
compareScreenshot(composeTestRule.onRoot(), brand, darkTheme) | ||
private fun `then screenshot is OK`(brand: Brand, style: ButtonStyle, icon: Boolean, darkTheme: Boolean) { | ||
val iconSuffix = if(icon) { |
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.
jepLint: a space is needed before the rounded brackets: if (icon) {
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.
Done
fun getStyleResFromButtonStyle(style: ButtonStyle) = when(style) { | ||
ButtonStyle.PRIMARY -> R.style.AppTheme_Button_DefaultButton | ||
ButtonStyle.PRIMARY_SMALL -> R.style.AppTheme_Button_DefaultButton_Small | ||
ButtonStyle.SECONDARY -> R.style.AppTheme_Button_SecondaryButton | ||
ButtonStyle.SECONDARY_SMALL -> R.style.AppTheme_Button_SecondaryButton_Small | ||
ButtonStyle.DANGER -> R.style.AppTheme_Button_DangerButton | ||
ButtonStyle.DANGER_SMALL -> R.style.AppTheme_Button_DangerButton_Small | ||
ButtonStyle.LINK -> R.style.AppTheme_Button_LinkButton | ||
ButtonStyle.PRIMARY_INVERSE -> R.style.AppTheme_Button_PrimaryButtonInverse | ||
ButtonStyle.PRIMARY_SMALL_INVERSE -> R.style.AppTheme_Button_PrimaryButtonInverse_Small | ||
ButtonStyle.SECONDARY_INVERSE -> R.style.AppTheme_Button_SecondaryButtonInverse | ||
ButtonStyle.SECONDARY_SMALL_INVERSE -> R.style.AppTheme_Button_SecondaryButtonInverse_Small | ||
ButtonStyle.LINK_INVERSE -> R.style.AppTheme_Button_LinkButtonInverse | ||
} |
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 would consider doing this as an extension funciton:
fun ButtonStyle.getStyleResFromButtonStyle() = when(this) {
Where is this used? It could be removed in case it's not used.
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.
It was not used. Removed!
@ParameterizedRobolectricTestRunner.Parameters(name = "ButtonXML {1} {0} icon={2}") | ||
fun brands(): List<Array<Any>> { | ||
val allBrands = getAllBrands() | ||
val buttonStyles = getAllButtonStyles() |
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.
or: val buttonStyles = ButtonStyle.values().toList()
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.
Done
button.setIconResource(android.R.drawable.ic_lock_power_off) | ||
iconSuffix = "icon" | ||
} | ||
if (style.isInverse()) { |
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.
Set background color for inverse buttons like in Catalog app to avoid white screenshots (white button over white background)
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.
Some images from the baseline probably will have to be updated after this change.
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
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.
Nice job. I would have left the button xml out of this as they are supposed to be removed some day but I guess we can remove all as well then.
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.
awesome job 👏
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
📱 New catalog for testing generated: Download |
@Test | ||
fun `check the button screenshot`() { |
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 thinking, we might want to also test a button with a very large text in it (with and without icon) but these 2 tests not by brand as I don't think having it for each brand gives more value. And perhaps also not needed in light and dark mode. What do you think about this? Should we also check multiline text?
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.
Maybe for a future PR
🥅 What's the goal?
DONT PANIC 493 files are just updated pngs
Add screenshot tests for all types of Mistica Buttons
🚧 How do we do it?
Parameterized tests with all combinations of brand, button style, has icon and dark theme. These tests have been done for both xml and compose buttons
☑️ Checks
🧪 How can I test this?