Remove accepted images from failed image path. #70
Conversation
There was a problem hiding this comment.
Thanks for the PR @babbage!
I've left some comments to discuss 😄
| @@ -10,7 +10,13 @@ import Foundation | |||
|
|
|||
| extension FileManager { | |||
| func moveItem(at fromURL: URL, to toURL: URL) throws { | |||
| try self.removeItem(at: toURL) | |||
| try deleteItem(at: toURL) | |||
There was a problem hiding this comment.
Is it necessary here to add a new method?
There was a problem hiding this comment.
Maybe we can rename deleteItem to something like deleteIfExists?
There was a problem hiding this comment.
I agree, deleteIfExists would be much better and I'd been thinking exactly the same thing since making the pull request. The reason for it being a separate method is because it's used both in moveItem here, and is called directly from the removeTestImages function in SnapshotTestResultSwapper.swift.
There was a problem hiding this comment.
I guess you mean the overhead of checking if file exists? I think it's fine and it should not add any performance cost 😄
| guard case let SnapshotTestResult.failed(testInformation, _, _, failedImagePath, build) = testResult, canSwap(testResult) else { | ||
| throw SnapshotTestResultSwapperError.canNotBeSwapped(testResult: testResult) | ||
| } | ||
| let failedImageURL = URL(fileURLWithPath: failedImagePath, isDirectory: false) | ||
| do { | ||
| let recordedImageURL = try buildRecordedImageURL(from: failedImagePath, of: testResult) | ||
| try fileManager.moveItem(at: failedImageURL, to: recordedImageURL) | ||
|
|
||
| if removeAcceptedImages { |
There was a problem hiding this comment.
I think we may want to test it somehow? Can you think of a unit or maybe integration test here?
When a new image is accepted, remove the reference image, failed image, and diff image from the failed image path.
Implemented here partly to ease merge with proposed changes in other pull requests.
|
Have rebased this branch on the current master after the Accept/Accept All change was committed, as that changed quite a lot of file and function names so there were conflicts. Lost your comment re tests in that rebasing but agree should be tested (sigh!) so will see what I can come up with. Have renamed the function to deleteIfExists. Note in this pull request I separated removeTestImages out into a separate function partly to maintain simplicity of functions, but additionally because a removeTestImages function is also needed by the Reject images proposal which is in pull request: #71. |
When a new image is accepted, remove the reference image, failed image, and diff image from the failed image path. As discussed in #27