swift-snapshot-testing icon indicating copy to clipboard operation
swift-snapshot-testing copied to clipboard

Added support for avoiding re-recording snapshots if there're no differences according to the matcher used.

Open grigorye opened this issue 3 years ago • 4 comments

This PR attempts to solve the problem of introducing unneeded changes in the snapshots for the projects that use "imprecise" matching, by avoiding recording a new version of a snapshot, if recording is enforced, and a new version of the snapshot does not differ from the original, according to the imprecise matcher used.

As an additional bonus, recording snapshots does not trigger test failures for versions that match the originals - it's now clearly seen from test results, what snapshots did not match the originals and were changed.

grigorye avatar Sep 18 '21 23:09 grigorye

@grigorye Is there anything else that needs to be done in this PR to get it marked as ready for review/merging?

agurtovoy avatar Jan 19 '22 17:01 agurtovoy

@agurtovoy Well, even though we've been using it for quite some time in "production", I would like to get a confirmation from somebody else, that it's useful and works for them. But, if you believe that it's worth pushing it for review, I don't mind starting the process.

grigorye avatar Jan 20 '22 05:01 grigorye

@grigorye We just started using https://github.com/JWStaiert/SnapshotTestingEx and, as you mention in this comment, we need something along the lines of this PR to avoid "drifting" of snapshots that are "fuzzily" compared when you enable shapshot recording for the whole test suite rather than on a test-by-test basis.

Adding SNAPSHOT_RECORDING env variable is an uncontroversial improvement and can probably be a separate (tiny) PR.

agurtovoy avatar Jan 20 '22 17:01 agurtovoy

@agurtovoy Re: SNAPSHOT_RECORDING - I also thought about a separate PR, but now I think that it's related to another feature "re-implemented" here: being able to delete the obsolete snapshots on "regeneration", without source code modifications.

In upstream, (as far as I know) the only way to delete the obsolete snapshots is: erase all the snapshots, execute all the tests, and then regenerate only the actual set. It did not require enforcing the recording by changing that variable, as by default the snapshot is recorded unless the original already exists. What is important is that this does not require any modification of the source code - we just delete files in the file system before running tests.

Here, to get rid of obsolete snapshots, we can not delete all the snapshots, as it would basically make it impossible to fuzzy compare new snapshots vs originals (originals would be gone). Instead, we rely on enforced recording and the file modification date - we touch every snapshot file that would be generated (especially the one that fuzzy matches and that we keep unchanged in terms of content). Then we can search for all the snapshots in the tree, based on the file name or residing Snapshots directory or whatever (yeah, it's a bit of a hack), and erase every snapshot which modification date is not changed. To support this very approach without modification of the source code, we need a way to enforce the recording from the outside. That's why "SNAPSHOT_RECORDING" is introduced instead of just "false".

I consider being able to erase the obsolete snapshots an existing/required feature, hence introducing this PR without support for that would be a regression from that perspective. That's why I wonder if it makes sense to keep SNAPSHOT_RECORDING inside this very PR.

What's your opinion? I feel like this SNAPSHOT_RECORDING is a hack, as it relies on certain file name structure/file modification dates, needs at least a sample script to demonstrate the approach. That's why I don't feel confident about this whole thing, I personally would not approve it in such a form... :-)

grigorye avatar Jan 20 '22 18:01 grigorye