Use INSTA_UPDATE=force for forcing snapshot updates (#482)

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 #479, which should
merge first.~ Merged

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

Cargo.toml

@@ -16,5 +16,5 @@ inherits = "release"
 lto = "thin"
 
 [workspace.dependencies]
-# Locking because of MSRV; wait for MSRV bump or msrv-resolver
+# Needs pinning in Cargo.lock because of MSRV
 clap = {version = "4.1", features = ["derive", "env"]}

cargo-insta/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "cargo-insta"
-version = "1.40.0"
+version = "1.41.0"
 license = "Apache-2.0"
 authors = ["Armin Ronacher <[email protected]>"]
 description = "A review tool for the insta snapshot testing library for Rust"
@@ -11,24 +11,25 @@ keywords = ["snapshot", "testing", "jest"]
 categories = ["development-tools::cargo-plugins"]
 edition = "2021"
 readme = "README.md"
-rust-version = "1.64.0"
+rust-version = "1.65.0"
 
 [dependencies]
-insta = { version = "=1.40.0", path = "../insta", features = ["json", "yaml", "redactions", "_cargo_insta_internal"] }
+insta = { version = "=1.41.0", path = "../insta", features = ["json", "yaml", "redactions", "_cargo_insta_internal"] }
 cargo_metadata = { version = "0.18.0", default-features = false }
 console = "0.15.4"
 serde = { version = "1.0.117", features = ["derive"] }
 serde_json = "1.0.59"
 proc-macro2 = { version = "1.0.60", features = ["span-locations"] }
-# Pinned because of MSRV; wait for MSRV bump or msrv-resolver
+# Needs pinning in Cargo.lock because of MSRV
 syn = { version = "2.0.8", features = ["full", "visit", "extra-traits"] }
+# Needs pinning in Cargo.lock because of MSRV
 ignore = "0.4.17"
 uuid = { version = "1.0.0", features = ["v4"] }
 tempfile = "3.5.0"
-# Not yet supported in our MSRV of 1.60.0
-# clap = { workspace=true }
-clap = {version = "4.1", features = ["derive", "env"]}
-
+# Needs pinning in Cargo.lock because of MSRV
+semver = {version = "1.0.7", features = ["serde"]}
+lazy_static = "1.4.0"
+clap = { workspace=true }
 
 [dev-dependencies]
 walkdir = "2.3.1"

cargo-insta/src/cli.rs

@@ -10,12 +10,14 @@ use insta::Snapshot;
 use insta::_cargo_insta_support::{
     is_ci, SnapshotPrinter, SnapshotUpdate, TestRunner, ToolConfig, UnreferencedSnapshots,
 };
+use semver::Version;
 use serde::Serialize;
 use uuid::Uuid;
 
 use crate::cargo::{find_snapshot_roots, Package};
 use crate::container::{Operation, SnapshotContainer};
 use crate::utils::cargo_insta_version;
+use crate::utils::INSTA_VERSION;
 use crate::utils::{err_msg, QuietExit};
 use crate::walk::{find_pending_snapshots, make_snapshot_walker, FindFlags};
 
@@ -592,6 +594,9 @@ fn test_run(mut cmd: TestCommand, color: ColorWhen) -> Result<(), Box<dyn Error>
                 cmd.accept = false;
             }
         }
+        SnapshotUpdate::Force => {
+            cmd.force_update_snapshots = true;
+        }
     }
 
     // --check always implies --no-force-pass as otherwise this command does not
@@ -943,16 +948,27 @@ fn prepare_test_runner<'snapshot_ref>(
 
     proc.env(
         "INSTA_UPDATE",
-        match (cmd.check, cmd.accept_unseen) {
-            (true, _) => "no",
-            (_, true) => "unseen",
-            (_, false) => "new",
-        },
+        // Don't set `INSTA_UPDATE=force` for `--force-update-snapshots` on
+        // older versions
+        if *INSTA_VERSION >= Version::new(1,41,0) {
+            match (cmd.check, cmd.accept_unseen, cmd.force_update_snapshots) {
+                (true, false, false) => "no",
+                (false, true, false) => "unseen",
+                (false, false, false) => "new",
+                (false, _, true) => "force",
+                _ => return Err(err_msg(format!("invalid combination of flags: check={}, accept-unseen={}, force-update-snapshots={}", cmd.check, cmd.accept_unseen, cmd.force_update_snapshots))),
+            }
+        } else {
+            match (cmd.check, cmd.accept_unseen) {
+                (true, _) => "no",
+                (_, true) => "unseen",
+                (_, false) => "new",
+            }
+        }
     );
-    if cmd.force_update_snapshots {
-        // for old versions of insta
+    if cmd.force_update_snapshots && *INSTA_VERSION < Version::new(1, 40, 0) {
+        // Currently compatible with older versions of insta.
         proc.env("INSTA_FORCE_UPDATE_SNAPSHOTS", "1");
-        // for newer versions of insta
         proc.env("INSTA_FORCE_UPDATE", "1");
     }
     if cmd.require_full_match {

cargo-insta/src/utils.rs

@@ -1,6 +1,10 @@
 use std::error::Error;
 use std::fmt;
 
+use cargo_metadata::MetadataCommand;
+use lazy_static::lazy_static;
+use semver::Version;
+
 /// Close without message but exit code.
 #[derive(Debug)]
 pub(crate) struct QuietExit(pub(crate) i32);
@@ -28,6 +32,22 @@ pub(crate) fn err_msg<S: Into<String>>(s: S) -> Box<dyn Error> {
     Box::new(ErrMsg(s.into()))
 }
 
+/// The insta version in the current workspace (i.e. not the `cargo-insta`
+/// binary that's running).
+fn read_insta_version() -> Result<Version, Box<dyn std::error::Error>> {
+    MetadataCommand::new()
+        .exec()?
+        .packages
+        .iter()
+        .find(|package| package.name == "insta")
+        .map(|package| package.version.clone())
+        .ok_or("insta not found in cargo metadata".into())
+}
+
+lazy_static! {
+    pub static ref INSTA_VERSION: Version = read_insta_version().unwrap();
+}
+
 /// cargo-insta version
 // We could put this in a lazy_static
 pub(crate) fn cargo_insta_version() -> String {

cargo-insta/tests/main.rs

@@ -756,12 +756,6 @@ fn test_virtual_manifest_single_crate() {
 
 #[test]
 fn test_force_update_snapshots() {
-    // We test with both 1.39 and `master`. These currently test the same code!
-    // But I copied the test from
-    // https://github.com/mitsuhiko/insta/pull/482/files where they'll test
-    // different code. If we don't end up merging that, we can remove one of the
-    // tests (but didn't think it was worthwhile to do the work to then undo it)
-
     fn create_test_force_update_project(name: &str, insta_dependency: &str) -> TestProject {
         TestFiles::new()
             .add_file(
@@ -811,7 +805,7 @@ Hello, world!
 
     let test_current_insta =
         create_test_force_update_project("current", "{ path = '$PROJECT_PATH' }");
-    let test_insta_1_39_0 = create_test_force_update_project("1_39_0", "\"1.39.0\"");
+    let test_insta_1_40_0 = create_test_force_update_project("1_40_0", "\"1.40.0\"");
 
     // Test with current insta version
     let output_current = test_current_insta
@@ -822,14 +816,14 @@ Hello, world!
 
     assert_success(&output_current);
 
-    // Test with insta 1.39.0
-    let output_1_39_0 = test_insta_1_39_0
+    // Test with insta 1.40.0
+    let output_1_40_0 = test_insta_1_40_0
         .cmd()
         .args(["test", "--accept", "--force-update-snapshots"])
         .output()
         .unwrap();
 
-    assert_success(&output_1_39_0);
+    assert_success(&output_1_40_0);
 
     // Check that both versions updated the snapshot correctly
     assert_snapshot!(test_current_insta.diff("src/snapshots/test_force_update_current__force_update.snap"), @r#"
@@ -847,9 +841,9 @@ Hello, world!
     -
     "#);
 
-    assert_snapshot!(test_insta_1_39_0.diff("src/snapshots/test_force_update_1_39_0__force_update.snap"), @r#"
-    --- Original: src/snapshots/test_force_update_1_39_0__force_update.snap
-    +++ Updated: src/snapshots/test_force_update_1_39_0__force_update.snap
+    assert_snapshot!(test_insta_1_40_0.diff("src/snapshots/test_force_update_1_40_0__force_update.snap"), @r#"
+    --- Original: src/snapshots/test_force_update_1_40_0__force_update.snap
+    +++ Updated: src/snapshots/test_force_update_1_40_0__force_update.snap
     @@ -1,8 +1,5 @@
     -
      ---

insta/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "insta"
-version = "1.40.0"
+version = "1.41.0"
 license = "Apache-2.0"
 authors = ["Armin Ronacher <[email protected]>"]
 description = "A snapshot testing library for Rust"

insta/src/env.rs

@@ -4,8 +4,11 @@ use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 use std::{env, fmt, fs};
 
-use crate::content::{yaml, Content};
 use crate::utils::is_ci;
+use crate::{
+    content::{yaml, Content},
+    elog,
+};
 
 lazy_static::lazy_static! {
     static ref WORKSPACES: Mutex<BTreeMap<String, Arc<PathBuf>>> = Mutex::new(BTreeMap::new());
@@ -64,6 +67,7 @@ pub enum SnapshotUpdate {
     Unseen,
     New,
     No,
+    Force,
 }
 
 #[derive(Debug)]
@@ -96,7 +100,6 @@ impl std::error::Error for Error {
 /// Represents a tool configuration.
 #[derive(Debug)]
 pub struct ToolConfig {
-    force_update_snapshots: bool,
     force_pass: bool,
     require_full_match: bool,
     output: OutputBehavior,
@@ -146,26 +149,44 @@ impl ToolConfig {
         }
         let cfg = cfg.unwrap_or_else(|| Content::Map(Default::default()));
 
-        // support for the deprecated environment variable.  This is implemented in a way that
-        // cargo-insta can support older and newer insta versions alike.  It will set both
-        // variables.  However if only `INSTA_FORCE_UPDATE_SNAPSHOTS` is set, we will emit
-        // a deprecation warning.
-        if env::var("INSTA_FORCE_UPDATE").is_err() {
-            if let Ok("1") = env::var("INSTA_FORCE_UPDATE_SNAPSHOTS").as_deref() {
-                eprintln!("INSTA_FORCE_UPDATE_SNAPSHOTS is deprecated, use INSTA_FORCE_UPDATE");
-                env::set_var("INSTA_FORCE_UPDATE", "1");
-            }
+        // Support for the deprecated environment variables.  This is
+        // implemented in a way that cargo-insta can support older and newer
+        // insta versions alike. Versions of `cargo-insta` <= 1.39 will set
+        // `INSTA_FORCE_UPDATE_SNAPSHOTS` & `INSTA_FORCE_UPDATE`.
+        //
+        // If `INSTA_FORCE_UPDATE_SNAPSHOTS` is the only env var present we emit
+        // a deprecation warning, later to be expanded to `INSTA_FORCE_UPDATE`.
+        //
+        // Another approach would be to pass the version of `cargo-insta` in a
+        // `INSTA_CARGO_INSTA_VERSION` env var, and then raise a warning unless
+        // running under cargo-insta <= 1.39. Though it would require adding a
+        // `semver` dependency to this crate or doing the version comparison
+        // ourselves (a tractable task...).
+        let force_update_old_env_vars = if let Ok("1") = env::var("INSTA_FORCE_UPDATE").as_deref() {
+            // Don't raise a warning yet, because recent versions of
+            // `cargo-insta` use this, so that it's compatible with older
+            // versions of `insta`.
+            //
+            //   elog!("INSTA_FORCE_UPDATE is deprecated, use
+            //   INSTA_UPDATE=force");
+            true
+        } else if let Ok("1") = env::var("INSTA_FORCE_UPDATE_SNAPSHOTS").as_deref() {
+            // Warn on an old envvar.
+            //
+            // There's some possibility that we're running from within an fairly
+            // old version of `cargo-insta` (before we added an
+            // `INSTA_CARGO_INSTA` env var, so we can't pick that up.) So offer
+            // a caveat in that case.
+            elog!("INSTA_FORCE_UPDATE_SNAPSHOTS is deprecated, use INSTA_UPDATE=force. (If running from `cargo insta`, no action is required; upgrading `cargo-insta` will silence this warning.)");
+            true
+        } else {
+            false
+        };
+        if force_update_old_env_vars {
+            env::set_var("INSTA_UPDATE", "force");
         }
 
         Ok(ToolConfig {
-            force_update_snapshots: match env::var("INSTA_FORCE_UPDATE").as_deref() {
-                Err(_) | Ok("") => resolve(&cfg, &["behavior", "force_update"])
-                    .and_then(|x| x.as_bool())
-                    .unwrap_or(false),
-                Ok("0") => false,
-                Ok("1") => true,
-                _ => return Err(Error::Env("INSTA_FORCE_UPDATE")),
-            },
             require_full_match: match env::var("INSTA_REQUIRE_FULL_MATCH").as_deref() {
                 Err(_) | Ok("") => resolve(&cfg, &["behavior", "require_full_match"])
                     .and_then(|x| x.as_bool())
@@ -203,6 +224,14 @@ impl ToolConfig {
                 let val = match env_var.as_deref() {
                     Err(_) | Ok("") => resolve(&cfg, &["behavior", "update"])
                         .and_then(|x| x.as_str())
+                        // Legacy support for the old force update config
+                        .or(resolve(&cfg, &["behavior", "force_update"]).and_then(|x| {
+                            elog!("`force_update: true` is deprecated in insta config files, use `update: force`");
+                            match x.as_bool() {
+                                Some(true) => Some("force"),
+                                _ => None,
+                            }
+                        }))
                         .unwrap_or("auto"),
                     Ok(val) => val,
                 };
@@ -212,6 +241,7 @@ impl ToolConfig {
                     "new" => SnapshotUpdate::New,
                     "unseen" => SnapshotUpdate::Unseen,
                     "no" => SnapshotUpdate::No,
+                    "force" => SnapshotUpdate::Force,
                     _ => return Err(Error::Env("INSTA_UPDATE")),
                 }
             },
@@ -278,11 +308,6 @@ impl ToolConfig {
 
     // TODO: Do we want all these methods, vs. just allowing access to the fields?
 
-    /// Is insta told to force update snapshots?
-    pub fn force_update_snapshots(&self) -> bool {
-        self.force_update_snapshots
-    }
-
     /// Should we fail if metadata doesn't match?
     pub fn require_full_match(&self) -> bool {
         self.require_full_match
@@ -380,6 +405,7 @@ pub fn snapshot_update_behavior(tool_config: &ToolConfig, unseen: bool) -> Snaps
         }
         SnapshotUpdate::New => SnapshotUpdateBehavior::NewFile,
         SnapshotUpdate::No => SnapshotUpdateBehavior::NoUpdate,
+        SnapshotUpdate::Force => SnapshotUpdateBehavior::InPlace,
     }
 }
 

insta/src/lib.rs

@@ -129,6 +129,7 @@
 //! - `unseen`: `always` for previously unseen snapshots or `new` for existing
 //!   snapshots
 //! - `no`: does not write to snapshot files at all; just runs tests
+//! - `force`: forcibly updates snapshot files, even if assertions pass
 //!
 //! When `new`, `auto` or `unseen` is used, the
 //! [`cargo-insta`](https://crates.io/crates/cargo-insta) command can be used to
@@ -207,16 +208,14 @@
 //!
 //! ```yaml
 //! behavior:
-//!   # also set by INSTA_FORCE_UPDATE
-//!   force_update: true/false
 //!   # also set by INSTA_REQUIRE_FULL_MATCH
 //!   require_full_match: true/false
 //!   # also set by INSTA_FORCE_PASS
 //!   force_pass: true/false
 //!   # also set by INSTA_OUTPUT
 //!   output: "diff" | "summary" | "minimal" | "none"
 //!   # also set by INSTA_UPDATE
-//!   update: "auto" | "always" | "new" | "unseen" | "no"
+//!   update: "auto" | "new" | "always" | "no" | "unseen" | "force"
 //!   # also set by INSTA_GLOB_FAIL_FAST
 //!   glob_fail_fast: true/false
 //!