Skip to content

Remove excessive serde tests and fix a bug #434

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

Merged
merged 6 commits into from
Jul 24, 2022
Merged

Remove excessive serde tests and fix a bug #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e7e4d4
Deduplicate test struct
Mingun Jul 20, 2022
11d8c1f
Remove `collection_of_enums` test because it's replaced by `seq::vari…
Mingun Jul 21, 2022
b627539
Remove `nested_collection` test because it's replaced by `seq::fixed_…
Mingun Jul 21, 2022
cd0793e
Remove `multiple_roots_attributes` test because it's replaced by `seq…
Mingun Jul 21, 2022
8f30b87
Add additional tests for error precedence
Mingun Jul 21, 2022
85a569d
Fix tests from the previous commit
Mingun Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
- [#421]: Fixed unknown bug in serde deserialization of externally tagged enums
when an enum variant represented as a `Text` event (i.e. `<xml>tag</xml>`)
and a document encoding is not an UTF-8
- [#434]: Fixed incorrect error generated in some cases by serde deserializer

### Misc Changes

Expand Down Expand Up @@ -158,6 +159,7 @@
- [#363]: Add tests for `Reader::read_event_impl` to ensure that proper events generated for corresponding inputs
- [#407]: Improved benchmark suite to cover whole-document parsing, escaping and unescaping text
- [#418]: Parameterized macrobenchmarks and comparative benchmarks, added throughput measurements via criterion
- [#434]: Added more tests for serde deserialier

[#8]: https://github.com/Mingun/fast-xml/pull/8
[#9]: https://github.com/Mingun/fast-xml/pull/9
Expand All @@ -178,6 +180,7 @@
[#418]: https://github.com/tafia/quick-xml/pull/418
[#421]: https://github.com/tafia/quick-xml/pull/421
[#423]: https://github.com/tafia/quick-xml/pull/423
[#434]: https://github.com/tafia/quick-xml/pull/434
[#437]: https://github.com/tafia/quick-xml/pull/437

## 0.23.0 -- 2022-05-08
Expand Down
8 changes: 7 additions & 1 deletion src/de/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,13 @@ where
};
key.map(Some)
}
_ => Ok(None),
// Stop iteration after reaching a closing tag
DeEvent::End(e) if e.name() == self.start.name() => Ok(None),
// This is a unmatched closing tag, so the XML is invalid
DeEvent::End(e) => Err(DeError::UnexpectedEnd(e.name().as_ref().to_owned())),
// We cannot get `Eof` legally, because we always inside of the
// opened tag `self.start`
DeEvent::Eof => Err(DeError::UnexpectedEof),
}
}
}
Expand Down
163 changes: 30 additions & 133 deletions tests/serde-de.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,115 +43,6 @@ fn string_borrow() {
assert_eq!(borrowed_item.text, "Hello world");
}

#[derive(Debug, Deserialize, PartialEq)]
struct Item {
name: String,
source: String,
}

#[test]
fn multiple_roots_attributes() {
let item: Vec<Item> = from_str(
r#"
<item name="hello1" source="world1.rs" />
<item name="hello2" source="world2.rs" />
"#,
)
.unwrap();
assert_eq!(
item,
vec![
Item {
name: "hello1".to_string(),
source: "world1.rs".to_string(),
},
Item {
name: "hello2".to_string(),
source: "world2.rs".to_string(),
},
]
);
}

#[test]
fn nested_collection() {
#[derive(Debug, Deserialize, PartialEq)]
struct Project {
name: String,

#[serde(rename = "item", default)]
items: Vec<Item>,
}

let project: Project = from_str(
r#"
<project name="my_project">
<item name="hello1" source="world1.rs" />
<item name="hello2" source="world2.rs" />
</project>
"#,
)
.unwrap();
assert_eq!(
project,
Project {
name: "my_project".to_string(),
items: vec![
Item {
name: "hello1".to_string(),
source: "world1.rs".to_string(),
},
Item {
name: "hello2".to_string(),
source: "world2.rs".to_string(),
},
],
}
);
}

#[test]
fn collection_of_enums() {
#[derive(Debug, Deserialize, PartialEq)]
enum MyEnum {
A(String),
B { name: String, flag: bool },
C,
}

#[derive(Debug, Deserialize, PartialEq)]
struct MyEnums {
// TODO: This should be #[serde(flatten)], but right now serde don't support flattening of sequences
// See https://github.com/serde-rs/serde/issues/1905
#[serde(rename = "$value")]
items: Vec<MyEnum>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a much more complex test than the one that replaces it (https://github.com/tafia/quick-xml/blob/d34b77972ccaad505c3348761382fe7d01ca49ce/tests/serde-de.rs#L1261=), is it really a full replacement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, yes. The mapping should be independent and composable, so we shouldn't bother of internals of the MyEnum type (or Item type in Project). For now we don't have a tests that ensures that mapping is really additive in all cases (which is why #435 is happened), but

  • that check should be added explicitly and for all combinations
  • in that test mapping is independent and composable

I'll plan to add necessary tests that would ensure that mapping is always composable (may be in form of doctests in #369)


let s = r#"
<enums>
<A>test</A>
<B name="hello" flag="t" />
<C />
</enums>
"#;

let project: MyEnums = from_str(s).unwrap();

assert_eq!(
project,
MyEnums {
items: vec![
MyEnum::A("test".to_string()),
MyEnum::B {
name: "hello".to_string(),
flag: true,
},
MyEnum::C,
],
}
);
}

/// Test for https://github.com/tafia/quick-xml/issues/231
#[test]
fn implicit_value() {
Expand Down Expand Up @@ -3589,6 +3480,18 @@ macro_rules! maplike_errors {
mod non_closed {
use super::*;

/// For struct we expect that error about not closed tag appears
/// earlier than error about missing fields
#[test]
fn missing_field() {
let data = from_str::<$type>(r#"<root>"#);

match data {
Err(DeError::UnexpectedEof) => (),
_ => panic!("Expected `UnexpectedEof`, found {:?}", data),
}
}

#[test]
fn attributes() {
let data = from_str::<$type>(r#"<root float="42" string="answer">"#);
Expand Down Expand Up @@ -3624,6 +3527,18 @@ macro_rules! maplike_errors {
use super::*;
use quick_xml::Error::EndEventMismatch;

/// For struct we expect that error about mismatched tag appears
/// earlier than error about missing fields
#[test]
fn missing_field() {
let data = from_str::<$type>(r#"<root></mismatched>"#);

match data {
Err(DeError::InvalidXml(EndEventMismatch { .. })) => (),
_ => panic!("Expected `InvalidXml(EndEventMismatch)`, found {:?}", data),
}
}

#[test]
fn attributes() {
let data = from_str::<$type>(
Expand Down Expand Up @@ -3922,6 +3837,12 @@ mod flatten_struct {
mod enum_ {
use super::*;

#[derive(Debug, Deserialize, PartialEq)]
struct Nested {
//TODO: change to f64 after fixing https://github.com/serde-rs/serde/issues/1183
float: String,
}

mod externally_tagged {
use super::*;
use pretty_assertions::assert_eq;
Expand All @@ -3947,12 +3868,6 @@ mod enum_ {
},
}

#[derive(Debug, Deserialize, PartialEq)]
struct Nested {
//TODO: change to f64 after fixing https://github.com/serde-rs/serde/issues/1183
float: String,
}

/// Workaround for serde bug https://github.com/serde-rs/serde/issues/1904
#[derive(Debug, Deserialize, PartialEq)]
enum Workaround {
Expand Down Expand Up @@ -4120,12 +4035,6 @@ mod enum_ {
value: bool,
}

#[derive(Debug, Deserialize, PartialEq)]
struct Nested {
//TODO: change to f64 after fixing https://github.com/serde-rs/serde/issues/1183
float: String,
}

mod unit {
use super::*;
use pretty_assertions::assert_eq;
Expand Down Expand Up @@ -4306,12 +4215,6 @@ mod enum_ {
},
}

#[derive(Debug, Deserialize, PartialEq)]
struct Nested {
//TODO: change to f64 after fixing https://github.com/serde-rs/serde/issues/1183
float: String,
}

/// Workaround for serde bug https://github.com/serde-rs/serde/issues/1904
#[derive(Debug, Deserialize, PartialEq)]
#[serde(tag = "tag", content = "content")]
Expand Down Expand Up @@ -4524,12 +4427,6 @@ mod enum_ {
},
}

#[derive(Debug, Deserialize, PartialEq)]
struct Nested {
//TODO: change to f64 after fixing https://github.com/serde-rs/serde/issues/1183
float: String,
}

/// Workaround for serde bug https://github.com/serde-rs/serde/issues/1904
#[derive(Debug, Deserialize, PartialEq)]
#[serde(untagged)]
Expand Down