Skip to content

Diffing.image(precision:) should be scale aware #324

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

Closed
moritzsternemann opened this issue May 5, 2020 · 3 comments
Closed

Diffing.image(precision:) should be scale aware #324

moritzsternemann opened this issue May 5, 2020 · 3 comments

Comments

@moritzsternemann
Copy link
Contributor

While working with snapshot-tests on a large scale, we resorted to lowering the snapshot resolution in some cases to improve performance. This was achieved by reducing the displayScale of the trait collection passed to the Snapshotting.image() method.

While the results are as expected, when the snapshot-test fails, the attached diff is incorrect.

Example

Notice the plus icon in the corner, which has the correct size while the i icon is scaled up.
difference_3_56070A52-D4ED-4B9D-B02B-696523A763C6

Solution

I noticed that the image diffing strategy uses the UIScreen.main.scale constant to load the reference snapshot from disk (Sources/SnapshotTesting/Snapshotting/UIImage.swift#L16).
This causes the reference image being a different size the new snapshot when creating the diff, which results in a pretty much useless XCTestAttachment.

I suggest to resolve this by passing the displayScale from the trait collection through to the image diffing strategy. I prepared the necessary changes and would like to open a PR as soon as possible if nobody finds anything wrong with my approach.

@stephencelis
Copy link
Member

This sounds good to us! Thanks for exploring the solution.

@stephencelis
Copy link
Member

@moritzsternemann This also appears to be a dupe of #243, right? I'm going to close this, but feel free to reopen if you think the issues are distinct.

@moritzsternemann
Copy link
Contributor Author

moritzsternemann commented May 11, 2020

@stephencelis yes, I totally missed that!

Though my solution won't allow snapshot tests to be run on different simulators than when the references were created because of the font rendering difference @kirillsh described in #243 (comment).

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

No branches or pull requests

2 participants