Better ways of dealing with snapshots in loops or helper functions

[ilyagr]

I have a few thoughts about 745b45b.

Firstly, the error message saying Insta does not allow inline snapshot assertions in loops isn't quite right. The new panic can happen without any loops:

fn helper(i:i64) {
   assert_snapshot!(i.to_string(), @"1");
}

#[test]
fn a_test() {
    helper(1);
    helper(2);
}

Here's how I encountered this situation.

Secondly, a friendlier behavior would be to not have an error merely because the snapshot at the same line was encountered twice. Rather, insta could error out only if , when this snapshot is repeatedly encountered, insta sees a different string compared to it than it saw before. Then, tests that were correct before wouldn't start failing because of upgrading insta.

In other words, the example above would have insta abort and report an error. But if we replace helper with

fn helper(i:i64) {
   do_something_complicated(i);
   assert_snapshot!(mission_report(), @"Total success!");
}

the test should be valid. Ideally (but not if it's too much trouble to implement), if mission_report() is modified to report "Complete success" both times, insta could handle that kind of change and allow the user to use the normal cargo insta review functionality to fix the test.

It's hard for me to tell how much effort it would take to make this happen. Is this the kind of thing you would accept a PR for? (No promises at this point, though) Do you think it's not worth the effort?

[mitsuhiko]

It could be changed so that it only complains if the assertion fails, but I'm not convinced that is a good idea. A snapshot test that should be consistent across multiple runs smells. What if the runs are not consistent and you get three different results? Particularly with cargo insta test this would create three inline update records which was not really intended.

[ilyagr]

A snapshot test that should be consistent across multiple runs smells.

I guess I don't feel the same way. This is the only kind of snapshot that insta can support across multiple runs, and that seems OK to me.

What if the runs are not consistent and you get three different results?

The idea is that once insta sees two different results, it treats that situation that the same as any other test failure that cannot be fixed by editing a golden test snapshot. Ideally, insta would report a detailed error showing both versions it saw and either panic or otherwise abort the test. It shouldn't try to create any update records at this point.

---

Update/clarification: This is not super-important, but to add on to my last sentence: ideally, the first time insta sees that snapshot, we could allow it to update the record if it sees a diff from what the assertion expected. The second time insta sees the same snapshot, it would always either do nothing (if it seems the same thing as the first time) or abort.

However, it would be OK if insta wasn't quite that smart and aborted on the second pass whenever it sees any diff from what the assertion expected (regardless of whether what it sees is the same as on the first pass).

[mitsuhiko]

For instance to detect all potential runs of a snapshot test it would require functionality that rust-test just does not provide. I'm not sure what it can realistically do in that scenario.

[ilyagr]

I'm not sure I understand your concern, forgive me if I'm missing some obvious flaw in my design. I wasn't suggesting that insta tries to predict whether a snapshot can or cannot be encountered multiple times, and I don't think that's necessary for it to work.

By the way, I'm not trying to present this as a particularly serious or urgent problem. In the one case this was a problem for me, it was just a small annoyance. The workaround of replacing insta::assert_snapshot with assert_eq! was fine. Its main downside is that it may potentially make the life of someone breaking the test slightly harder, but the difference would be slight. I have no idea if this is/can become a problem without an easy workaround for someone (or a future me).

Also, thanks for making insta! It's a neat and really useful tool.

[mitsuhiko]

@ilyagr the proposal if I understand it correctly is that Jinja collects all (or at least two) snapshots that diverge:

Ideally, insta would report a detailed error showing both versions it saw and either panic or otherwise abort the test. It shouldn't try to create any update records at this point.

The way inline snapshots work is that they append themselves to a pending snapshots log file where the latest ones one. Previous snapshots cannot be distinguished form previous runs or previous invocations in the same run reliably due to lack of hooks in rust-test.

By the way, I'm not trying to present this as a particularly serious or urgent problem. In the one case this was a problem for me, it was just a small annoyance. The workaround of replacing insta::assert_snapshot with assert_eq! was fine.

In a way there is an easier solution I believe which is to have the helper add to a HashSet and assert on the contents of it.

[ilyagr]

You are correct about my proposal. I didn't quite understand your explanation (I didn't experience insta having any trouble restricting itself to snapshots from latest invocation), but I am sure I can figure it out if I put a bit of time into it. In any case, I accept that there is indeed some issue I haven't considered. :)

In a way there is an easier solution I believe which is to have the helper add to a HashSet and assert on the contents of it.

That's a good point. I don't actually know how to use snapshots that are HashSet-s or vectors, as opposed to strings, but I'll look at the docs. If that's easy enough, I like this approach.

Update: On second thought, using a HashSet would lose the information of which call to the function failed. Using a Vector is just a more complicated version of returning the string from the helper function and asserting on that. (In my case, I wanted to avoid that because the error message had little to do with the purpose of a function, but it wouldn't be the end of the world either).

Thanks again!

[mitsuhiko]

A hypothetical solution would be to explicitly handle this. eg such a system could be added:

insta::multi_snapshots! {
    helper(1);
    helper(2);
}

Since we control the scope, the end of the multi_snapshots! block could do the necessary assertions. But that's probably a larger change.

[irevoire]

I was wondering about the exact same thing while rewriting our test suite with more insta everywhere.
I just wanted to share my use case without diving into the implementation details.

At meilisearch, we use insta a lot to ensure we return the expected error message.
And one route we work with a lot is our search route. Since this route can be accessed both with GET query parameter and POST payload, we made a function that takes a serde_json::Value and serializes it as a query parameter and a json payload.

Since both routes are supposed to work similarly, we apply the same asserts.
Here is what it looks like in practice;

#[actix_rt::test]
async fn search_unexisting_index() {
    let server = Server::new().await;
    let index = server.index("test");

    let expected_response = json!({
        "message": "Index `test` not found.",
        "code": "index_not_found",
        "type": "invalid_request",
        "link": "https://docs.meilisearch.com/errors#index_not_found"
    });

    index
        .search(json!({"q": "hello"}), |response, code| {
            assert_eq!(code, 404);
            assert_eq!(response, expected_response);
        })
        .await;
}

And what I would like to write instead;

#[actix_rt::test]
async fn search_unexisting_index() {
    let server = Server::new().await;
    let index = server.index("test");

    index
        .search(json!({"q": "hello"}), |response, code| {
            assert_display_snapshot!(code, @"404");
            assert_json_snapshot!(response, @"...");
        })
        .await;
}

[mitsuhiko]

If that functionality were to be added, would this be an acceptable way to express that desire?

#[actix_rt::test]
async fn search_unexisting_index() {
    let server = Server::new().await;
    let index = server.index("test");

    insta::multi_snapshots! {
        index
            .search(json!({"q": "hello"}), |response, code| {
                assert_display_snapshot!(code, @"404");
                assert_json_snapshot!(response, @"...");
            })
            .await;
    }
}

Or alternatively probably that could be a setting:

#[actix_rt::test]
async fn search_unexisting_index() {
    let server = Server::new().await;
    let index = server.index("test");
    let mut settings = Settings::clone_current();
    settings.set_multi_snapshot(true);
    let _guard = settings.bind_to_scope()

    index
        .search(json!({"q": "hello"}), |response, code| {
            assert_display_snapshot!(code, @"404");
            assert_json_snapshot!(response, @"...");
        })
        .await;
}

[irevoire]

Yep definitely, both would work, I just wanted to share another "real" use case we encountered while using insta 😁

[mtbc]

For what it's worth, I just ran into the same issue. I have two tests that I want to be clear have the same first part of setup but a different conclusion. So, I had them both call the same helper to start out with, to show the difference is only toward the end. However, that helper setup uses insta::assert... to make sure the setup's going as expected, leaving me with that same "Insta does not allow inline snapshot assertions in loops" where there isn't a loop.

[mitsuhiko]

I want to implement this but I cannot come up with a good name for the macro that explains what it's actually doing. Here some options:

Option A

insta::assert_perfect_duplicates! {
    for x in (0..10).step_by(2) {
        let is_even = x % 2 == 0;
        insta::assert_debug_snapshot!(is_even, @"true");
    }
}

Option B

insta::assert_multi_snapshots! {
    for x in (0..10).step_by(2) {
        let is_even = x % 2 == 0;
        insta::assert_debug_snapshot!(is_even, @"true");
    }
}

Option C

insta::multi_snapshots! {
    for x in (0..10).step_by(2) {
        let is_even = x % 2 == 0;
        insta::assert_debug_snapshot!(is_even, @"true");
    }
}

Option D

insta::allow_matching_duplicates! {
    for x in (0..10).step_by(2) {
        let is_even = x % 2 == 0;
        insta::assert_debug_snapshot!(is_even, @"true");
    }
}

Option E

insta::allow_duplicates! {
    for x in (0..10).step_by(2) {
        let is_even = x % 2 == 0;
        insta::assert_debug_snapshot!(is_even, @"true");
    }
}

[irevoire]

Personally, I would say that since this macro isn't doing the assert directly, it shouldn't contain the word assert at all.

I'm not a fan of the word allow either, but I can't really explain why.
Thus, my favourite one is option C. Or duplicate_snapshot / perfect_duplicate_snapshot / matching_duplicates.

Hope that helps 😂

[mitsuhiko]

I'm a bit conflicted on the naming still. I at the moment think prefer allow_duplicates because it states that in that block duplicates are permitted. But not sure.

[max-sixty]

Naive question — would using the existing insta::with_settings! approach not work here? Possibly it's only for scoped sections and so making it a normal setting would be confusing?

[mitsuhiko]

It maintains a stack of previous run results. I would not want to abuse the settings for that since they are not necessarily scope bound.

[ilyagr]

allow_reentrant_snapshots is a bit verbose but descriptive.

[mitsuhiko]

@ilyagr reentrancy means that a function can be invoked again before it finished. This is not what is happening here so I think the name would not be correct.

[ilyagr]

allow_snapshot_reexecution?