-
Notifications
You must be signed in to change notification settings - Fork 211
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
Implement AutoRecord for new snapshots. #32
base: main
Are you sure you want to change the base?
Conversation
Complementary feature to the implementation of AutoRecord proposed to iOSSnapshotTestCase in this pull request: uber/ios-snapshot-test-case#32
One use of such a change is in partnership with a tool like FBSnapshotsViewer, which enables the review of failed snapshot tests, comparison with reference images, and replacement of old reference images with test snapshots. The pull request at Antondomashnev/FBSnapshotsViewer#72 implements the ability to recognise the images created with the autoRecord functionality in the current pull request, and to accept those images as the reference image. |
Complementary feature to the implementation of AutoRecord proposed to iOSSnapshotTestCase in this pull request: uber/ios-snapshot-test-case#32
ddd2fd1
to
6eb98b0
Compare
Could we not implement this via the Right now, by going through the In addition you also had to use this hack:
To force the library to save the image for you. But in this case, 'imagesSame` has nothing to do with the mode we're in ("auto record"). I think it would be a little easier and clearer if this started from
Then in
...and finally back in
...this is easier to understand, and doesn't hack the recording of snapshots into the verify code path. |
if (self.recordMode) { | ||
return @"Test ran in record mode. Reference image is now saved. Disable record mode to perform an actual snapshot comparison!"; | ||
} | ||
else if (!testSuccess && !self.autoRecord) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please match the indentation of the library
I think you might be right that the implementation I've produced has some hacky elements, and there is a cleaner approach. The alternative approach you outline does actually probably produce something that is closer to what a user might expect from something called "AutoRecord", in that if I've read your alternative correctly, the way it would work is that it would make comparisons where a reference image exists, and automatically and silently store a new reference image for any test where there wasn't one. Closer to what a default expectation might be for "AutoRecord", but the behaviour I've described above is not what I was trying to achieve. Instead, what I was interested in was to have a test fail but produce a reference image candidate. Then, using FBSnapshotsViewer as I linked at the top, there would be a review step where you look at the candidate image, and have to manually select whether to Accept or Reject the candidate image. Perhaps both of these ideas are worth having; your approach being perhaps AutoRecord and another one that is AutoRecordButFail or something. In either case, this could still potentially be done through recordSnapshotOfViewOrLayer, just that if it is done there, there would need to be knowledge in that method of how to save a failed comparison image. But let's agree on the intention first, then can figure out the least hacky way to do it. Later: OK, I think I'm partly wrong above. Your proposed approach wouldn't silently store the new reference image. It would return an error, and show the test as failed. But, I think I am correct re the part that it would at least as currently outlined save that image to the reference images directory immediately, rather than saving it as a failed comparison image that could then be accepted or rejected. |
Yep I totally want the tests to fail in auto record mode; I just wanted this implemented in a more obvious way like the other mode (‘record mode’) for code clarity reasons. I personally don’t understand how this could be called ‘auto record’ if the images aren’t placed in the reference images directory. Without that, it’s not “auto record” to put them in failed. I guess it won’t work for that Mac tool you’re planning on updating, but “auto record” to me really sounds like “hey if I don’t have this reference image just generate it and move on” not “if I don’t have this reference image save it as a failed image in the failed snapshots directory”. |
Essentially, the current proposal is trying to implement two things that are related but could be seen separately: b) Have a process that provides for a deliberate decision about whether something should be accepted as a new reference image, rather than saving that image to the reference images directory automatically. I guess I have always wondered about the original implementation of this tool, where if you have a new snapshot or set of snapshots being added, your only option essentially is to blindly add them to the reference images directory, and then use some sort of external process (e.g., source control) to find the new snapshots, review them, and confirm that they are good. Or, to just assume they are good unless you find otherwise during other testing, so you are only looking for future regressions, rather than the possibility that there is already an issue at the point of initially running the process. Since iOS-snapshot-test-case doesn't provide a tool for snapshot review, perhaps that is just business as usual. It is the case that I have only ever used it in conjunction with the Mac tool for the review process, so I don't have practical experience with working through the other method. Fair enough that perhaps AutoRecord is the wrong term. Or that AutoRecord should be used as a term that implements a) and saves them to the reference images directory, but having a second option that instead creates a failed image instead? Because if you see an absence of a baseline as being like having a current baseline that is blank, then having a failed image and a diff that shows it is 100% different makes sense. Would that be acceptable? |
To answer the general question there; yes the intention of the library was to be used in conjunction with source control tools. At Facebook, they use Phabricator which allows engineers to see what has changed in a PR just as the commit would look if landed (though of course they don't use this library anymore or this workflow, for snapshots) — so when the library was created, that was the intended workflow. Personally I'm of mind that if there is going to be a GUI tool for the library, it should be provided by the library. That way, any choices made as to how the workflow changes for that GUI tool that end up in source level changes are done once, for 'the official way'. This helps us to not have to accommodate every tool that might pop up wanting to do similar things. As for building that GUI tool, I'm a little wary of doing that considering that Apple might sherlock this library really soon: |
Provides an autoRecord parameter. If set, new tests that do not have a reference image will still execute, will return a failure, and will store a reference image for approval. If recordMode == true, autoRecord is ignored and behavior is as previously.
Provides an autoRecord parameter. If set, new tests that do not have a reference image will still execute, will return a failure, and will provide a fail image which is a snapshot of the view. This provides the opportunity for the user to review the image and decide whether to set it as a reference image. If recordMode == true, autoRecord is ignored and behavior is as previously.