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

Users should be able to define a manualScale instead of being locked into the mainScreen.scale #15

Closed
wants to merge 3 commits into from

Conversation

regnerjr
Copy link

In order to stay flexible, users of this library "need" to upload two version of there reference images to keep tests passing on various environments (Other Developers, CI, etc)

This proposal would not lock users in to always using the "one true simulator" to run their tests on.

We've recently adopted Snapshot testing at my shop and I don't really want to be the the code review czar ensuring every new PR which adds a snapshot has one at @2x and @3x. Other cancers are the size of our git repo growing over time to support 2 screenshots for each new test.

Currently the framework uses [[UIScreen mainScreen] scale] for saving and comparing snapshots of views and layers.

If the view or layer are meant to be displayed at a different scale that doesn't match the main screen's one, the screenshots get resized (minified / magnified) and the comparisons are not correct any more from the point of view of the user, since the images are different from what will be shown to the user.

This PR tries to remove this limitation for those special cases by allowing the test developer to manually override the scale that will be used.


This work was originally proposed by @djrtl as facebookarchive/ios-snapshot-test-case#166


Thanks for doing the original implementation @djrtl, I hope I did not overstep by re-opening this here now that this lib is under new ownership.

* Manual scale wasn't propagated properly to image capture and reference image selection methods
@CLAassistant
Copy link

CLAassistant commented Feb 21, 2018

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ regnerjr
❌ MatteoMecucci-TomTom
You have signed the CLA already but the status is still pending? Let us recheck it.

This better reflects the duality between it an `UIScreen.scale` which
has type `CGFloat`
@djrtl
Copy link

djrtl commented Feb 22, 2018

@regnerjr of course no problem, go ahead 👍

@alanzeino
Copy link
Collaborator

I'll take a look at this soon.

@alanzeino
Copy link
Collaborator

Is this resolved here?

facebookarchive/ios-snapshot-test-case#166 (comment)

@m-ruhl
Copy link

m-ruhl commented Mar 22, 2018

@alanzeino
The described problem seams to be fixed.
Unfortunately with the scaling the images have most of the time little differences, and are failing therefore.

@regnerjr
Copy link
Author

Thanks for the support on this Alan. 👍 Really glad you are supporting this project.

I'm thinking perhaps a better approach (instead of having a fixed size window and trying to do all of our snapshotting in there) will be to do snapshots for all devices we support.
(Current project is iPhone only, only Portrait orientation).
I've been thinking of adopting the technique of running the snapshot tests in their own, test target and making a build for each device and testing all 4 configs iPhoneX, iPhone 5.5", iPhone 4.7", and iPhone 4".

Glad we've got a fix for that issue sorted out (each just needs it's own build folder) #37

Cheers. Keep up the good work.

@regnerjr regnerjr closed this Jul 31, 2018
@Weizzz
Copy link

Weizzz commented Aug 27, 2020

It seems the capability for using manualScale is no longer possible and it's back to using [[UIScreen mainScreen] scale].

@alanzeino
Copy link
Collaborator

It seems the capability for using manualScale is no longer possible and it's back to using [[UIScreen mainScreen] scale].

What did you see when debugging the manualScale?

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

Successfully merging this pull request may close these issues.

None yet

8 participants