mitsuhiko / insta

A snapshot testing library for rust
https://insta.rs
Apache License 2.0
2.06k stars 93 forks source link

Align inine snapshot handling with file snapshots #510

Open max-sixty opened 1 week ago

max-sixty commented 1 week ago

I'm finding the crate is getting increasingly complicated, to the extent that as the second contributor (albeit far behind @mitsuhiko...) I'm sometimes scratching my head in how things work. Features such as https://github.com/mitsuhiko/insta/pull/489 are great but will continue to make the main path more complicated.

Potentially some unnecessary complication comes from how differently inline & file snapshots are handled:

Could we instead use .snap.new files for both file and pending snapshots, with a marker in their metadata indicating which is which? We'd read the old value from the rust file itself, wouldn't need to track run-id, and so collapse a lot of code in our main path. (We'd need to think about how line numbers change as code is updated, maybe other things I haven't thought of). As well as making the crate simpler, we also make its explanation simpler — when writing docs we often have to give footnotes like this: https://github.com/mitsuhiko/insta/blob/58daea1a4e565b2d2ae8700e7e54bd63ab8daf58/insta/src/lib.rs#L171-L172 because of the way things are handled.

We'd need to handle both for a while, and so somewhat dependent on a mechanism such as https://github.com/mitsuhiko/insta/pull/509 to ensure we don't have to handle both forever

mitsuhiko commented 6 days ago

I agree on the complexity. I'm really mentally going more and more back to the discussion in #456. I think that part of the challenge here really is that these two features (inline and non inline) just came at different times and in the wrong order.

If you treat the primary goal of the crate to just update snapshots in locations, then how that happens can be simplified. Say we have a snapshot!(...) macro then all it does, is recording changes and either applying them instantly or deferred. The reason the .pending stuff exists for inline snapshots has been concurrency problems with the way cargo tests sometimes run and that it's impossible to detect the end of a run. However I believe that there might be better ways to go around this.

But I agree that this should be aligned between the two modes.