Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a relatively basic implementation of binary snapshots. I decided to adapt my implementation mentioned in #196 (comment) to handle binary files in general and remove the whole thing that prints images to the terminal.
The basic approach is store a file next to the snapshot file with the provided extension appended. The snapshot macro gets passed a closure with an
&mut File
parameter. Just using aVec<u8>
would be somewhat simpler in implementation, but I would be worried about memory usage if there's a bunch of tests running with large files.I decided not to return a
Result
from the closure, because if there's an I/O error inside it probably makes sense just to panic there because we're inside a test anyway. But maybe it would still make sense to return a Result there just so there is no need for.unwrap()
inside the closure?Currently it directly writes the file at the location where it will end up at the beginning of the
assert_snapshot
function. This means the binary file is created assnap.new.$extension
much sooner than the metadata is saved because we need the file to be able to compare it. What is a bit weird is if there is a crash somewhere between overwriting the binary file and storing the .snap file it will leave the two files in an inconsistent state. Creating it as.snap.new.$extension.tmp
and then moving it when saving the metadata might be a good alternative approach.There is also a problem if the extension changes between two runs where the old pending binary file will stay around. It doesn't seem to a problem in practice though because when running with
cargo insta test
becausecargo-insta
removes the old pending snapshots first. Maybe it would make sense to also do that generally at the start ofassert_snapshot
or alternatively cleanup all.snap.new.*
somewhere.The review TUI looks like this:
The open action opens both files (or just one if there's no existing snapshot, or the existing one is text) in an external application using the
open
crate.The file paths are using the OSC-8 escape sequence (described here) to link to the files. Here it says that
file:///
without a hostname should be avoided, but at least my terminal (WezTerm) seems to support it without a hostname and so I left that out for now since it would mean an extra dependency on a crate to get the hostname. As an alternative we could also just print the path includingfile://
to the terminal since many terminals seem to support clicking on those as well.