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

Added Tolerance on pixel level #43

Merged
merged 1 commit into from
Jan 10, 2019

Conversation

JerryTheIntern
Copy link
Contributor

@JerryTheIntern JerryTheIntern commented Jul 23, 2018

Pixel Tolerance

The Problem

Does it really matter if there is 2 shades of colour difference in an element on the screen ?
Is it really worth spending our time fixing it ?

There have been a couple of instances where the snapshot testing has produced entirely grey diff images, or at least seemingly so to the naked eye, when run on different machines. After increasing contrast in an image editor, there are minuscule changes (sometimes a couple of shades of grey difference) in the colour of a pixel. This brings about test failures which are difficult to diagnose, provide very little value and chew up developer time.

See #27 and #37 for an example of this.

Regular Tolerance

The default tolerance implementation in the test case is very simplistic:

no. of diff. pixels / total pixels = percentage difference

Under the current implementation introducing even the smallest of tolerances here to ignore a slight difference in colour could result in small actual UI changes being missed. Basically any change to any element with a small enough share of the screens pixels will be missed.

The Solution

Pixel Tolerance

This PR introduces a new tolerance implementation which considers the percentage change in a pixels RGBA colour components to determine whether or not a pixel is considered 'different'.

(referencePixel.colourComponent - actualPixel.colourComponent) / 256 = percentage difference

This new implementation means we are able to tolerate slight changes in a pixels colour components but still pick up what would be a 'major UI change' such as moving, adding, or removing an element on the screen or drastically changing its colour.

The new implementation can be used as a replacement, or work in tandem with the old implementation to provide the greatest flexibility to the developer. It's also fully backwards compatible and shouldn't cause any issues for consumers getting the latest version of ios-snapshot-test-case.

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2018

CLA assistant check
All committers have signed the CLA.

@agentk
Copy link

agentk commented Jul 23, 2018

Thanks Jerry. Great work.

@martinstolz
Copy link

martinstolz commented Sep 10, 2018

Fantastic solution @JerryTheIntern 👏

How's it looking with this improvement?
Looking forward to use this 👍

@JerryTheIntern
Copy link
Contributor Author

Thanks @martinstolz ! haven't hear anything from the repo owners :(

@alanzeino
Copy link
Collaborator

Sorry @JerryTheIntern, I've been silent on this PR because we've been having a discussion internally about how this repository should evolve in the future. Since this PR came out of nowhere, we weren't expecting it, and we weren't sure if we should take it.

The first issue stems from us only getting this repository in the past year and not knowing it well enough to PR such a large contribution. If I was 100% intimate with every bit of this repo, it would be easier for me to review your change. But I'm not, so it's tricky. For smaller contributions I can usually take an hour or so to test the change and code review it. But for larger ones, we err on the side of caution and unfortunately they just don't get much attention — but that's my fault and I'm sorry for that.

The second is that we just didn't know at the time when your PR'd this if the library would continue to churn with lots of feature development or if we should focus on stability maintenance, mostly because someone found this in Xcode 10's betas.

But here's what I suggest for now. Re–implement your change on top of master (because it has changed a lot since you PR'd this and rebasing will be tough). Just try to follow the existing repository's code style (you can do $ find . -name *.[hm] | xargs bin/clang-format -i -style=file to format). I'll also leave a few comments on your older PR for you to address.

Once you update, I'll take a day to PR it and we'll see if it's ready to go.

Again, sorry for the delay in responding.

@JerryTheIntern JerryTheIntern force-pushed the ToleranceOnPixel branch 2 times, most recently from 9ec959d to fdd123a Compare January 7, 2019 22:11
@JerryTheIntern
Copy link
Contributor Author

@alanzeino My turn to apologise for the delay, I've addressed the comments and run the format as well as rebased.

FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
FBSnapshotTestCase/Categories/UIImage+Compare.m Outdated Show resolved Hide resolved
@alanzeino
Copy link
Collaborator

Looks good. I'll merge after you address these and I'll bump the major version number before publishing to cocoapods.

@JerryTheIntern
Copy link
Contributor Author

All addressed 👍

Copy link
Collaborator

@alanzeino alanzeino left a comment

Choose a reason for hiding this comment

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

Tested this locally and it's good to go, just found a few regressions in nullability from the previous public methods.

FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestController.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
FBSnapshotTestCase/FBSnapshotTestCase.h Outdated Show resolved Hide resolved
@JerryTheIntern
Copy link
Contributor Author

nullablilities fixed 👍

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

6 participants