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

Use serde_test for unit test instead of serde_json. #67

Merged
merged 3 commits into from Dec 25, 2022
Merged

Use serde_test for unit test instead of serde_json. #67

merged 3 commits into from Dec 25, 2022

Conversation

BratSinot
Copy link
Contributor

Hello again!

These PRs have no new functionality, but may be implementing a more idiomatic way to test serde::{Serialize, Deserialize} by serde_test crate.

But because ArcSwapAny doesn't have Eq, PartialEq traits implementation, in this case I'm using some wrapper around it.

What do you think about this ?

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2021

Codecov Report

Merging #67 (e4004c3) into master (7e16ca3) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
- Coverage   86.34%   86.15%   -0.20%     
==========================================
  Files          18       18              
  Lines        1091     1076      -15     
==========================================
- Hits          942      927      -15     
  Misses        149      149              
Impacted Files Coverage Δ
src/serde.rs 100.00% <100.00%> (ø)

Copy link
Owner

@vorner vorner left a comment

Choose a reason for hiding this comment

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

Honestly, I'm not sure.

This brings that wrapper and somewhat less readable and longer testing code than before.

What's the advantage of using serde_test over serde_json? I know it has test in its name, so it should be somehow more appropriate, but I feel like we are not using any of the features it provides so we don't benefit from it.

So unless there's some advantage I don't see, I'd propose leaving it with the json one.

src/serde.rs Outdated Show resolved Hide resolved
@BratSinot
Copy link
Contributor Author

What's the advantage of using serde_test over serde_json? I know it has test in its name, so it should be somehow more appropriate, but I feel like we are not using any of the features it provides so we don't benefit from it.

Hypoteticaly, can exist some formats which supported by serde but can't serialize / deserialize into Json.
Also, IMHO, serde_test tokens thing much more undertandable than to_string / from_string comming from serde_json.

@vorner
Copy link
Owner

vorner commented Nov 27, 2021

I don't think the part with formats unsupported by JSON applies here in any way ‒ the test doesn't use such and we only test that the ArcSwap is „transparent“. But 🤷 whatever, let's have this.

But please have a look at the simplification as noted above.

src/serde.rs Show resolved Hide resolved
@vorner vorner merged commit 634dd80 into vorner:master Dec 25, 2022
@vorner
Copy link
Owner

vorner commented Dec 25, 2022

Hello

Ooops, I must have missed this somehow, my apologies. I've merged it now, when cleaning things up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants