Types (de)serialization tests#326
Conversation
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
This looks great, thank you very much. :)
| // 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.
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.
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.
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.
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.
cool, we should implement PartialEq for the types manually that has serde_json::RawValue entrie(s) in another PR I guess.
3 participants
Some tests for RPC types (de)serialization wouldn't harm, would they?