vorner / arc-swap

Support atomic operations on Arc itself
Apache License 2.0
778 stars 31 forks source link

Use serde_test for unit test instead of serde_json. #67

Closed BratSinot closed 1 year ago

BratSinot commented 2 years ago

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 commented 2 years ago

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%> (ø)
BratSinot commented 2 years ago

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 commented 2 years ago

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 :shrug: whatever, let's have this.

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

vorner commented 1 year ago

Hello

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