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
Conversation
Codecov Report
@@ 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
|
There was a problem hiding this 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.
Hypoteticaly, can exist some formats which supported by |
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. |
Hello Ooops, I must have missed this somehow, my apologies. I've merged it now, when cleaning things up. |
Hello again!
These PRs have no new functionality, but may be implementing a more idiomatic way to test
serde::{Serialize, Deserialize}
byserde_test
crate.But because
ArcSwapAny
doesn't haveEq
,PartialEq
traits implementation, in this case I'm using some wrapper around it.What do you think about this ?