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.

Sign up for GitHub

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

Jump to bottom

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

Open
ilyagr opened this issue Feb 14, 2024 · 5 comments
Open

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

ilyagr opened this issue Feb 14, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@ilyagr
Copy link

ilyagr commented Feb 14, 2024 •
edited

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.
@ilyagr ilyagr added the bug Something isn't working label Feb 14, 2024
@mitsuhiko
Copy link
Owner

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
Copy link
Author

ilyagr commented Feb 19, 2024

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.

@ilyagr ilyagr mentioned this issue Feb 23, 2024
Closed
@max-sixty
Copy link
Sponsor Contributor

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
Copy link
Owner

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

@mitsuhiko mitsuhiko mentioned this issue Mar 3, 2024
Closed
@ilyagr
Copy link
Author

ilyagr commented Mar 3, 2024

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

@mitsuhiko mitsuhiko mentioned this issue Mar 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Bump serialization formats to newer versions mitsuhiko/insta
Insta 2 mitsuhiko/insta
3 participants
@mitsuhiko @ilyagr @max-sixty