mitsuhiko / insta

A snapshot testing library for rust
https://insta.rs
Apache License 2.0
2.19k stars 99 forks source link

`assert_toml_snapshot` produces invalid TOML when the `toml` library would generate an error. #439

Open ilyagr opened 8 months ago

ilyagr commented 8 months ago

What happened?

Here is a demo (using toml, maplit and insta {features=["serde"]} crates. Note how the first assert_toml_snaphot produces invalid TOML. The rest of the example is simply to provide some context of how the TOML library errors out and to confirm that JSON is generated correctly:

    use std::collections::BTreeMap;

    use maplit::btreemap;

    #[test]
    fn toml() {
        let m: BTreeMap<&str, Vec<Option<&str>>> = btreemap! {
            "a" => vec![Some("something"), None, Some("something else")]
        };
        // BUG!
        insta::assert_toml_snapshot!(m, 
        @r###"
        a = [
            'something'
        "###);

        // JSON can be generated fine
        insta::assert_json_snapshot!(m, 
        @r###"
        {
          "a": [
            "something",
            null,
            "something else"
          ]
        }
        "###);

       // The `toml` library generates an error in this case
        insta::assert_debug_snapshot!(toml::ser::to_string(&m), @r###"
        Err(
            Error {
                inner: UnsupportedNone,
            },
        )
        "###);

    }

Reproduction steps

No response

Insta Version

1.34.0

rustc Version

1.78.0-nightly (d44e3b95c 2024-02-09)

What did you expect?

I guess assert_toml_snapshot should have panicked.

mitsuhiko commented 8 months ago

This is an issue with the old version of toml that insta depends on. Upgrading to a newer one will unfortunately break all snapshots because the format of strings changed 'foo' => "foo". Not sure what is the best course of action here.

ilyagr commented 8 months ago

One possibility that comes to mind is to introduce a new feature to the crate along the lines of toml_updated. Then, new users can use it and old user can consciously upgrade. If there's ever a major version bump for insta, it can replace the current toml feature.

I'm not sure if it's worth the trouble, though.

max-sixty commented 8 months ago

The new feature is clever...

...though from someone who is not a maintainer (but a keen advocate and has contributed over the years) — a breaking change seems fairly reasonable. Cargo's dependency management means that it's a single commit; it's not like different contributors are clashing on different versions...

mitsuhiko commented 7 months ago

I wonder if the right course of action here is a major version of insta that bumps up these old libraries.

ilyagr commented 7 months ago

That sounds great to me, if you are willing. I am not sure whether or not this would inconvenience others.