Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove length encoding #102
Conversation
| @@ -398,6 +398,6 @@ fn bytes() { | |||
| let data = b"abc\0123"; | |||
| let s = serialize(&data, Infinite).unwrap(); | |||
| let s2 = serialize(&Bytes::new(data), Infinite).unwrap(); | |||
| assert_eq!(s, s2); | |||
| assert_ne!(s, s2); | |||
This comment has been minimized.
This comment has been minimized.
TyOverby
Feb 3, 2017
Collaborator
assert_ne isn't really buying us much test coverage here. Could you replace this with a comparison against the serialized data?
e.g. assert_eq!(s, vec![ byte values here ])
This comment has been minimized.
This comment has been minimized.
ZoeyR
Feb 3, 2017
Author
Collaborator
So I'm looking at the tests we already have. We do fixed sized array tests so just doing assert_eq!(s, vec![ byte values here ]) wouldn't really add anything to the test coverage. What I could do is assert_eq!(s[..], s2[8..]); to verify that after the length encoding they are identical.
This comment has been minimized.
This comment has been minimized.
TyOverby
Feb 3, 2017
Collaborator
assert_eq!(s[..], s2[8..]); to verify that after the length encoding they are identical.
That sounds good; I like it!
| @@ -398,6 +398,6 @@ fn bytes() { | |||
| let data = b"abc\0123"; | |||
| let s = serialize(&data, Infinite).unwrap(); | |||
| let s2 = serialize(&Bytes::new(data), Infinite).unwrap(); | |||
| assert_eq!(s, s2); | |||
| assert_ne!(s, s2); | |||
| } | |||
|
|
|||
This comment has been minimized.
This comment has been minimized.
TyOverby
Feb 3, 2017
Collaborator
Please add another test that does serialization and deserialization.
This comment has been minimized.
This comment has been minimized.
ZoeyR
Feb 3, 2017
Author
Collaborator
Since we do tests for fixed sized arrays, is adding a test for both serialization and deserialization really necessary?
This comment has been minimized.
This comment has been minimized.
TyOverby
Feb 3, 2017
Collaborator
Looking at this some more, I think that my request is out-of-scope for this PR. However, the structures serde::bytes::Bytes and serde::bytes::ByteBuf have special handling that we certainly want to be testing. I'll file another issue for this.
ZoeyR commentedFeb 1, 2017
Closes #86