Use `INSTA_UPDATE=force` for forcing snapshot updates

mitsuhiko / insta

A snapshot testing library for rust

https://insta.rs

Apache License 2.0

2.15k stars 98 forks source link

Use `INSTA_UPDATE=force` for forcing snapshot updates #482

Closed max-sixty closed 2 weeks ago

max-sixty commented 5 months ago

In an effort to simplify the configs, this merges the INSTA_UPDATE and INSTA_FORCE_UPDATE configs. Conceptually, INSTA_FORCE_UPDATE overwrites INSTA_UPDATE; they naturally fit into the same config setting.

I realized after starting this that we want to be careful about supporting new & old versions of cargo-insta. So this takes a conservative approach, ~only changing insta at first, but with the future updates to cargo-insta commented in the code. I realize that adds a bit of complication; though on balance I think simplifying the configs would be helpful and this makes a step towards that.~ Adjusted to use the underlying version of insta; I think a good approach!

~It stacks on https://github.com/mitsuhiko/insta/pull/479, which should merge first.~ Merged

~I'd be open to writing some tests for this if that'd be helpful.~ Written

mitsuhiko commented 4 months ago

I think this makes sense in general. I do want to run this first though so I will hold on merging until the next release.

max-sixty commented 1 month ago

The latest commit:

Targets the insta version when deciding which env var to use. This lets us support older versions while also using the new env var immediately for newer versions
Adds some integration tests for both pre & post 1.40.0
For those to work, I needed to bump the current version in the repo to 1.40.0. (FYI for similar reasons, we bump the version in PRQL immediately after releasing, rather than immediately before.)
Bumps MSRV to 1.65 for cargo-insta...

mitsuhiko commented 4 weeks ago

I would merge this but could you resolve the conflicts?

max-sixty commented 4 weeks ago

OK, done. The prior commit actually caught a semantic conflict when running --force-update-snapshots on inline snapshots, so made a small change to make that work.

This PR ends up being a lot of additional code for a "simplification" PR. But in retrospect I think in the long-long term it does get simpler, and there's some nice machinery here for making insta & cargo-insta work together nicely (+ our ability to test that).

max-sixty commented 3 weeks ago

Missed the release so changed the 1.39->1.40, 1.40->1.41

max-sixty commented 3 weeks ago

Will merge in the next week without more feedback, given the note above, to keep things moving.

Lmk if that's too aggressive, both for this PR and the principle