Skip to content

Commit

Permalink
test: migrate the remaining snapshot-test to insta (obi1kenobi#951)
Browse files Browse the repository at this point in the history 
* moved the query execution snapshot to a separate place

* made sure that the formatting is the same for the moved snaps as from insta

* adjust the testcase to work via insta instead of manually implementing similar logic

* add the headers which insta creates

* documented the change in the `CONTRIBUTING.md`

* Made sure that unrelated snapshots are passing CI

* change the wording in the contribution guide as requested

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

* change the wording in the contribution guide as requested

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

* Reworded a part of the contribution guide

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

* applied a nitpic

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

* Fixed a typo

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>

* Revert "Made sure that unrelated snapshots are passing CI"

This reverts commit f0f7d9c.

* Changed the documentation to align with the change in the testcase

* added a testcase against `.snap.new` files existing

* fixed linting issues

* removed something which I thought of as a simple typo fix

* moved testcases

* made sure that the CONTRIBUTING.md guide is better formatted

* alligned more with other code

* fixed a typo

* Apply suggestions from code review

---------

Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
  • Loading branch information
@CommanderStorm @obi1kenobi
CommanderStorm and obi1kenobi authored Oct 2, 2024
1 parent cb75bbe commit 868b095
Show file tree
Hide file tree
Showing 186 changed files with 6,482 additions and 6,006 deletions.
137 changes: 100 additions & 37 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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!
Expand Down Expand Up @@ -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
Expand All @@ -205,8 +269,11 @@ 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!

Expand Down Expand Up @@ -269,18 +336,14 @@ 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**;
Expand Down
6 changes: 5 additions & 1 deletion scripts/make_new_lint.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
70 changes: 28 additions & 42 deletions src/query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)| {
(
Expand All @@ -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();
Expand All @@ -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)| {
(
Expand Down
61 changes: 61 additions & 0 deletions src/snapshot_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?"
);
}
Loading

0 comments on commit 868b095

Please sign in to comment.