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

Recognise and enable acceptance of AutoRecorded images. #72

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

babbage
Copy link
Collaborator

@babbage babbage commented Apr 7, 2018

Complementary feature to the implementation of AutoRecord proposed to iOSSnapshotTestCase in this pull request: uber/ios-snapshot-test-case#32. Enables image to be accepted (“swapped”) as a reference image, when an original reference image does not exist.

@babbage
Copy link
Collaborator Author

babbage commented Apr 7, 2018

This pull request stands alone, but would function best in combination with the following three pull requests:
#69
#70
#71

In combination, and with the AutoRecord change to iOSSnapshotTestCase linked above, these changes enable snapshots to be automatically created on first run of a test if a reference image does not exist, to be reviewed, to be accepted or rejected via FBSnapshotsViewer, and in both cases for the test images to be cleaned up afterwards.

Complementary feature to the implementation of AutoRecord proposed to iOSSnapshotTestCase in this pull request: uber/ios-snapshot-test-case#32
Enable image to be accepted (“swapped”) as a reference image, when an original reference image does not exist.
@babbage
Copy link
Collaborator Author

babbage commented Apr 9, 2018

Rebased on current master.

@@ -34,7 +34,7 @@ class SnapshotTestResultAcceptor {
let failedImageSizeSuffix = imagePath.substring(with: failedImageSizeSuffixRange)
let recordedImageURLs = testResult.build.fbReferenceImageDirectoryURLs.flatMap { fbReferenceImageDirectoryURL -> URL? in
let url = fbReferenceImageDirectoryURL.appendingPathComponent(testResult.testClassName).appendingPathComponent("\(testResult.testName)\(failedImageSizeSuffix)").appendingPathExtension("png")
return fileManager.fileExists(atPath: url.path) ? url : nil
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to change it?

Copy link
Collaborator Author

@babbage babbage Apr 16, 2018

Choose a reason for hiding this comment

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

Hmm. This change was the shortest path to implement the desired functionality here, which was to still create test images even if a reference image didn't exist. However, now that I look at it, it seems like maybe it would be better to still throw SnapshotTestResultAcceptorError.notExistedRecordedImage but then catch it and handle it by doing what we want with the autoRecording functionality. I think I was put off a bit by framing this as a SnapshotTestResultAcceptorError when really it's a SnapshotTestResultAcceptorResult, given a missing reference snapshot is no longer considered an error per se, if and when this pull request is integrated.

I will take another look at this.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @babbage ;)

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.

2 participants