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.

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

Improve screenshot testing with ComposablePreviewScanner #3125

Merged
merged 21 commits into from
Jul 3, 2024

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Jul 2, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other : Improve CI processes

Content

  • Replaces Showkase for screenshot tests with ComposablePreviewScanner. Showkase is only used now for the browser in :libraries:designsystem.
  • Use reflection to 'fix' the generated screenshot names with Paparazzi so we don't have any more _null_1_2_ suffixes in the screenshot filenames.
  • Add a very crude 'sharding' for recording and verifying screenshots, which allows for parallelisation and should improve CI and local test times.

Motivation and context

Improve DevX. It should also fix the case where adding/removing a single preview in a module renamed every single other one.

Screenshots / GIFs

There are a few in this PR 😇 .

Tests

We should test this against the Element Gallery.

Tested devices

Checklist

  • Changes have been tested on an Android device or Android emulator with API 23
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off
  • You've made a self review of your PR

@jmartinesp jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 2, 2024
@github-actions github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 2, 2024
@jmartinesp jmartinesp changed the title WIP Improve screenshot testing with ComposablePreviewScanner Jul 2, 2024
@jmartinesp jmartinesp added Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. PR-Change For updates to an existing feature labels Jul 2, 2024
@github-actions github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jul 2, 2024
@jmartinesp jmartinesp force-pushed the misc/jme/improve-screenshot-testing branch from 82baa39 to e8cc654 Compare July 2, 2024 10:01
@jmartinesp jmartinesp force-pushed the misc/jme/improve-screenshot-testing branch from 83a2307 to 8118aed Compare July 2, 2024 10:09
Copy link
Contributor

github-actions bot commented Jul 2, 2024

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/JLzRE2

@jmartinesp jmartinesp requested a review from bmarty July 2, 2024 10:27
@jmartinesp jmartinesp marked this pull request as ready for review July 2, 2024 10:27
@jmartinesp jmartinesp requested a review from a team as a code owner July 2, 2024 10:27
@jmartinesp
Copy link
Member Author

I also hope Showkase/KSP somehow had something to do with the OOM issues we were having in the CI and this might fix it. Although it's quite hard to test it.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.01%. Comparing base (d68ddc6) to head (c0a8025).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3125   +/-   ##
========================================
  Coverage    76.01%   76.01%           
========================================
  Files         1635     1635           
  Lines        38578    38578           
  Branches      7460     7460           
========================================
+ Hits         29325    29326    +1     
  Misses        5361     5361           
+ Partials      3892     3891    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have spot some strange things in the recorded screenshots, but I did not check every single ones.
Also there are some formatting issue on build.gradle.kts files (I have commented them all).
Can you ensure that generating Element Gallery is still working as expected?

app/build.gradle.kts Outdated Show resolved Hide resolved
features/lockscreen/impl/build.gradle.kts Outdated Show resolved Hide resolved
features/messages/impl/build.gradle.kts Outdated Show resolved Hide resolved
features/networkmonitor/api/build.gradle.kts Outdated Show resolved Hide resolved
features/poll/api/build.gradle.kts Outdated Show resolved Hide resolved
features/rageshake/api/build.gradle.kts Outdated Show resolved Hide resolved
@bmarty bmarty added PR-Build For changes related to build, tools, CI/CD and removed PR-Change For updates to an existing feature labels Jul 2, 2024
@bmarty
Copy link
Member

bmarty commented Jul 2, 2024

(changing the label to PR-Build which seems more appropriate than PR-Change, let me know if you agree)

@bmarty
Copy link
Member

bmarty commented Jul 2, 2024

@jmartinesp jmartinesp force-pushed the misc/jme/improve-screenshot-testing branch from 8bfbe8a to ed9249a Compare July 2, 2024 12:17
@jmartinesp jmartinesp force-pushed the misc/jme/improve-screenshot-testing branch from ed9249a to e417bec Compare July 2, 2024 12:20
@jmartinesp
Copy link
Member Author

I believe with the latest changes everything should work. @bmarty can you also test the gallery locally? It seems to work for me, but having a fresh pair of eyes on it is probably a good idea.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Ideally the file data.js should be updated by this PR, so that the index.html is not broken.

Also I have noticed that the Preview are not sorted by name. It should start by AboutView like here:

image

This was done here: https://github.com/element-hq/element-x-android/blob/develop/tools/test/generateAllScreenshots.py#L120. See more details in the comment.

tests/uitests/src/test/kotlin/ui/PreviewTest.kt Outdated Show resolved Hide resolved
tools/test/generateAllScreenshots.py Outdated Show resolved Hide resolved
tools/test/generateAllScreenshots.py Outdated Show resolved Hide resolved
tools/test/generateAllScreenshots.py Outdated Show resolved Hide resolved
tools/test/generateAllScreenshots.py Outdated Show resolved Hide resolved
@jmartinesp
Copy link
Member Author

Ideally the file data.js should be updated by this PR, so that the index.html is not broken.

I think for that we need the 'Sync Localazy' flow to run since any screenshot I record in my computer will later be overridden. Maybe I should just trigger a sync as soon as this PR is merged?

The latest changes should fix the issues.

@jmartinesp
Copy link
Member Author

jmartinesp commented Jul 3, 2024

Well, there goes my theory about the OOM errors maybe being caused by KSP/Showkase 🫤 : https://github.com/element-hq/element-x-android/actions/runs/9772948574/attempts/1

@bmarty
Copy link
Member

bmarty commented Jul 3, 2024

The latest changes should fix the issues.

Let me check! I'll commit the change on data.js on the branch

Html is empty because DE screenshots will need to be generated again.
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

OK, I have updated the file data.js.
I think we can merge the PR and trigger a Localazy sync just after to ensure that DE screenshots will be correctly recorded and the Gallery correctly rendered.
Thanks for all the updates!

Copy link

sonarcloud bot commented Jul 3, 2024

@jmartinesp jmartinesp merged commit b0cebf5 into develop Jul 3, 2024
26 checks passed
@jmartinesp jmartinesp deleted the misc/jme/improve-screenshot-testing branch July 3, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Build For changes related to build, tools, CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants