Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Migrate Paparazzi to Compose Preview Screenshot testing #3507

Closed
wants to merge 5 commits into from
Closed

Migrate Paparazzi to Compose Preview Screenshot testing #3507

wants to merge 5 commits into from

Conversation

isaacnguyen0809
Copy link
Contributor

@isaacnguyen0809 isaacnguyen0809 commented Sep 14, 2024 •
edited
Loading

Pull request (PR) checklist

Please check if your pull request fulfills the following requirements:

  • The PR is submitted to the main branch.
  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read and understand the Developer Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • My PR includes only the necessary changes to fix the issue (i.e., no unnecessary files or lines of code are changed).
  • 🎬 I've attached a screen recording of using the new code to the next paragraph (if applicable).

Description

  • Let's check the integration first and if it's fine, i will start write the docs for it

What's changed?

  • Migrate Paparzzi testing to Compose preveview screenshot testing
  • Remove Paparazzi integration
  • Add image difference threshold

Risk factors

...

Does this PR close any GitHub issues?

Troubleshooting CI failures ❌

Pull request checks failing? Read our CI Troubleshooting guide.

@isaacnguyen0809 isaacnguyen0809 changed the title Fix issue 3243 [Darf] Fix issue 3243 Sep 14, 2024
@isaacnguyen0809 isaacnguyen0809 changed the title [Darf] Fix issue 3243 [Draft] Fix issue 3243 Sep 14, 2024
ILIYANGERMANOV
ILIYANGERMANOV suggested changes Sep 14, 2024
View reviewed changes
buildSrc/src/main/kotlin/Constants.kt Outdated
@@ -1,3 +1,3 @@
object BuildConfigConstants {
const val IMAGE_DIFFERENCE_THRESHOLD = 0.1f
const val IMAGE_DIFFERENCE_THRESHOLD = 1f
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that threshold 100%?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing with CI/CD so it's not final yet, I'm looking for the closest deviation. Pls wait hehe

Copy link
Collaborator

Choose a reason for hiding this comment

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

No rush^

@ILIYANGERMANOV
Copy link
Collaborator

Nice! Google have added threshold support. A few notes:

  • currently 100% threshold makes the tests useless (if I understood it correctly)
  • We neee to update the docs before merging, otherwise contributors will start pinging me

@ILIYANGERMANOV ILIYANGERMANOV added the requested changes Changes are needed. Please, apply them label Sep 14, 2024
@isaacnguyen0809 isaacnguyen0809 force-pushed the fix-issue-3243-new branch 4 times, most recently from cae9695 to ffd5332 Compare September 16, 2024 07:13
@isaacnguyen0809
Copy link
Contributor Author

isaacnguyen0809 commented Sep 16, 2024 •
edited
Loading

@ILIYANGERMANOV - please ignore this comment for now, i need to check it again

According to what I have tried, an image difference threshold of 6% can pass all cases. The case that raises the threshold to 6% is due to the Disclaimer Screen. I will attach the test results in the file below, with a 3% threshold, it passes only 50%. Excluding this screen, I find that the remaining test cases can pass with a threshold of 1.3%.

image

Therefore, i think we have two options:

  1. We accept a 6% threshold (if you’re comfortable with this)
  2. Because the Disclaimer Screen is just a static screen showing text without any complex UI, we can skip the screenshot test for this screen and use the 1.3% threshold for the rest.
    Wdyt?

BTW, I have updated the docs for this part. Could you check if anything else is needed?

@isaacnguyen0809 isaacnguyen0809 changed the title [Draft] Fix issue 3243 Migrate Paparazzi to Compose Preview Screenshot testing Sep 16, 2024
@ILIYANGERMANOV
Copy link
Collaborator

Nice work @isaacnguyen0809! I think 2) is a good option. The best choice would be to set 1.3% for all tests and 6% exception for the Disclaimer (e.g. support threshold customization for different tests). Or we can find what makes the Disclaimer screen flaky.

Anyway, I'm happy with skipping its tests if my suggestions are too hard to impl

@ILIYANGERMANOV ILIYANGERMANOV added the keep Keep it from automatically getting closed by Stale label Sep 16, 2024
@isaacnguyen0809 isaacnguyen0809 force-pushed the fix-issue-3243-new branch 2 times, most recently from 0598432 to ae9a725 Compare September 17, 2024 03:42
ILIYANGERMANOV
ILIYANGERMANOV reviewed Sep 19, 2024
View reviewed changes
.github/workflows/compose_screenshot_test.yml
@@ -8,7 +8,7 @@ on:

jobs:
sceenshot_test:
runs-on: ubuntu-latest
runs-on: windows-latest
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@isaacnguyen0809 can we use Linux? What's the benefit of running them Windows? My concern is that GitHub Actions Windows are more expensive => will provide less free instances for open-source projects

Copy link
Contributor Author

@isaacnguyen0809 isaacnguyen0809 Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ILIYANGERMANOV
I'm currently running some additional tests, but they've been temporarily paused due to my full-time job.
After multiple checks, I realized the large discrepancies are caused by the execution environment. Since I'm using a MacBook, the base screenshots were created on macOS. When I run the tests on macos-latest on GitHub Actions, the deviation is minimal, and the tests pass smoothly. However, when running them on windows-latest or ubuntu-latest, a few screens have much larger discrepancies, forcing us to raise the allowable threshold.

I’m trying to figure out the maximum allowable threshold for testing across two environments besides macOS. I’m also considering having different thresholds for each module or screen, but I find that pretty inconvenient. We need to think about it some more after i have the data from those tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, no rush^ I marked the PR with "keep" so the CI won't close it. Take your time :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The screenshot tests must work for all platforms because we have many contributors with different machines

@ILIYANGERMANOV ILIYANGERMANOV removed the keep Keep it from automatically getting closed by Stale label Oct 14, 2024
@ILIYANGERMANOV
Copy link
Collaborator

We can re-open it when done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ILIYANGERMANOV ILIYANGERMANOV ILIYANGERMANOV requested changes

@shamim-emon shamim-emon Awaiting requested review from shamim-emon

@akashs056 akashs056 Awaiting requested review from akashs056

Assignees
No one assigned
Labels
requested changes Changes are needed. Please, apply them
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate Paparazzi to Compose Preview Screenshot testing
2 participants
@isaacnguyen0809 @ILIYANGERMANOV