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.

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

Struct length is ignored by assert_de_tokens #7

Closed
peterjoel opened this issue May 20, 2019 · 3 comments
Closed

Struct length is ignored by assert_de_tokens #7

peterjoel opened this issue May 20, 2019 · 3 comments
Labels
bug Something isn't working

Comments

@peterjoel
Copy link

peterjoel commented May 20, 2019

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.

@dtolnay dtolnay added the bug Something isn't working label Jul 9, 2023
@Mingun
Copy link
Contributor

Mingun commented Aug 10, 2023

@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

@dtolnay dtolnay transferred this issue from serde-rs/serde Aug 10, 2023
@dtolnay
Copy link
Member

dtolnay commented Aug 10, 2023

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.

@dtolnay dtolnay closed this as completed Aug 10, 2023
@dtolnay
Copy link
Member

dtolnay commented Aug 10, 2023

Related: #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants