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

Make sure that perceptuallyCompare fails if Metal is not available #702

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mschuetz72
Copy link

What

Metal is currently unavailable on Github Actions. The perceptuallyCompare diffing algorithm relies on Metal render to obtain the level of correspondence to compare it with the given threshold. Snapshot assertions will falsely succeed, even if a given image does not match the stored reference image.

Why

The snapshot assertion should fail if it hasn't got the requirements to execute the tests. Currently, the only indication that something's wrong is hidden inside the logs, stating

I'm aware that there are other initiatives, such as #666, which would be perfect, but until this is solved I think it's important to make people aware that the assertions might be wrong when relying on virtual machines without Metal support.

[api] No Metal renderer available.
[api] -[CIContext render:toBitmap:rowBytes:bounds:format:colorSpace:] format Rf on GLES.

any real issue, any real inconsistency will be undetected when using perceptuallyCompare and running on a virtual machine without Metal support.

How

Added a pre-condition check to perceptuallyCompare which will fail when Metal is unavailable.

Testing

Any image assertion using perceptuallyCompare running on an environment without Metal support. See also https://github.com/mschuetz-viz/DemoSnapshotTest

Copy link

@mileswright mileswright left a comment

Choose a reason for hiding this comment

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

This would be helpful.

@knellr
Copy link

knellr commented Feb 16, 2023

+1 for this. I wish I'd seen this sooner as I did a parallel investigation in #710 on Bitrise.

@dazigna
Copy link

dazigna commented May 12, 2023

Hi,

We are facing the same issues in our company, seeing that this fix is already in place and working in this branch, are you planning on pushing it soon to the main branch then release it ? this would truly save a lot of people some headaches!

Thanks!

@NachoSoto
Copy link
Contributor

This would be very useful. I just ran into this on CircleCI, I had no idea our snapshot tests weren't doing anything on CI even though recording was working correctly 🤦🏻

NachoSoto referenced this pull request Dec 19, 2023
* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* fix

* beta 6

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* fix

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* small things

* wip

* DocC + swift-format (#765)

* DocC and swift-format support

* wip

* wip

* wip

---------

Co-authored-by: Brandon Williams <mbrandonw@hey.com>
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

7 participants