ANDROID-13796 Roborazzi screenshot tests #296

jeprubio · 2023-08-28T09:57:49Z

🥅 What's the goal?

Add screenshot testing in mística: POC Roborazzi tests -> Jira Task

🚧 How do we do it?

Adding Roboelectric
Adding Roborazzi following the README instructions.

What is missing:

Perhaps we want to store the screenshots inside the codebase instead of what they do for demonstration, we could check if passing the fileName is ok or if we have other problems then.
Github integration, there is https://github.com/takahirom/roborazzi-compare-on-github-comment-sample we could play with it and check if that's ok or do we need something different.

☑️ Checks

I updated the documentation, including readmes and wikis. If this is a breaking change, update UPGRADING.md to inform users how to proceed. If no updates are necessary, indicate so.
Tested with dark mode.
Tested with API 23.

🧪 How can I test this?

The test is being run on each commit to this pr for now (Compare Screenshots action), we should be able to run it also on demand once in integration as it has the workflow_dispatch as well (apart from being run on each pr).

… tests

gmerinojimenez · 2023-10-11T10:34:26Z

build.gradle

@@ -27,6 +28,7 @@ buildscript {
 plugins {
    id 'org.jetbrains.kotlin.android' version '1.5.21' apply false
    id 'io.github.gradle-nexus.publish-plugin' version '1.1.0' apply false
+    id "io.github.takahirom.roborazzi" version '1.5.0' apply false


just to be sure, the plugin and the library use different versions?

ups, no, good catch, I should have used 1.4.0 in here 0e78b6e As in 1.5.0 I saw that roborazzi also creates compare.png images when there is no difference and that would break the report we do.

Luckily the lib is going to add it's own html reports so we won't have that problem in future releases from 1.7.0 on.

gmerinojimenez · 2023-10-11T10:37:17Z

library/src/test/java/com/telefonica/mistica/testutils/ScreenshotsTest.kt

+@Config(qualifiers = RobolectricDeviceQualifiers.Pixel5)
+open class ScreenshotsTest {
+    fun compareScreenshot(
+        node: SemanticsNodeInteraction,
+        brand: Brand? = null,
+        darkTheme: Boolean = false,
+        extra: String = "",
+    ) {
+        node.captureRoboImage(screenshotName(brand, darkTheme, extra))
+    }
+
+    fun compareScreenshot(
+        node: ViewInteraction,
+        brand: Brand? = null,
+        darkTheme: Boolean = false,
+        extra: String = "",
+    ) {
+        node.captureRoboImage(screenshotName(brand, darkTheme, extra))
+    }
+
+    private fun screenshotName(brand: Brand?, darkTheme: Boolean, extra: String) = ScreenshotUtils.getScreenshotName(
+        brand = brand,
+        extra = if (darkTheme) "_dark$extra" else extra
+    )
+}


totally personal preference, but since those methods doesn't require anything from the class I would move them to pure functions to have as less inheritance as possible

Which ones? The compareScreenshot() ones I like them to be here as it forces the test to inherit from ScreenshotsTest in order to see them and this way we are applying the global config qualifiers. If they weren't here we would probably forget to extend from ScreenshotsTest and that qualifiers would not be applied to that tests so it would be more human error prone. In other words, as it's done now we avoid many possible errors that we could have if moved to pure functions. I didn't want inheritance, I was asked to do that before for the config annotation.

in my opinion there are two things: on one side is the qualifier, that force us to inherit and in the other the base methods. I don't have a strong opinion, so do what you prefer, but I don't think it is needed to add methods to this class (and couple the child tests to it) VS avoiding the inheritance.

Actually, if you ask me I would completely remove the ScreenshotsTest base class and instead add the qualifier to each test, like we do with some other tests like the acceptance.

Agree, but then we'll be back to the initial code in which I was asked not to add the qualifier to each test: #296 (comment) 🤔

Exactly... I'm not a fan of inheritance per se but here I see the same benefits we got from using a base class for unit tests in https://github.com/Telefonica/android-messenger/blob/integration/test-commons/src/main/java/com/tuenti/messenger/test/UnitTest.java to avoid repeating code in tests over and over again (and in for roborazzi, also possible errors to be introduced if we forget to set the qualifier or use a different one)

Yup, in the end we agreed on leaving this base class for screenshot tests to avoid repeating code at least for now unless at some point we find another way to set the default qualifier and then this can be updated.

pmartinbTEF · 2023-10-11T11:14:17Z

library/screenshots/check_TextInput_from_layout_1.png

Is it a good idea to group several elements in one layout? Instead of doing parameterized test of each UI element like the Button example? I think it should be unified

In my opinion it's good offering multiple options and multiple ways to do this. In here we can use the layout from catalog as it has the different inputs and just use that instead of having to recreate the different TextInput states one by one and checking them separately, which would work as well. I'm just leaving more options to the ones creating the tests. With this way we could have mistica tested quite quickly and then decide wether it's worth doing it one by one or just doing it when a component is updated... Wdyt?

But perhaps we could move this to the catalog, I'm going to try that and check how it looks. I'll be back.

I've removed the tests of the layout from the pr, we might want to reintroduce them in the catalog, or not. But I'm leaving them out of the scope for this pr 7416b62

gmerinojimenez

Nice job!

nimeacuerdo

Get the ball rolling!

pmartinbTEF

Great job!

nimeacuerdo · 2023-10-13T08:34:14Z

library/src/test/java/com/telefonica/mistica/testutils/ScreenshotsTest.kt

@@ -7,7 +7,7 @@ import com.github.takahirom.roborazzi.captureRoboImage
 import com.telefonica.mistica.compose.theme.brand.Brand
 import org.robolectric.annotation.Config

-@Config(qualifiers = RobolectricDeviceQualifiers.Pixel5)
+@Config(sdk = [33], qualifiers = RobolectricDeviceQualifiers.Pixel5)


why is the sdk needed here? what happens if we do not specify it?

It's not really needed, but it seems it's good to specify the sdk in case you want to be able to create the baseline from a different machine than the ci. I've read it might happen than the images not match on the ci if this sdk is not specified, specially if the baseline is created on a different OS than what the CI uses.

This is just the global config value which my understanding is that it can be overridden in the specific tests we want to.

ok, +100 to have anything that could be affected by differences in the CI vs local environment specified in the config in order to prevent unexpected errors not related to actual bugs.

github-actions · 2023-10-13T10:11:51Z

📱 New catalog for testing generated: Download

haynlo

Great job!! 🚀

jeprubio added 2 commits August 28, 2023 09:07

POC roborazzi screenshot tests

303b648

POC roborazzi set screenshot folder

260a6ed

Telefonica deleted a comment from github-actions bot Sep 1, 2023

jeprubio and others added 27 commits September 4, 2023 10:23

POC roborazzi using getScreenshotName() to get the screenshot file name

849960c

POC roborazzi add compare_screenshots.yml

595750e

POC roborazzi updating failure report

53865e1

Merge branch 'main' into poc/roborazzi

f97a966

POC roborazzi buttons test

9745034

POC roborazzi update screenshots baseline

c050235

POC roborazzi execute update_screenshot_baseline.yml on push

4155351

Updated screenshots baseline

db7ff60

POC roborazzi update record screenshots baseline

8db018d

POC roborazzi runs on ubuntu-latest

9734388

Updated screenshots baseline

a84e7c4

POC roborazzi remove on push

0e92faa

Merge remote-tracking branch 'origin/poc/roborazzi' into poc/roborazzi

e1520b2

POC roborazzi upload to azure

c0e890c

POC roborazzi upload to azure if failure

07ecb8f

POC roborazzi updated upload to azure

3e586c1

POC roborazzi updated upload to azure

693f71c

POC roborazzi force screenshots error

434bb80

POC roborazzi add pngs of other directories

99f2239

POC roborazzi add pngs of other directories

f5114d3

POC roborazzi revert brand names

027f5bb

POC roborazzi update baseline

a13ee3a

POC roborazzi update baseline

64ead38

POC roborazzi update baseline

adffa31

POC roborazzi using Enclosed to run parametrized and not parametrized…

f04c8d3

… tests

POC roborazzi update screenshots

026d865

POC roborazzi update screenshots

3ad5a71

gmerinojimenez reviewed Oct 11, 2023

View reviewed changes

ANDROID-13796 downgrade roborazzi to 1.4.0

0e78b6e

pmartinbTEF reviewed Oct 11, 2023

View reviewed changes

jeprubio added 2 commits October 11, 2023 13:22

ANDROID-13796 add generate-html-report action

7fb6fc6

ANDROID-13796 add gradle/gradle-build-action

e905f27

Telefonica deleted a comment from github-actions bot Oct 11, 2023

gmerinojimenez approved these changes Oct 11, 2023

View reviewed changes

nimeacuerdo approved these changes Oct 11, 2023

View reviewed changes

ANDROID-13796 removing the layout tests from library

7416b62

Telefonica deleted a comment from github-actions bot Oct 11, 2023

ANDROID-13796 add TextInputWithError test

fdb6337

Telefonica deleted a comment from github-actions bot Oct 11, 2023

jeprubio requested a review from pmartinbTEF October 11, 2023 15:00

ANDROID-13796 add TextInputDisabled test

69e609f

pmartinbTEF approved these changes Oct 11, 2023

View reviewed changes

jeprubio added 2 commits October 13, 2023 09:49

ANDROID-13796 remove unneeded code

b217f04

ANDROID-13796 configure sdk version

af15dd0

Telefonica deleted a comment from github-actions bot Oct 13, 2023

nimeacuerdo reviewed Oct 13, 2023

View reviewed changes

Telefonica deleted a comment from github-actions bot Oct 13, 2023

jeprubio added 2 commits October 13, 2023 11:52

ANDROID-13796 extract repeated code to private method

601dca0

ANDROID-13796 add dark mode tests for classic views

c24fc2a

Telefonica deleted a comment from github-actions bot Oct 13, 2023

haynlo approved these changes Oct 16, 2023

View reviewed changes

jeprubio merged commit b043c2f into main Oct 16, 2023
5 checks passed

jeprubio deleted the poc/roborazzi branch October 16, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ANDROID-13796 Roborazzi screenshot tests #296

ANDROID-13796 Roborazzi screenshot tests #296

jeprubio commented Aug 28, 2023 •

edited

Loading

gmerinojimenez Oct 11, 2023

jeprubio Oct 11, 2023 •

edited

Loading

gmerinojimenez Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

gmerinojimenez Oct 11, 2023

jeprubio Oct 11, 2023 •

edited

Loading

nimeacuerdo Oct 11, 2023

jeprubio Oct 11, 2023

pmartinbTEF Oct 11, 2023

jeprubio Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

gmerinojimenez left a comment

nimeacuerdo left a comment

pmartinbTEF left a comment

nimeacuerdo Oct 13, 2023

jeprubio Oct 13, 2023 •

edited

Loading

nimeacuerdo Oct 13, 2023

github-actions bot commented Oct 13, 2023

haynlo left a comment

ANDROID-13796 Roborazzi screenshot tests #296

ANDROID-13796 Roborazzi screenshot tests #296

Conversation

jeprubio commented Aug 28, 2023 • edited Loading

🥅 What's the goal?

🚧 How do we do it?

What is missing:

☑️ Checks

🧪 How can I test this?

gmerinojimenez Oct 11, 2023

Choose a reason for hiding this comment

jeprubio Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

gmerinojimenez Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jeprubio Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

gmerinojimenez Oct 11, 2023

Choose a reason for hiding this comment

jeprubio Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

nimeacuerdo Oct 11, 2023

Choose a reason for hiding this comment

jeprubio Oct 11, 2023

Choose a reason for hiding this comment

pmartinbTEF Oct 11, 2023

Choose a reason for hiding this comment

jeprubio Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jeprubio Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

gmerinojimenez left a comment

Choose a reason for hiding this comment

nimeacuerdo left a comment

Choose a reason for hiding this comment

pmartinbTEF left a comment

Choose a reason for hiding this comment

nimeacuerdo Oct 13, 2023

Choose a reason for hiding this comment

jeprubio Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

nimeacuerdo Oct 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 13, 2023

haynlo left a comment

Choose a reason for hiding this comment

jeprubio commented Aug 28, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

gmerinojimenez Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

jeprubio Oct 11, 2023 •

edited

Loading

jeprubio Oct 13, 2023 •

edited

Loading