search

Antondomashnev / FBSnapshotsViewer

A mac os application that shows the failing snapshot tests from FBSnapshotTestCase
MIT License
224 stars 15 forks source link

Mechanism to reject snapshot. #71

Open babbage opened 6 years ago

babbage commented 6 years ago

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.

babbage commented 6 years ago

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: https://github.com/Antondomashnev/FBSnapshotsViewer/pull/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.

babbage commented 6 years ago

The merging of TestResultCell.xib between this pull request and https://github.com/Antondomashnev/FBSnapshotsViewer/pull/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: https://github.com/babbage/FBSnapshotsViewer/commit/9829a3c99074eb3fa8f1e4ad8a267a99ffda809d

babbage commented 6 years ago

Rebased onto current master, with change to Accept and Accept All.

Antondomashnev commented 6 years ago

@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.

babbage commented 6 years ago

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:

  1. The test image represents an improvement in the app, and thus is now the preferred state over the reference image. (ACCEPT)

  2. The test image represents a regression. (REJECT)

  3. There is no reference image. The test image might represent a desirable state (ACCEPT) or might represent a view that is actually revealed to be broken in some way (REJECT).

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?

babbage commented 5 years ago

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?

babbage commented 5 years ago

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. :)