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

Remove excessive serde tests and fix a bug #434

Merged
merged 6 commits into from
Jul 24, 2022

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Jul 21, 2022

This PR removes tests which have better alternatives in seq submodule since #387.

Also, while working on namespaces support, I accidently found a bug in a deserializer, so I've added a test and a fix for it.

@Mingun Mingun added the serde Issues related to mapping from Rust types to XML label Jul 21, 2022
@Mingun Mingun requested a review from dralley July 21, 2022 18:28
// 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)

@dralley
Copy link
Collaborator

dralley commented Jul 24, 2022

I used the merge conflict cleanup feature just to try it out - you can squash that commit if you want.

@codecov-commenter
Copy link

Codecov Report

Merging #434 (acdb8c5) into master (a5da628) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   51.51%   51.44%   -0.07%     
==========================================
  Files          25       25              
  Lines       13310    13312       +2     
==========================================
- Hits         6856     6849       -7     
- Misses       6454     6463       +9     
Flag Coverage Δ
unittests 51.44% <66.66%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/de/map.rs 72.35% <66.66%> (-0.21%) ⬇️
src/de/var.rs 63.82% <0.00%> (-21.28%) ⬇️
src/reader/mod.rs 90.42% <0.00%> (ø)
src/lib.rs 12.33% <0.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5da628...acdb8c5. Read the comment docs.

…name::variable_size::field_before_list` tests
failures (1):
    struct_::non_closed::missing_field
Fixes:
- struct_::non_closed::missing_field
@Mingun
Copy link
Collaborator Author

Mingun commented Jul 24, 2022

The history becomes too confusing because of the back-and-forth merges. I'd rather do a rebase.

@dralley dralley merged commit e7f30d2 into tafia:master Jul 24, 2022
@Mingun Mingun deleted the serde-tests-cleanup branch July 24, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
serde Issues related to mapping from Rust types to XML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants