Mechanism to reject snapshot. #71
Conversation
|
One test is failing in this pull request (when tests are run on my local machine), which is due to an error in the test, which is fixed in: #67 Seems to be fine in the Travis CI build which is interesting. CodeBeat complains of two functions that are too similar. Looking at the code, they are similar for good reason and I can't see a useful way to combine them into a more generic function that would represent an actual code improvement. |
|
The merging of TestResultCell.xib between this pull request and #69 requires careful tweaking as xib files don't merge nicely and there are changes in both. A correctly merged version can be found in this commit: babbage@9829a3c |
This function is also identically introduced in the RemoveAccepted pull request.
Where a snapshot displays a regression, provides an option to reject a snapshot. This removes the snapshot test files from the test results directory, and removes it from the snapshot results. The original reference file remains.
Implemented here partly to ease merge with proposed changes in other pull requests.
|
Rebased onto current master, with change to Accept and Accept All. |
| @@ -66,4 +67,37 @@ class SnapshotTestResultAcceptor { | |||
| throw SnapshotTestResultAcceptorError.canNotPerformFileManagerOperation(testResult: testResult, underlyingError: error) | |||
| } | |||
| } | |||
|
|
|||
| func reject(_ testResult: SnapshotTestResult) throws -> SnapshotTestResult { | |||
| guard case let SnapshotTestResult.failed(testInformation, referenceImagePath, _, _, build) = testResult, canAccept(testResult) else { | |||
There was a problem hiding this comment.
Should canAccept be here?
|
@babbage could you please clarify the main use case for the reject button. I'm sure you've added it for reason and I'm curious about it. |
|
Sure, good question. My process was that I have been reviewing a series of test results, and wanted to be able to process them by taking action on each of them, so that when I'm done I'd have an empty list. When a test runs and fails, there's two possible reasons it might fail, and three if the AutoRecord mechanism is added to create test images even when no baseline exists:
I selected the word "Reject" because it seemed like the natural partner to "Accept" but taking a different perspective I may have the wrong word here. The intention isn't to reject the test result... actually, quite the opposite. Rather, it's rejecting the test image as a candidate to be a new reference image. Really, what this button is doing is Confirming that the failed test result was correct. It might be that what this suggests is that both "Accept" and "Reject" are the wrong words (ironic given they've only just been integrated) as there is the immediate potential conflict between accepting a test result vs. accepting an image as a new reference image. What I liked about having this feature was the ability to a) end up with an empty test images folder at the end of processing all the results and b) to have an empty list of test results in FBSnapshotsViewer so I knew I'd reviewed everything. Maybe there is a better process that would give me some of the benefits of feeling like I'd worked through all the results that is different to this? |
|
Would happily continue working on things but for the life of me for months now I've not been able to get FBSnapshotsViewer to respond to failed test snapshots. It just sits inert and doesn't recognise tests have run and have failed. Don't know if there is some incompatibility with something else that has changed in Xcode, or in my setup. Started poking around in parts of FBSnapshotsViewer that I'm not familiar with but I'm not finding it the most straightforward code base to comprehend, and there isn't a lot (any?) documentation on how it is intended to work. I know this isn't a support channel, so I've delayed posting anything for a long time, but—Is this tool still working for anyone else? |
|
OK, worked out that FBSnapshotsViewer's regex to find test results hadn't been updated to the new .xcresult format. Have written a new regex and it works. Is late. Plan to post as a pull request tomorrow. :) |
Where a snapshot displays a regression, provides an option to reject a snapshot. This removes the snapshot test files from the test results directory, and removes it from the snapshot results. The original reference file remains.
In order to implement this new function, this pull request implements the same removeTestImages supporter function that is incorporated in the RemoveAccepted pull request. The two pull requests are not however dependent on each other.