mitsuhiko / insta

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

BTreeMap json-serialization not supported #689

Open AlisCode opened 4 hours ago

AlisCode commented 4 hours ago

What happened?

It looks like insta does not support serializing the type BTreeMap<K, V> when K is a custom enum. serde_json does that fine, though.

Reproduction steps

Here is an example that reproduces the issue.

use serde::Serialize;

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)]
pub enum MyEnum {
    A,
    B,
    C,
}

#[test]
fn can_serialize() {
    let map = maplit::btreemap! {
        MyEnum::A => true,
        MyEnum::B => false,
        MyEnum::C => true,
    };
    // serde_json can serialize that BTreeMap
    assert!(serde_json::to_string_pretty(&map).is_ok());
}

#[test]
fn assert_get_map() {
    let map = maplit::btreemap! {
        MyEnum::A => true,
        MyEnum::B => false,
        MyEnum::C => true,
    };
    insta::assert_json_snapshot!(map);
}

The serialization through serde works fine, but insta fails to serialize with the following error :

.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.41.1/src/content/json.rs:188:25:
cannot serialize maps without string keys to JSON

Insta Version

1.41.1

rustc Version

1.80.1

What did you expect?

I expect my BTreeMap to be correctly serialized in JSON and a snapshot to be generated

mitsuhiko commented 3 hours ago

The key that it tries to serialize is a Content::UnitVariant("MyEnum", 0, "A"). I'm not sure what the right thing is to do here. Maybe format it as MyEnum::A? I don't think we can reliably match what serde would do normally.

AlisCode commented 2 hours ago

serde_json serializes it as "A" for sure, and as "a" if using e.g. #[serde(rename_all = "camelCase")], but I see that adding the rename to MyEnum changes it to Content::UnitVariant("MyEnum", 0, "a"), so it looks like that 3rd argument is what we're looking for ? I dont think MyEnum::A is the right way to go.

What do you think of adding a case for UnitVariant in the code that serializes a map, like this :

diff --git a/insta/src/content/json.rs b/insta/src/content/json.rs
index e4dd5e6..c03556c 100644
--- a/insta/src/content/json.rs
+++ b/insta/src/content/json.rs
@@ -180,6 +180,8 @@ impl Serializer {
                     let real_key = key.resolve_inner();
                     if let Content::String(ref s) = real_key {
                         self.write_escaped_str(s);
+                    } else if let Content::UnitVariant(_, _, s) = real_key {
+                        self.write_escaped_str(s);
                     } else if let Some(num) = real_key.as_i64() {
                         self.write_escaped_str(&num.to_string());
                     } else if let Some(num) = real_key.as_i128() {

I dont know if that would solve every other use cases, but it does work for the use case of this bug. If you approve of that idea, I'll happily create a PR :)

I don't think we can reliably match what serde would do normally.

I was surprised to learn that insta is not using serde_json under the hood, I wonder what the rationale is ?