-
Notifications
You must be signed in to change notification settings - Fork 168
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
Types (de)serialization tests #326
Conversation
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.
This looks great, thank you very much. :)
types/src/v2/request.rs
Outdated
// With all fields set. | ||
(r#"{"jsonrpc":"2.0", "method":"subtract", "params":[42, 23], "id":1}"#, Some(id), Some(params)), | ||
// Without ID field. | ||
(r#"{"jsonrpc":"2.0", "method":"subtract", "params":[42, 23]}"#, None, Some(params)), |
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.
not your fault but this shouldn't be parsed as a JsonRpcRequest
instead should be parsed as JsonRpcNotification
, #325 fixes this.
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.
Should I just remove it now, or wait until the linked PR is merged and implement a negative test?
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.
I merged it #325, do mind merging/rebasing to master? Otherwise, I can help with it that.
Sorry for introducing this merge conflicts.
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.
No problem. I've rebased to master, removed tests that became duplicated (tried to leave ones that are more comprehensive), and aligned my tests with the surrounding code.
assert_eq!(request.jsonrpc, TwoPointZero); | ||
assert_eq!(request.id, id); | ||
assert_eq!(request.method, method); | ||
assert_eq!(request.params.map(RawValue::get), params); |
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.
cool, we should implement PartialEq
for the types manually that has serde_json::RawValue
entrie(s) in another PR I guess.
Some tests for RPC types (de)serialization wouldn't harm, would they?