Save failed images as attachments with XCTest

[mdiep]

We use this change because it makes it easy to save failed snapshots on CI. When a snapshot test fails, developers can view the failures directly on CI without needing to reproduce the issue locally. This is often very helpful—particularly for localization issues and tests that depend on the timezone.

[CLAassistant]

All committers have signed the CLA.

[reidmain]

Sorry it has taken so long to address this @mdiep. We are all for adding this functionality but there is one gotcha with the implementation that you may not have seen because I assume you're using this in Swift. That or you are manually providing a non-null identifier in your Objective-C code.

All of our example code generally uses the FBSnapshotVerifyView macro and passes in nil for the identifier property. That property is nullable so there are no issues there but unfortunately the runActivityNamed:block: method requires that the name parameter not be null. If it is you get an exception in your test when the failed reference image is saved.

Do you require the activity to be named after the identifier for this to work on your CI environment or could we change it?

Another alternative would be to not use runActivityNamed:block: and instead find a way to access the XCTestCase that is being run and directly add the attachments to them.

[mdiep]

I don't think we depend on the name of the activity, but we do use a wrapper API that ensures that there will be one. So it wouldn't be a problem to change the name. It might be nice to use the identifier if it's not null?

[reidmain]

That is definitely possible. We could use the selector as a fallback as it is used in the name of the file?
Something like [XCTContext runActivityNamed:identifier ?: NSStringFromSelector(selector) block:^(id<XCTActivity> _Nonnull activity) {. Or we could just make it generic like "Image Attachments".

Changing gears, there are three other PRs (#71, #84, #19) that are implementing derivatives of this feature but yours is the only one that uses runActivityNamed:block: so I think we should move forward with it. Would you be averse to updating this PR to add what is common among the others which is naming the attachments and adding the diff image?

Also I am more than happy to take this work on myself and put up a PR that integrates your change, attaches the diff image, names all of the attachments and also attaches the image when ran in record mode. It could close all four outstanding PRs at once. I feel bad that we have taken so long to address this feature so I'd like to do whatever I can to see this through ASAP.

[mdiep]

I'd be happy to let you take it over. 😅 I don't have a lot of context here and I'm not sure exactly what's required.

[reidmain]

I'd be happy to let you take it over. 😅 I don't have a lot of context here and I'm not sure exactly what's required.

Sounds good. I'll get a PR up today that should finally take care of all of this stuff. Again, so sorry it took us this long to act.