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

[NTV-648] Fix Screenshot Tests Between Intel Recordings and M1 Verifications #1757

Closed
wants to merge 3 commits into from

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Nov 27, 2022

📲 What

Existing snapshot test verifications failing on M1. These snapshots were recorded on Intel machines and as we port over our build processes to M1, we hadn't checked if the test cases were passing locally.

Turns out there are issues and the best future proof solution is to individually re-record them on M1 machines, but given the number of screenshots its not a practical solution for now. We can re-record as we go, so this is a holdover to ensure both M1 and Intel machines can still use our base of screenshots and have tests pass locally.

🤔 Why

These articles go into detail regarding the idea of perceptual precision and how this issue is a community wide problem at the moment.

  1. Original FBSnapshotTestCase M1/Intel issue.
  2. M1/Intel fix with SnapshotTesting PR
  3. Context PR for subpixel precision.
  4. Explainer of perceptual precision.
Perceptual Precision Value Description
100% Must be exactly equal
≥ 99% Allows differences not perceptible by human eyes
≥ 98% Allows differences possibly perceptible through close observation
≥ 90% Allows differences perceptible at a glance
≥ 50% Allows differences when more similar than opposite
≥ 0% Allows any differences

🛠 How

From the reading I did on the above links it seems like replacing iOSSnapshotTestCase framework with SnapshotTesting (the fix is here) and using perceptualPrecision in our tests is the right way to go.

Wrote a new assetExistingSnapshot and verifyExistingSnapshot function based on the versions written in SnapshotTesting and hardcoded the screenshots directory. The file names are still dynamic based on the test names.

The idea is just a hold-over until we eventually completely remove iOSSnapshotTestCase from the project.

🏎 Performance

  • SnapshotTesting should be faster due to some internal reworkings on CoreImage, but test this locally on the entire suite once all existing snapshots use the new assert function.

✅ Acceptance criteria

  • Pre-recorded screenshots need no new re-recordings and pass with no less than 98% perceptual precision on M1 machines.

⏰ TODO

  • Ensure existing test cases are not re-recorded, but use the new assert function with at least 98% perceptual precision.
  • Record tests for SetYourPasswordViewController and ResetYourFacebookPasswordViewController as they are missing.
  • Commented out testOptimizelyFeatureFlagToolsViewController until this pr is merged. Comment back in with this pr.

matched an Intel record screenshot against an M1 run test using SnapshotTesting framework instead of iOSSnapshotTestCase. With precision 99% the tests pass. Still to test the reverse of M1 record snapshot on Intel using the functions outlined.
…eenshots and verify them on M1. Seems to work up to a perceptable difference of 98%

pointfreeco/swift-snapshot-testing#628 (comment)
@msadoon msadoon added this to the release-5.7.0 milestone Nov 27, 2022
@msadoon msadoon self-assigned this Nov 27, 2022
@tdrhq
Copy link

tdrhq commented Jan 18, 2023

@msadoon Hi Mubarak! I've previously chatted with Kickstarter folk (Isabel, Justin, Hari) about Screenshotbot, but back then they didn't have the bandwidth (we did get pretty close to an integration though). Screenshotbot would solve this issue nicely, without having to worry about perceptualPrecision.

Basically, as long as you generate screenshots in CI, (so always identical machines), you would never have this problem. You don't have to store screenshots in your repo, and let Screenshotbot notify you on changes on your Pull Request.

I still have the demo of all of Kickstarter-ios' screenshots imported. Let me know if you're interested in chatting!

(PS. Last time I chatted with Kickstarter, we didn't really have customers TBH. These days we do have people using this with both ios-snapshot-test-case and swift-snapshot-testing, so the integration should go pretty smoothly.)

@msadoon msadoon changed the title [Unticketed] Fix Screenshot Tests Between Intel Recordings and M1 Verifications [NTV-648] Fix Screenshot Tests Between Intel Recordings and M1 Verifications Feb 13, 2023
@msadoon
Copy link
Contributor Author

msadoon commented Feb 16, 2023

@msadoon Hi Mubarak! I've previously chatted with Kickstarter folk (Isabel, Justin, Hari) about Screenshotbot, but back then they didn't have the bandwidth (we did get pretty close to an integration though). Screenshotbot would solve this issue nicely, without having to worry about perceptualPrecision.

Basically, as long as you generate screenshots in CI, (so always identical machines), you would never have this problem. You don't have to store screenshots in your repo, and let Screenshotbot notify you on changes on your Pull Request.

I still have the demo of all of Kickstarter-ios' screenshots imported. Let me know if you're interested in chatting!

(PS. Last time I chatted with Kickstarter, we didn't really have customers TBH. These days we do have people using this with both ios-snapshot-test-case and swift-snapshot-testing, so the integration should go pretty smoothly.)

Hi @tdrhq !

Screenshotbot looks like a good solution to move our testing completely to CI. Maybe we can discuss with @Arkariang to get a better understanding of the scope of the work (on Android as well) and how screenshotbot could help us write tests in the future. My only thought is that we are still building out a pipeline for things like end-to-end testing and trying to remove our dependence on human effort for regression testing. We're also building our team this year and there are going to be more eyes on this project soon, so your solution makes sense and will be discussed on our side.

@msadoon msadoon closed this Feb 16, 2023
@msadoon
Copy link
Contributor Author

msadoon commented Feb 16, 2023

Closing out this PR as it is pretty out of date and replaced by this one.

@msadoon msadoon removed this from the release-5.7.0 milestone Feb 16, 2023
@tdrhq
Copy link

tdrhq commented Feb 16, 2023

@msadoon Sounds good, reach out to me at arnold@screenshotbot.io whenever you're ready to make this move!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants