-
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-13796 Roborazzi screenshot tests #296
Changes from 73 commits
303b648
260a6ed
849960c
595750e
53865e1
f97a966
9745034
c050235
4155351
db7ff60
8db018d
9734388
a84e7c4
0e92faa
e1520b2
c0e890c
07ecb8f
3e586c1
693f71c
434bb80
99f2239
f5114d3
027f5bb
a13ee3a
64ead38
adffa31
f04c8d3
026d865
3ad5a71
d6ed913
9573d41
bc19b0c
b42a4af
bc2df7b
5b73d35
a363339
84a264f
e1ab44c
61bc591
11ee4d8
7e730f0
07e60ae
4d3342c
4d385b4
79b3ca2
349fcbc
e39ae82
af575ee
f31a83e
92ce41f
6da54d3
b9cc05a
fbbd271
ca6efc5
d0488c6
c0daf65
83aeb97
e93c2da
5528abd
9d5c5fb
593539f
7599217
da6d038
8ef2191
626f9dc
9fd7417
363798d
7a12e7c
36b3aea
a19421e
d193e19
1f4193e
059380c
0e78b6e
7fb6fc6
e905f27
7416b62
fdb6337
69e609f
b217f04
af15dd0
601dca0
c24fc2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
name: Compare Screenshots | ||
|
||
on: | ||
workflow_dispatch: | ||
pull_request: | ||
|
||
jobs: | ||
CompareScreenshots: | ||
|
||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Checkout repo | ||
uses: actions/checkout@v3 | ||
|
||
- name: Verify Screenshots (roborazzi) | ||
run: 'bash ./gradlew verifyRoborazziDebug' | ||
|
||
- id: generate-html | ||
name: Generate Html Report | ||
if: failure() | ||
env: | ||
BRANCH_NAME: companion_${{ github.event.workflow_run.head_branch }} | ||
shell: bash | ||
run: | | ||
mkdir reports | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can wrap all this script in a separated file and invoke it from here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, we could, I thought about this, but then I thought as the roborazzi lib is adding it's own html reports we could use them once that version is stable and we update and then remove this script. So I thought it's not worth moving it if it has to be removed later on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use a custom action for it. It takes 5 min to create it. See https://github.com/Telefonica/novum-platform-android/tree/main/.github/actions/boot-emulator as an example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even if it is going to be deleted, it doesn't feel clean to have a html in this main action file. We can wrap it inside a separated script or custom action as Guille said, invoke it from this main file and add a comment that this html generator execution is temporary and will be removed as soon as Roborazzi support is stable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, done 7fb6fc6 and checked in https://github.com/Telefonica/mistica-android/actions/runs/6481800149 |
||
touch reports/report.html | ||
cp */screenshots/*_compare.png reports/ | ||
files=$(find . -type f -name "*_compare.png" | grep "reports/") | ||
{ | ||
echo '<!doctype html>' | ||
echo '<html>' | ||
echo '<head>' | ||
echo '<title>Screenshots failure report</title>' | ||
echo '<link href="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.100.2/css/materialize.min.css" rel="stylesheet">' | ||
echo '<link href="https://fonts.googleapis.com/icon?family=Material+Icons" rel="stylesheet">' | ||
echo '<meta charset="UTF-8">' | ||
echo '</head>' | ||
echo '<style>' | ||
echo 'body {' | ||
echo 'display: flex;' | ||
echo 'min-height: 100vh;' | ||
echo 'flex-direction: column;' | ||
echo '}' | ||
echo 'main {' | ||
echo 'flex: 1 0 auto;' | ||
echo '}' | ||
echo '</style>' | ||
echo '<body>' | ||
echo '<nav>' | ||
echo '<div class="nav-wrapper indigo darken-3">' | ||
echo '<a href="#" class="brand-logo left">Screenshots failure report</a>' | ||
echo '<ul id="nav-mobile" class="right hide-on-med-and-down">' | ||
echo '<li><a href="https://github.com/takahirom/roborazzi">Roborazzi</a></li>' | ||
echo '</ul>' | ||
echo '</div>' | ||
echo '</nav>' | ||
echo '<main class="container">' | ||
echo '<div class="section">' | ||
echo '<table class="highlight responsive-table">' | ||
echo '<tr><th>File name</th><th>Comparison</th></tr>' | ||
} >> reports/report.html | ||
|
||
for file in $files; do | ||
# Get the file name and insert newlines every 100 characters | ||
fileName=$(basename "$file" | sed -r 's/(.{100})/\1<br>/g') | ||
echo "<tr><td>$(basename "$file")</td>" >> reports/report.html | ||
echo "<td><a href=\"$(basename "$file")\"><img src=\"$(basename "$file")\" width=\"100%\" height=\"100%\" /></a></td></tr>" >> reports/report.html | ||
done | ||
{ | ||
echo '</table>' | ||
echo '</div>' | ||
echo '</main>' | ||
echo '<footer class="page-footer indigo darken-3">' | ||
echo '<div></div>' | ||
echo '</footer>' | ||
echo '<script src="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.100.2/js/materialize.min.js"></script>' | ||
echo '</body></html>' | ||
} >> reports/report.html | ||
|
||
echo "Report: " | ||
cat reports/report.html | ||
|
||
- name: Generate screenshots tests reports tar.gz | ||
if: failure() | ||
run: | | ||
tar cvzf mistica-screenshots-tests-report.tar.gz reports || echo "No screenshots tests reports found" | ||
shell: bash | ||
|
||
- name: Checkout Telefonica/github-actions repo | ||
if: failure() | ||
uses: actions/checkout@v3 | ||
with: | ||
repository: Telefonica/github-actions | ||
token: "${{ secrets.NOVUM_PRIVATE_REPOS }}" | ||
path: .github/shared-actions | ||
|
||
- name: Upload reports to azure | ||
if: failure() | ||
uses: ./.github/shared-actions/azure/upload-to-storage | ||
with: | ||
azure-account-name: ${{secrets.AZURE_ACCOUNT_NAME}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will obfuscate the url of the screenshots report. Could it be clear text? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, this is the cause we have urls in the reports with the *** as for example: https://***.blob.core.windows.net/ci-container-1696848766116/mistica-screenshots-tests-report.tar.gz There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly, that's what we have secrets for |
||
azure-account-key: ${{secrets.AZURE_ACCOUNT_KEY}} | ||
globs: | | ||
mistica-screenshots-tests-report.tar.gz | ||
generate-summary: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
name: "Update screenshot baseline" | ||
on: | ||
workflow_dispatch: | ||
|
||
jobs: | ||
screenshots_baseline: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout repo | ||
uses: actions/checkout@v2 | ||
|
||
- name: Run Roborazzi Record | ||
run: 'bash ./gradlew clean recordRoborazziDebug' | ||
|
||
- name: Check Git status | ||
run: 'git status' | ||
shell: bash | ||
Comment on lines
+15
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was copied from https://github.com/Telefonica/niji-for-home-android/blob/master/.github/workflows/run_acceptance_tests.yml as we thought this was necessary for the azure upload There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it justs prints git status info to the console, I would get rid of this here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, yup, then it makes sense, removed 626f9dc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that is removed the screenshots are not uploaded to azure and it prints:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
- name: Commit and push screenshots baseline | ||
id: commitAndPushScreenshotsBaseline | ||
uses: EndBug/add-and-commit@v7 | ||
with: | ||
message: 'Updated screenshots baseline' | ||
add: './**/screenshots/*' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ buildscript { | |
accompanist_version = "0.30.1" | ||
coil_version = '2.2.2' | ||
constraintComposeVersion = '1.0.1' | ||
roborazzi_version = "1.4.0" | ||
} | ||
repositories { | ||
google() | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to be sure, the plugin and the library use different versions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
allprojects { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
#Fri May 19 13:07:06 CEST 2023 | ||
distributionBase=GRADLE_USER_HOME | ||
distributionPath=wrapper/dists | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5-bin.zip | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6.2-bin.zip | ||
zipStoreBase=GRADLE_USER_HOME | ||
zipStorePath=wrapper/dists |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
package="com.telefonica.mistica" | ||
> | ||
|
||
<uses-permission android:name="android.permission.VIBRATE" /> | ||
pmartinbTEF marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
<application> | ||
<activity | ||
android:name=".DummyActivity" | ||
android:label="Text Input Activity" | ||
android:exported="true" | ||
android:theme="@style/MisticaTheme.Movistar"> | ||
|
||
<intent-filter> | ||
<action android:name="android.intent.action.MAIN" /> | ||
<category android:name="android.intent.category.LAUNCHER" /> | ||
</intent-filter> | ||
</activity> | ||
</application> | ||
</manifest> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package com.telefonica.mistica | ||
|
||
import android.os.Bundle | ||
import androidx.appcompat.app.AppCompatActivity | ||
import com.telefonica.mistica.R | ||
|
||
class DummyActivity : AppCompatActivity() { | ||
|
||
override fun onCreate(savedInstanceState: Bundle?) { | ||
super.onCreate(savedInstanceState) | ||
setContentView(R.layout.test_dummy_activity) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
package com.telefonica.mistica.compose.button | ||
|
||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.test.junit4.createComposeRule | ||
import androidx.compose.ui.test.onNodeWithText | ||
import androidx.compose.ui.test.performClick | ||
import androidx.compose.ui.unit.dp | ||
import com.telefonica.mistica.compose.theme.MisticaTheme | ||
import com.telefonica.mistica.compose.theme.brand.MovistarBrand | ||
import org.junit.Assert.assertTrue | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.robolectric.RobolectricTestRunner | ||
|
||
@RunWith(RobolectricTestRunner::class) | ||
internal class ButtonBehaviourTest { | ||
@get:Rule | ||
val composeTestRule = createComposeRule() | ||
|
||
@Test | ||
fun `check the button is clicked`() = test { | ||
`given Button`() | ||
|
||
`when the button is clicked`() | ||
|
||
`then the onClickListener has been invoked`() | ||
} | ||
|
||
private fun TestScope.`given Button`() { | ||
`when Button`() | ||
} | ||
|
||
private fun TestScope.`when Button`() { | ||
composeTestRule.setContent { | ||
MisticaTheme(brand = MovistarBrand) { | ||
Button( | ||
text = textValue, | ||
onClickListener = onClickListener, | ||
modifier = Modifier.padding(16.dp) | ||
) | ||
} | ||
} | ||
} | ||
|
||
private fun TestScope.`when the button is clicked`() { | ||
composeTestRule.onNodeWithText(textValue).performClick() | ||
} | ||
|
||
private fun TestScope.`then the onClickListener has been invoked`() { | ||
assertTrue(clicked) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about using a mock instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would work as well and it wouldn't be a big deal but I prefer not to use mock when it's not needed as in here and all the compose ui tests I've seen until now. As my understanding is that using a mock framework adds a little performance overhead. But I'm also not against using mocks in tests like this, I just prefer not using them if possible. |
||
} | ||
|
||
private fun test(block: TestScope.() -> Unit) { | ||
TestScope().block() | ||
} | ||
|
||
private class TestScope { | ||
val textValue = "textValue" | ||
var clicked = false | ||
val onClickListener: () -> Unit = { clicked = true } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package com.telefonica.mistica.compose.button | ||
|
||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.material.Surface | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.test.junit4.createComposeRule | ||
import androidx.compose.ui.test.onRoot | ||
import androidx.compose.ui.unit.dp | ||
import com.telefonica.mistica.compose.theme.MisticaTheme | ||
import com.telefonica.mistica.compose.theme.brand.BlauBrand | ||
import com.telefonica.mistica.compose.theme.brand.Brand | ||
import com.telefonica.mistica.compose.theme.brand.MovistarBrand | ||
import com.telefonica.mistica.compose.theme.brand.O2Brand | ||
import com.telefonica.mistica.compose.theme.brand.TelefonicaBrand | ||
import com.telefonica.mistica.compose.theme.brand.VivoBrand | ||
import com.telefonica.mistica.testutils.ScreenshotsTest | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.robolectric.ParameterizedRobolectricTestRunner | ||
|
||
@RunWith(ParameterizedRobolectricTestRunner::class) | ||
internal class ButtonKtTest(private val brand: Brand, private val darkTheme: Boolean): ScreenshotsTest() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ButtonKtTest is the name android studio gives automatically to this test when done from the IDE, I can rename it to ButtonTest which is what I understand that you mean but then when navigating from the IDE with |
||
@get:Rule | ||
val composeTestRule = createComposeRule() | ||
|
||
@Test | ||
fun `check the button screenshot`() { | ||
`when Button`(brand, darkTheme) | ||
|
||
`then screenshot is OK`(brand, darkTheme) | ||
} | ||
|
||
private fun `when Button`(brand: Brand = MovistarBrand, darkTheme: Boolean) { | ||
composeTestRule.setContent { | ||
MisticaTheme(brand = brand, darkTheme = darkTheme) { | ||
Surface { | ||
Button( | ||
text = "textValue", | ||
onClickListener = { }, | ||
modifier = Modifier.padding(16.dp) | ||
) | ||
} | ||
} | ||
} | ||
} | ||
|
||
private fun `then screenshot is OK`(brand: Brand, darkTheme: Boolean) { | ||
compareScreenshot(composeTestRule.onRoot(), brand, darkTheme) | ||
} | ||
|
||
companion object { | ||
@JvmStatic | ||
@ParameterizedRobolectricTestRunner.Parameters(name = "Input: {0}") | ||
fun brands() = listOf( | ||
arrayOf(MovistarBrand, false), | ||
arrayOf(VivoBrand, false), | ||
arrayOf(O2Brand, false), | ||
arrayOf(BlauBrand, false), | ||
arrayOf(TelefonicaBrand, false), | ||
arrayOf(MovistarBrand, true), | ||
arrayOf(VivoBrand, true), | ||
arrayOf(O2Brand, true), | ||
arrayOf(BlauBrand, true), | ||
arrayOf(TelefonicaBrand, 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 suggest using https://github.com/marketplace/actions/gradle-build-action to improve the performance
You can see examples in https://github.com/Telefonica/niji-for-home-android/blob/master/.github/workflows/autotest.yml
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 e905f27