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

Adds Screenshot testing with Roborazzi #876

Merged
merged 25 commits into from
Aug 23, 2023
Merged

Adds Screenshot testing with Roborazzi #876

merged 25 commits into from
Aug 23, 2023

Conversation

JoseAlcerreca
Copy link
Contributor

No description provided.

@google-cla
Copy link

google-cla bot commented Aug 1, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@JoseAlcerreca JoseAlcerreca force-pushed the roborazzi branch 3 times, most recently from f4736f2 to d38169d Compare August 2, 2023 16:48
@JoseAlcerreca JoseAlcerreca marked this pull request as ready for review August 3, 2023 08:33
@JoseAlcerreca JoseAlcerreca reopened this Aug 3, 2023
@SimonMarquis
Copy link
Contributor

Hi @JoseAlcerreca, awesome work!
I was just wondering if you can comment on why you chose Roborazzi over Paparazzi?
Adding the Robolectric to this codebase has been delayed for a long time.
And to me, it sometimes unintentionally pushes to write unit tests at the wrong layer of abstraction.
Also, I don't see in these tests any access to the Android framework (which would be the main reason to use Roborazzi over Paparazzi).

SimonMarquis
SimonMarquis reviewed Aug 5, 2023
View reviewed changes
.github/workflows/Build.yaml Show resolved Hide resolved
.github/workflows/Build.yaml Show resolved Hide resolved
.github/workflows/Build.yaml Show resolved Hide resolved
.github/workflows/Build.yaml Show resolved Hide resolved
@SimonMarquis
Copy link
Contributor

FWIW, Windows devs won't be able to run the whole test suite (at the moment) because of:

@JoseAlcerreca
Copy link
Contributor Author

nt on why you chose Roborazzi over Paparazzi?

Roborazzi uses Robolectric Native Graphics which is a supported API. Currently, paparazzi uses LayoutLib in an unsupported way.

Adding the Robolectric to this codebase has been delayed for a long time.
And to me, it sometimes unintentionally pushes to write unit tests at the wrong layer of abstraction.

We're not changing unit tests or the test strategy with this change.

Also, I don't see in these tests any access to the Android framework (which would be the main reason to use Roborazzi over Paparazzi).

It doesn't access the framework but it uses RNG.

@JoseAlcerreca
Copy link
Contributor Author

FWIW, Windows devs won't be able to run the whole test suite (at the moment) because of:

Yeah, known issue. Running ./gradlew test shouldn't be affected by that and we're taking the screenshots on CI, so it's independent of the developing platform.

@JoseAlcerreca JoseAlcerreca mentioned this pull request Aug 18, 2023
Merged
@JoseAlcerreca
Copy link
Contributor Author

Currently blocked by #909

@JoseAlcerreca
Adds screenshot tests using Roborazzi (Robolectric Native Graphics)
679e326 
- Adds Roborazzi to convention plugins
- Adds Screenshot helper in :core-testing
- Creates screenshot suites for :app and :feature-foryou

Change-Id: Idcdbbe6258f4bf2ad7d5ebefe208adeb2d3341f2
@JoseAlcerreca
CI and spotless
461f32a 
Change-Id: I81cb9c4c66d118c73be6dc8378b6afec5c83d43a
@JoseAlcerreca
Moves :app tests to testDemo and makes NiaAppScreenSizesScreenshotTes…
773e2bd 
…ts prettier

Change-Id: I21c1fbf174d8af06adf0676e81076ceefa02eb0e
@JoseAlcerreca
CI: Moves local tests to their own step
9e6ddf8 
Change-Id: I371552c35a7159bcd4220711791bb6adad138937
@JoseAlcerreca
CI: Adds --rerun to screenshot task
acb576e 
Change-Id: I7c50f4530b24779fea0805704ea4db3d91b431d5
@JoseAlcerreca
CI: Moves screenshots before local tests
5c9b27a 
Change-Id: Iaa5d3facff03e90c7741aa7ede706ea1bf7ea615
@JoseAlcerreca
CI: Fixes wrong if statement in workflow
25c9977 
Change-Id: I29f390595dde5510e4f50cc2a834ff9338d2a7f2
@JoseAlcerreca
CI WIP: trying to trigger the push step
39e9e67 
Change-Id: I579da1585f449610bab172b53888c45b561e3a12
@JoseAlcerreca
CI: Re-enables roborazzi verification
2065bbd 
Change-Id: I2a9e3cef61ce456cce778c9c2b9b579fc766988c
@JoseAlcerreca
Fixes flaky screenshot tests by setting LocalInspectionMode on
77cae41 
Change-Id: I59deed76b75fee4c2d57ddbb6c058df9d5632bbf
@JoseAlcerreca
CI: screenshot commits now use the original author intead of bot account
dfd869e 
Change-Id: Id48e012fbd2034ce0834db3a4633830f65bc8533
JoseAlcerreca and others added 12 commits August 18, 2023 18:57
@JoseAlcerreca
CI: Disables globbing because file_pattern didn't work
643dd0c 
Change-Id: Ic3a1e2f11c4adeaa3c22d71a2f7659f1ef25ac56
@JoseAlcerreca
CI: Trying new file pattern for png files
6603f51 
Change-Id: I1c41bed35e27d09742cde1dcd3dfc5f9e3ac60e7
@JoseAlcerreca
CI: Adds a check for forks
1090f39 
Change-Id: I6b8add80b3935cb61960b291dc73d1503397b55a
@JoseAlcerreca
🤖 Updates screenshots
851c55f
@JoseAlcerreca
Code review: toml cleanup, comments
bb9addf 
Change-Id: Ib3e6fa1ff57fb4ba2b187c039a48e1168e235d29
@JoseAlcerreca @SimonMarquis
Use new github.event.pull_request.head.repo.fork
a24bedc 
Co-authored-by: Simon Marquis <contact@simon-marquis.fr>
@JoseAlcerreca
Uses Robolectric qualifiers to set the dpi, adds section to README
67c0793 
Change-Id: I75fa32871e0123ed4ce1c2232d1d9e01f31983a7
@JoseAlcerreca
Spotless
5ae4952 
Change-Id: Iaec7eeab7f01fd332b5bf5620db7d531d6b57258
@JoseAlcerreca
Delegates creation of repository to Hilt in test
0130553 
Change-Id: I2ee5db788ec1cf17ce14b70a4959a3a5567db1af
@JoseAlcerreca
Revert "Use new github.event.pull_request.head.repo.fork"
fa5d7c6 
This reverts commit 1f15780.
@JoseAlcerreca
🤖 Updates screenshots
573b827
@JoseAlcerreca
Empty commit to trigger GHA on main branch
baf4623 
Change-Id: I94f81c39d016dd405fcbc6f3ab447f81f1bc9db7
@mmoczkowski mmoczkowski self-requested a review August 21, 2023 12:35
mmoczkowski
mmoczkowski requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@mmoczkowski mmoczkowski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

scr Screenshot 2023-08-21 at 17 06 27

NewsResourceCard.kt:246

Reference screenshots on my machine had a different date (off by one day). The dates are mapped to local time zone in the UI layer. This makes the tests non-deterministic.

@JoseAlcerreca
Makes time zones deterministic in screenshot tests
5fbc43a 
Change-Id: I7f068329b8a8aac72c56a523efc9ef61694cc7f6
@JoseAlcerreca
Copy link
Contributor Author

Good catch @mmoczkowski, forced UTC timezone in screenshot tests.

mmoczkowski
mmoczkowski approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@mmoczkowski mmoczkowski left a comment

Choose a reason for hiding this comment

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

LGTM

@JoseAlcerreca
Increases GMD timeout to 90m, but it has to be reduced
bc25b6f 
Change-Id: I083a9e8e6f720dd408a80ed12ac5ed76f1d57617
alexvanyo
alexvanyo approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@alexvanyo alexvanyo 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! 🖼️

@JoseAlcerreca JoseAlcerreca merged commit 886158d into android:main Aug 23, 2023
6 checks passed
@SimonMarquis SimonMarquis mentioned this pull request Nov 3, 2023
Closed
@alexvanyo alexvanyo mentioned this pull request Feb 1, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonMarquis SimonMarquis SimonMarquis left review comments

@mmoczkowski mmoczkowski mmoczkowski approved these changes

@alexvanyo alexvanyo alexvanyo approved these changes

@dturner dturner Awaiting requested review from dturner

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

Successfully merging this pull request may close these issues.
4 participants
@JoseAlcerreca @SimonMarquis @alexvanyo @mmoczkowski