Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary snapshots #489

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lasernoises
Copy link

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 a Vec<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 as snap.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 because cargo-insta removes the old pending snapshots first. Maybe it would make sense to also do that generally at the start of assert_snapshot or alternatively cleanup all .snap.new.* somewhere.

The review TUI looks like this:

2024-05-02_15:36:43

2024-05-02_15:58:36

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 including file:// to the terminal since many terminals seem to support clicking on those as well.

@mitsuhiko
Copy link
Owner

Creating it as .snap.new.$extension.tmp and then moving it when saving the metadata might be a good alternative approach.

I like this in theory. What might make this whole thing more robust would be to say that the following has to be true:

foo.snap.ANY_EXT has to be unique. On review/write if more than one extension is found, other files are deleted. A not yet fully persisted file is temporarily stored as foo.snap._ to mark it as temporary. If it's left over after a run, it would be cleaned up like any other duplicate extension.

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

It would be interesting to also add a p for preview on macos which would open quick-look on supported files. Not sure if an equivalent exists on windows. But this could be done in a followup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants