serde-rs/test

Struct length is ignored by `assert_de_tokens`

peterjoel opened this issue · 3 comments

I just noticed this anomaly in the code I provided for a different issue.

#[derive(Deserialize)]
#[serde(rename = "Message")]
struct Flattened {
    a: String,
    b: String,
}

impl Flattened {
    fn new(a: &str, b: &str) -> Flattened {
        Flattened {
            a: a.to_string(),
            b: b.to_string(),
        }
    }
}

#[test]
fn both_deserialize_as_flat_struct_tokens() {
    let expected_tokens = &[
        Token::Struct { name: "Message", len: 5 },
        Token::Str("a"),
        Token::Str("v1"),
        Token::Str("b"),
        Token::Str("99"),
        Token::StructEnd,
    ];

    let flattened = Flattened::new("v1", "99");
    assert_de_tokens(&flattened, expected_tokens);
}

The struct only has two fields, but the test is checking for len: 5, and still passes.

Mingun commented

@dtolnay, it seems that this issue should be moved to https://github.com/serde-rs/test.

Actually, I think, it should be just closed, because len is just a hint from a deserializer to a type being deserialized, and not a strict requirement

Only assert_ser_tokens pays attention to this len, comparing it against the len passed to serialize_struct, while assert_de_tokens just ignores it, as you found.

One might expect assert_de_tokens to compare this len against the number of field names passed by the Deserialize impl to deserialize_struct.

The reason that is not a good option is because sometimes these 2 disagree.

For example, this struct always has 3 possible field names for deserialization, but always serializes 2 fields.

#[derive(Serialize, Deserialize)]
pub struct Demo {
    a: i32,
    #[serde(alias = "c")]
    b: i32,
}

As one counterproposal perhaps assert_de_tokens should pay attention and assert_tokens should not. Currently assert_tokens is equivalent to assert_ser_tokens + assert_de_tokens and I think we are better off keeping it that way.

Another possibility is make assert_de_tokens compare the len not against the field names of the deserialize_struct call, but against the number of fields actually present in the test tokens list. I think that's not so useful; the point is to test the Deserialize impl, not test the test code. This only makes the test code more tedious.

Related: #15