test: migrate the remaining snapshot-test to insta

CONTRIBUTING.md

@@ -111,6 +111,37 @@ To generate this data, please run `./scripts/regenerate_test_rustdocs.sh`.
 To use a specific toolchain, like beta or nightly, pass it as
 an argument: `./scripts/regenerate_test_rustdocs.sh +nightly`.
 
+## What are those `.snap` or `.snap.new` files generated via `cargo test` 
+
+As part of our overall testing strategy, we use a technique called "snapshot testing."
+The tool we use for this ([`insta`](https://insta.rs/docs/)) is user friendly and has mutliple ways to interact with it:
+
+These snapshots are by default written to `.snap.new` files (because `INSTA_UPDATE` explained below defaults to `auto`) if they differ and fail the testcase.
+Reviewing them is possible via these options:
+
+1. **With `cargo-insta`**: If you install (or have already installed) the `insta` CLI with
+   `cargo install --locked cargo-insta`, you can run `cargo insta review`. Check that the
+   new output is what you expect, and accept it in the TUI.
+2. **Without `cargo-insta`**:
+   From [`insta`s docs](https://insta.rs/docs/quickstart/#tests-without-insta):
+   > You can also just use insta directly from cargo test and control it via the `INSTA_UPDATE` environment variable.
+   > The default is auto which will write all new snapshots into .snap.new files if no CI is detected so that cargo-insta can pick them up. The following other modes are possible:
+   > - `auto`: the default. no for CI environments or new otherwise
+   > - `always`: overwrites old snapshot files with new ones unasked
+   > - `unseen`: behaves like always for new snapshots and new for others
+   > - `new`: write new snapshots into .snap.new files
+   > - `no`: does not update snapshot files at all (just runs tests)
+
+   Thus, if you run the following command, you can accept the current snapshots after reviewing the `.snap.new` files.
+   ```text
+   INSTA_UPDATE=always cargo test
+   ```
+3. **Manually**: If you can't (or don't want to) use `cargo-insta`, you can verify the snapshot
+   file manually.  There should be a file called `test_outputs/<some_path>/<lint_name>.snap.new`.
+   Open it, and verify that its contents match what you expected: all expected data is present, and no unexpected data is included. 
+   Once you've checked it, remove the `.new` suffix so that the file's new path
+   is `test_outputs/<some_path>/<lint_name>.snap`
+
 ## Adding a new lint
 
 ### Background
@@ -136,7 +167,7 @@ First, choose an appropriate name for your lint. We'll refer to it as `<lint_nam
 We'll use the [`scripts/make_new_lint.sh`](https://github.com/obi1kenobi/cargo-semver-checks/tree/main/scripts/make_new_lint.sh) script to automatically create the necessary file stubs, which you'll then fill in. It will:
 - Add a new lint file: `src/lints/<lint_name>.ron`.
 - Create a new test crate pair: `test_crates/<lint_name>/old` and `test_crates/<lint_name>/new`.
-- Add an empty expected test outputs file: `test_outputs/<lint_name>.output.ron`.
+- Add an empty expected test outputs file: `test_outputs/query_execution/<lint_name>.snap`.
 - Register your new lint in the `add_lints!()` macro near the bottom of [`src/query.rs`](https://github.com/obi1kenobi/cargo-semver-checks/tree/main/src/query.rs).
 
 Now it's time to fill in these files!
@@ -171,32 +202,65 @@ and probably isn't working quite right.
 For a lint named `enum_struct_variant_field_added`, you'll probably see its test fail with
 a message similar to this:
 ```
-Query enum_struct_variant_field_added produced incorrect output (./src/lints/enum_struct_variant_field_added.ron).
-
-Expected output (./test_outputs/enum_struct_variant_field_added.output.ron):
-{
-    "./test_crates/enum_struct_variant_field_added/": [],
-}
-
-Actual output:
-{
-    "./test_crates/enum_struct_variant_field_added/": [
-        {
-            "enum_name": String("PubEnum"),
-            "field_name": String("y"),
-            "path": List([
-                String("enum_struct_variant_field_added"),
-                String("PubEnum"),
-            ]),
-            "span_begin_line": Uint64(4),
-            "span_filename": String("src/lib.rs"),
-            "variant_name": String("Foo"),
-        },
-    ],
-}
+---- query::tests_lints::enum_struct_variant_field_added stdout ----
+━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+Snapshot file: src/../test_outputs/query_execution/enum_struct_variant_field_added.snap
+Snapshot: enum_struct_variant_field_added
+Source: src/query.rs:646
+────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
+Expression: &query_execution_results
+────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
+-old snapshot
++new results
+────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
+    0     0 │ {
+    1       │-  "./test_crates/enum_struct_variant_field_added/": []
+          1 │+  "./test_crates/enum_no_repr_variant_discriminant_changed/": [
+          2 │+    {
+          3 │+      "enum_name": String("UnitOnlyBecomesUndefined"),
+          4 │+      "field_name": String("a"),
+          5 │+      "path": List([
+          6 │+        String("enum_no_repr_variant_discriminant_changed"),
+          7 │+        String("UnitOnlyBecomesUndefined"),
+          8 │+      ]),
+          9 │+      "span_begin_line": Uint64(77),
+         10 │+      "span_filename": String("src/lib.rs"),
+         11 │+      "variant_name": String("Struct"),
+         12 │+    },
+         13 │+  ],
+         14 │+  "./test_crates/enum_struct_field_hidden_from_public_api/": [
+         15 │+    {
+         16 │+      "enum_name": String("AddedVariantField"),
+         17 │+      "field_name": String("y"),
+         18 │+      "path": List([
+         19 │+        String("enum_struct_field_hidden_from_public_api"),
+         20 │+        String("AddedVariantField"),
+         21 │+      ]),
+         22 │+      "span_begin_line": Uint64(38),
+         23 │+      "span_filename": String("src/lib.rs"),
+         24 │+      "variant_name": String("StructVariant"),
+         25 │+    },
+         26 │+  ],
+         27 │+  "./test_crates/enum_struct_variant_field_added/": [
+         28 │+    {
+         29 │+      "enum_name": String("PubEnum"),
+         30 │+      "field_name": String("y"),
+         31 │+      "path": List([
+         32 │+        String("enum_struct_variant_field_added"),
+         33 │+        String("PubEnum"),
+         34 │+      ]),
+         35 │+      "span_begin_line": Uint64(4),
+         36 │+      "span_filename": String("src/lib.rs"),
+         37 │+      "variant_name": String("Foo"),
+         38 │+    },
+         39 │+  ],
+    2    40 │ }
+────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
+To update snapshots run `cargo insta review`
+Stopped on the first failure. Run `cargo insta test` to run all snapshots.
 ```
 
-Inspect the "actual" output:
+Inspect the generated "actual" output in the `.snap.new` file:
 - Does it report the semver issue your lint was supposed to catch? If not, the lint query
   or the test crates' code may need to be tweaked.
 - Does it report correct span information? Is the span as specific as possible, for example
@@ -205,8 +269,9 @@ Inspect the "actual" output:
   If so, ensure the reported code is indeed violating semver and is not being flagged
   by any other lint.
 
-If everything looks okay, edit your `test_outputs/<lint_name>.output.ron` file adding
-the "actual" output, then re-run `cargo test` and make sure everything passes.
+If everything looks okay, either run `cargo insta review` (see the [snapshot instructions](#what-are-those-snap-or-snapnew-files-generated-via-cargo-test-) for context)
+or manually move `test_outputs/query_execution/<lint_name>.snap.new` to  `test_outputs/query_execution/<lint_name>.snap`.
+Then re-run `cargo test` and make sure everything passes.
 
 Congrats on the new lint!
 
@@ -269,18 +334,13 @@ time, run `cargo test` to start generating the snapshots.  The first time you ru
 it will fail, because there's no expected result to compare to.  Let's make the test pass:
 
 We use `insta` for snapshot testing witness results, so after adding/changing a witness, we need
-to update the test outputs. Note that it may contain output for other test crates - this
-is not necessarily an error: see the troubleshooting section for more info.
+to update the test outputs.
 
-There are two ways to update the output:
+> [!TIP]
+> It may contain output for other test crates — this is not necessarily an error:
+> See the [troubleshooting section](#troubleshooting) for more info.
 
-1. **With `cargo insta`**: If you install (or have already installed) the `insta` CLI with
-   `cargo install --locked cargo-insta`, you can run `cargo insta review`. Check that the
-   new output is what you expect, and accept it in the TUI.
-2. **Manually**: If you can't (or don't want to) use `cargo-insta`, you can verify the snapshot
-   file manually.  There should be a file called `test_outputs/witnesses/<lint_name>.snap.new`.
-   Open it, and verify that the witnesses generated as expected.  Once you've checked it, move it
-   to `test_outputs/witnesses/<lint_name>.snap` (remove the `.new`)
+To update the output, please refer to the section on [snapshot testing](#what-are-those-snap-or-snapnew-files-generated-via-cargo-test)
 
 Once you've update the test output, run `cargo test` again and the `<lint_name>` test should pass!
 **Make sure to commit and push the `test_outputs/witnesses/<lint_name>.snap` into git**;

scripts/make_new_lint.sh

@@ -65,12 +65,16 @@ NEW_LINT_TEST_CRATES_DIR="$TEST_CRATES_DIR/$NEW_LINT_NAME"
 ./scripts/make_new_test_crate.sh "$NEW_LINT_NAME"
 
 # Add the test outputs file.
-NEW_TEST_OUTPUT_FILE="$TEST_OUTPUTS_DIR/$NEW_LINT_NAME.output.ron"
+NEW_TEST_OUTPUT_FILE="$TEST_OUTPUTS_DIR/query_execution/$NEW_LINT_NAME.snap"
 echo -n "Creating test outputs file ${NEW_TEST_OUTPUT_FILE#"$TOPLEVEL/"} ..."
 if [[ -f "$NEW_TEST_OUTPUT_FILE" ]]; then
     echo ' already exists.'
 else
     cat <<EOF >"$NEW_TEST_OUTPUT_FILE"
+---
+source: src/query.rs
+expression: "&query_execution_results"
+---
 {
     "./test_crates/$NEW_LINT_NAME/": [
         // TODO

src/query.rs

@@ -590,14 +590,7 @@ mod tests {
         let query_text = std::fs::read_to_string(format!("./src/lints/{query_name}.ron")).unwrap();
         let semver_query = SemverQuery::from_ron_str(&query_text).unwrap();
 
-        let expected_result_text =
-            std::fs::read_to_string(format!("./test_outputs/{query_name}.output.ron"))
-            .with_context(|| format!("Could not load test_outputs/{query_name}.output.ron expected-outputs file, did you forget to add it?"))
-            .expect("failed to load expected outputs");
-        let mut expected_results: TestOutput = ron::from_str(&expected_result_text)
-            .expect("could not parse expected outputs as ron format");
-
-        let mut actual_results: TestOutput = get_test_crate_names()
+        let mut query_execution_results: TestOutput = get_test_crate_names()
             .iter()
             .map(|crate_pair_name| {
                 let (crate_old, crate_new) = get_test_crate_rustdocs(crate_pair_name);
@@ -629,42 +622,35 @@ mod tests {
             .filter(|(_crate_pair_name, output)| !output.is_empty())
             .collect();
 
-        // Reorder both vectors of results into a deterministic order that will compensate for
+        // Reorder vector of results into a deterministic order that will compensate for
         // nondeterminism in how the results are ordered.
-        let sort_individual_outputs = |results: &mut TestOutput| {
-            let key_func = |elem: &BTreeMap<String, FieldValue>| {
-                let filename = elem.get("span_filename").and_then(|value| value.as_str());
-                let line = elem.get("span_begin_line");
-
-                match (filename, line) {
-                    (Some(filename), Some(line)) => (filename.to_owned(), line.as_usize()),
-                    (Some(_filename), _) => panic!("A valid query must output `span_filename`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
-                    (_, Some(_line)) => panic!("A valid query must output `span_begin_line`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
-                    _ => panic!("A valid query must output both `span_filename` and `span_begin_line`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
-                }
-            };
-            for value in results.values_mut() {
-                value.sort_unstable_by_key(key_func);
+        let key_func = |elem: &BTreeMap<String, FieldValue>| {
+            let filename = elem.get("span_filename").and_then(|value| value.as_str());
+            let line = elem.get("span_begin_line");
+
+            match (filename, line) {
+                (Some(filename), Some(line)) => (filename.to_owned(), line.as_usize()),
+                (Some(_filename), None) => panic!("A valid query must output `span_filename`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
+                (None, Some(_line)) => panic!("A valid query must output `span_begin_line`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
+                (None, None) => panic!("A valid query must output both `span_filename` and `span_begin_line`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for details."),
             }
         };
-        sort_individual_outputs(&mut expected_results);
-        sort_individual_outputs(&mut actual_results);
-
-        if expected_results != actual_results {
-            panic!(
-                "\n{}\n",
-                pretty_format_output_difference(
-                    query_name,
-                    "expected",
-                    expected_results,
-                    "actual",
-                    actual_results
-                )
-            );
+        for value in query_execution_results.values_mut() {
+            value.sort_unstable_by_key(key_func);
         }
 
-        let transparent_actual_results: BTreeMap<_, Vec<BTreeMap<_, TransparentValue>>> =
-            actual_results
+        insta::with_settings!(
+            {
+                prepend_module_to_snapshot => false,
+                snapshot_path => "../test_outputs/query_execution",
+            },
+            {
+                insta::assert_ron_snapshot!(query_name, &query_execution_results);
+            }
+        );
+
+        let transparent_results: BTreeMap<_, Vec<BTreeMap<_, TransparentValue>>> =
+            query_execution_results
                 .into_iter()
                 .map(|(k, v)| {
                     (
@@ -678,9 +664,9 @@ mod tests {
 
         let registry = make_handlebars_registry();
         if let Some(template) = semver_query.per_result_error_template {
-            assert!(!transparent_actual_results.is_empty());
+            assert!(!transparent_results.is_empty());
 
-            let flattened_actual_results: Vec<_> = transparent_actual_results
+            let flattened_actual_results: Vec<_> = transparent_results
                 .iter()
                 .flat_map(|(_key, value)| value)
                 .collect();
@@ -693,7 +679,7 @@ mod tests {
         }
 
         if let Some(witness) = semver_query.witness {
-            let actual_witnesses: BTreeMap<_, BTreeSet<_>> = transparent_actual_results
+            let actual_witnesses: BTreeMap<_, BTreeSet<_>> = transparent_results
                 .iter()
                 .map(|(k, v)| {
                     (

src/snapshot_tests.rs

@@ -368,3 +368,64 @@ fn multiple_ambiguous_package_name_definitions() {
         ],
     );
 }
+
+/// Helper function which lists all files in the directory recursively.
+///
+/// # Arguments
+///
+/// - `path` is the path from which all [`std::fs::DirEntry`]'s will be expanded from
+fn recurse_list_files(path: impl AsRef<Path>) -> Vec<std::fs::DirEntry> {
+    let mut buf = vec![];
+    let entries = std::fs::read_dir(path).expect("failed to read the requested path");
+
+    for entry in entries {
+        let entry = entry.expect("failed to iterate due to intermittent IO errors");
+        let meta = entry.metadata().expect("failed to read file metadata");
+
+        if meta.is_dir() {
+            let mut subdir = recurse_list_files(entry.path());
+            buf.append(&mut subdir);
+        }
+        if meta.is_file() {
+            buf.push(entry);
+        }
+    }
+
+    buf
+}
+
+/// Make sure that no `.snap.new` snapshots exist.
+///
+/// This testcase exists as [`insta`] behaves as the following
+/// if seeing both a `.snap.new` and a `.snap` file:
+/// - If the content of both files is equal,
+///   it will pass without failure and remove `.snap.new`-file
+/// - If not, it will fail and show the diff
+///
+/// This behavior is problematic as
+/// - we might not pick up on a `.snap.new` file being added as the testcase pass in CI and
+/// - future contributors would be confused where this change comes from
+///
+/// Therefore, we are asserting that no such files exist.
+#[test]
+fn no_new_snapshots() {
+    let files = recurse_list_files("test_outputs/");
+    let new_snaps = files
+        .into_iter()
+        .map(|f| f.path())
+        .filter(|f| {
+            if let Some(name) = f.file_name().unwrap().to_str() {
+                return name.ends_with("snap.new");
+            }
+            false
+        })
+        .collect::<Vec<_>>();
+    assert_eq!(
+        new_snaps,
+        Vec::<PathBuf>::new(),
+        "`.snap.new` files exit, but should not. Did you\n\
+        - forget to run `cargo insta review` or\n\
+        - forget to move the `.snap.new` file to `.snap` after verifying \
+        the content is exactly as expected?"
+    );
+}