Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: migrate the remaining snapshot-test to insta #951

Merged
merged 21 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8a7e83a
moved the query execution snapshot to a separate place
CommanderStorm Sep 26, 2024
6e75e00
made sure that the formatting is the same for the moved snaps as from…
CommanderStorm Sep 26, 2024
487077c
adjust the testcase to work via insta instead of manually implementin…
CommanderStorm Sep 26, 2024
6de3bab
add the headers which insta creates
CommanderStorm Sep 26, 2024
0de8498
documented the change in the `CONTRIBUTING.md`
CommanderStorm Sep 27, 2024
090346b
Made sure that unrelated snapshots are passing CI
CommanderStorm Sep 26, 2024
31a53bc
change the wording in the contribution guide as requested
CommanderStorm Oct 1, 2024
711630a
change the wording in the contribution guide as requested
CommanderStorm Oct 1, 2024
bb6f363
Reworded a part of the contribution guide
CommanderStorm Oct 1, 2024
f081646
applied a nitpic
CommanderStorm Oct 1, 2024
4478097
Fixed a typo
CommanderStorm Oct 1, 2024
4f5676b
Revert "Made sure that unrelated snapshots are passing CI"
CommanderStorm Oct 1, 2024
53c008b
Changed the documentation to align with the change in the testcase
CommanderStorm Oct 1, 2024
6c4d33a
added a testcase against `.snap.new` files existing
CommanderStorm Oct 2, 2024
bed214c
fixed linting issues
CommanderStorm Oct 2, 2024
f5a8411
removed something which I thought of as a simple typo fix
CommanderStorm Oct 2, 2024
5d0fa74
moved testcases
CommanderStorm Oct 2, 2024
a84bde7
made sure that the CONTRIBUTING.md guide is better formatted
CommanderStorm Oct 2, 2024
783a94a
alligned more with other code
CommanderStorm Oct 2, 2024
69cdcc0
fixed a typo
CommanderStorm Oct 2, 2024
cb54a3c
Apply suggestions from code review
obi1kenobi Oct 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 98 additions & 38 deletions CONTRIBUTING.md
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`
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved

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,42 +202,76 @@ 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.
thread 'query::tests_lints::enum_struct_variant_field_added' panicked at /home/frank/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.40.0/src/runtime.rs:548:9:
```

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
pointing to a struct's field rather than the whole struct if the lint refers to that field?
pointing to a `struct`'s field rather than the whole struct if the lint refers to that field?
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved
- Does the output also report any code from test crates other than `test_crates/<lint_name>`?
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`.
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved
Then re-run `cargo test` and make sure everything passes.

Congrats on the new lint!

Expand Down Expand Up @@ -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**;
Expand Down
6 changes: 5 additions & 1 deletion scripts/make_new_lint.sh
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
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
Loading
Loading