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

Consolidate and add new tests of adjacently tagged enums #2804

Merged
merged 19 commits into from
Aug 23, 2024

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 16, 2024

Similar to #2788, this PR moves all tests which tests top-level adjacently tagged enums (those which is defined using #[serde(tag = "t", content = "c")] attribute) into a dedicated file, removes duplicated tests and organizes tests in a logical manner. Flatten tests when adjacently tagged enum is flattened into structure I consider foremost as flatten tests, so they left untouched.

Removed tests which tests deserialization of generated tag and content field names from integers and bytes for each enum variant kind, such tests leaved only for Unit variant. This is because they are handled by the either TagOrContentFieldVisitor or TagContentOtherFieldVisitor, which can be tested only once.

Added tests includes deserialization from sequential representation, so this part of generated code becomes tested:

serde/serde_derive/src/de.rs

Lines 1680 to 1708 in 1a9ffdb

fn visit_seq<__A>(self, mut __seq: __A) -> _serde::__private::Result<Self::Value, __A::Error>
where
__A: _serde::de::SeqAccess<#delife>,
{
// Visit the first element - the tag.
match _serde::de::SeqAccess::next_element(&mut __seq)? {
_serde::__private::Some(__field) => {
// Visit the second element - the content.
match _serde::de::SeqAccess::next_element_seed(
&mut __seq,
__Seed {
field: __field,
marker: _serde::__private::PhantomData,
lifetime: _serde::__private::PhantomData,
},
)? {
_serde::__private::Some(__ret) => _serde::__private::Ok(__ret),
// There is no second element.
_serde::__private::None => {
_serde::__private::Err(_serde::de::Error::invalid_length(1, &self))
}
}
}
// There is no first element.
_serde::__private::None => {
_serde::__private::Err(_serde::de::Error::invalid_length(0, &self))
}
}
}

Mingun and others added 19 commits August 16, 2024 21:36
…ed module

Moved and renamed:
From test_annotatons
- test_adjacently_tagged_enum_bytes               => bytes
- flatten::enum_::adjacently_tagged::straitforward=> struct_with_flatten
- test_expecting_message_adjacently_tagged_enum   => expecting_message
- test_partially_untagged_adjacently_tagged_enum  => partially_untagged

From test_macros
- test_adjacently_tagged_newtype_struct           => newtype_with_newtype
- test_adjacently_tagged_enum
- test_adjacently_tagged_enum_deny_unknown_fields => deny_unknown_fields
(review this commit with "ignore whitespace changes" option on)
Change 0i32 to 1u8 so the test can be merged with the previous in the next commit
`newtype` test also integrates test with `Bytes` tag, so be like

Removed the first assert_tokens because it is the same as the first assert in the merged method
(review this commit with "ignore whitespace changes" option on)
…ed for Unit case

There is no difference what variant is deserialized so we can test only one kind of variant
(review this commit with "ignore whitespace changes" option on)
(review this commit with "ignore whitespace changes" option on)
(review this commit with "ignore whitespace changes" option on)
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

These test suite PRs are much appreciated.

@dtolnay dtolnay merged commit 31ca16d into serde-rs:master Aug 23, 2024
15 checks passed
@Mingun Mingun deleted the adjacently-tagged-tests branch August 23, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants