search

mitsuhiko / insta

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

Should we have `snapshot_kind: text` in snap files? #676

Open nyurik opened 4 weeks ago

nyurik commented 4 weeks ago

The binary support is a great step, but I don't think it warrants a massive regeneration of all the snapshot files in all the projects - which always happens if someone does a rm -rf expected + bless new results. I think the snapshot files should implicitly be text unless they have a snapshot_kind: ... value. text should also be supported there, but I don't think its a good idea to generate it... just due to the churn it causes the whole ecosystem.

max-sixty commented 4 weeks ago

I haven't put much weight on the cost of the churn — maybe it's higher than I expected. Do others / @mitsuhiko have thoughts?

How would you see this happening through time? Would we never add the field for text snapshots? Assuming we introduce it at some point in the future, is there a benefit to pushing it to the future?

And to set the context for the discussion — as you point out — the field is only added with --force-update-snapshots or new snapshots....

nyurik commented 3 weeks ago

Yes, I think "less is more" applies here - there is near-zero value of explicitly adding snapshot_kind: text to the snapshots - most of them will be that anyway. All other types should be set though, but this provides a clean path to the new types without any changes to the existing files, even when regenerated.

max-sixty commented 3 weeks ago

OK, I'll leave open to see if others have a view.

I appreciate it's some aesthetic churn. But OTOH it only appears when updating snapshots. And for those who care about the cleanliness of diffs, doing a single run of --force-update-snapshots will update everything in a single commit. And in general it's important for insta to be able to evolve the snapshot format, albeit in the specific case we could have traded away consistency in metadata to reduce changes.

Let's see what @mitsuhiko & others think over the next few weeks. For transparency, if we don't get traction, I plan to leave the current state.

nyurik commented 3 weeks ago

@max-sixty one more reason I think it is important: I used to have rm -rf snapshots && cargo insta test --accept command to fully refresh the snapshots. Because of this issue, I switched to cargo insta test --acept --unreferenced=delete - which at first seemed to do what I needed, only to realize that there were cases when the insta files were silently preserved even though removing and regenerating would delete them. So now I essentially have to go back to removing files all the time because I cannot rely on insta to delete extra files :(

max-sixty commented 3 weeks ago

I used to have rm -rf snapshots && cargo insta test --accept command to fully refresh the snapshots.

OoI, why not --force-update-snapshots?

only to realize that there were cases when the insta files were silently preserved even though removing and regenerating would delete them

I'm not sure I understand — is this a bug? We will fix them if we can repro them. Otherwise it's difficult to put much weight on it in making this decision...

max-sixty commented 3 weeks ago

(Also thanks for engaging and caring about insta @nyurik — I'm pushing back a bit because I want to be transparent, in contrast to leaving the issue to wither; hope you interpret that as reasonable)

mitsuhiko commented 3 weeks ago

I think it makes sense to default to snapshot_kind: text. In general I'm a bit worried about churn myself too because churn on snapshot files is a good reason for people to stop using a tool like this.

max-sixty commented 3 weeks ago

OK! I'll make this change. (will be a few days as am traveling, if anyone is in a rush feel free to have a go, will merge and release quickly if so)

nyurik commented 1 week ago

hi @max-sixty, is this still planned? I don't want to step on anyone's toes though :)

max-sixty commented 1 week ago

Is it, sorry, I have been super busy at work since returning. I'll try to get to in the next few days. I would also very happily take a PR! But even if not, I will get to it...

ilyagr commented 1 week ago

We keep getting that line added and removed from https://github.com/martinvonz/jj/blob/main/cli/tests/cli-reference%40.md.snap, depending on what version of insta each PR author uses. This would probably stabilize in a few month once everyone uses cargo-insta 1.41, but for now cargo-insta 1.41 isn't packaged for Nix, so Nix users don't have a good way to upgrade.

So, if there could be a cargo-insta 1.41.2 that doesn't add that line unnecessarily, it would help us.

Fortunately, it's not a huge deal since cargo-insta doesn't add that line if nothing else in the snapshot changes.