Fix PrepareRespResult wire encoding
#2469
Conversation
Include the 4 byte length prefix when encoding ping-pong messages into a `PrepareRespResult`. Closes #2466
| @@ -2900,7 +2902,8 @@ where | |||
| let encoding = hex::decode(hex_encoding).unwrap(); | |||
| assert_eq!( | |||
| encoded_val, encoding, | |||
| "Couldn't roundtrip (encoded value differs): {val:?}" | |||
| "Couldn't roundtrip (encoded value differs): {val:?} | |||
| encoded:\t\t{encoded_val:02x?}\nexpected encoding:\t{encoding:02x?}" | |||
There was a problem hiding this comment.
On failure, assert_eq! will print out the mismatched values as vectors of unpadded decimals, which was driving me nuts because the message encoding tests are written in padded (i.e. each byte is two characters) hex vectors. So you had to convert from decimal to hex, and also the good/bad vectors wouldn't line up vertically. On failure, we now get this output:
---- tests::roundtrip_aggregation_job_resp stdout ----
thread 'tests::roundtrip_aggregation_job_resp' panicked at messages/src/lib.rs:2903:9:
assertion `left == right` failed: Couldn't roundtrip (encoded value differs): AggregationJobResp { prepare_resps: [PrepareResp { report_id: ReportId(AQIDBAUGBwgJCgsMDQ4PEA), result: Continue }, PrepareResp { report_id: ReportId(EA8ODQwLCgkIBwYFBAMCAQ), result: Finished }] }
encoded: [00, 00, 00, 39, 01, 02, 03, 04, 05, 06, 07, 08, 09, 0a, 0b, 0c, 0d, 0e, 0f, 10, 00, 00, 00, 00, 13, 01, 00, 00, 00, 05, 30, 31, 32, 33, 34, 00, 00, 00, 05, 35, 36, 37, 38, 39, 10, 0f, 0e, 0d, 0c, 0b, 0a, 09, 08, 07, 06, 05, 04, 03, 02, 01, 01]
expected encoding: [00, 00, 00, 35, 01, 02, 03, 04, 05, 06, 07, 08, 09, 0a, 0b, 0c, 0d, 0e, 0f, 10, 00, 00, 00, 00, 13, 01, 00, 00, 00, 05, 30, 31, 32, 33, 34, 00, 00, 00, 05, 35, 36, 37, 38, 39, 10, 0f, 0e, 0d, 0c, 0b, 0a, 09, 08, 07, 06, 05, 04, 03, 02, 01, 01]
left: [0, 0, 0, 57, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 0, 0, 0, 0, 19, 1, 0, 0, 0, 5, 48, 49, 50, 51, 52, 0, 0, 0, 5, 53, 54, 55, 56, 57, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 1]
right: [0, 0, 0, 53, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 0, 0, 0, 0, 19, 1, 0, 0, 0, 5, 48, 49, 50, 51, 52, 0, 0, 0, 5, 53, 54, 55, 56, 57, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 1]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
tests::roundtrip_aggregation_job_resp
test result: FAILED. 44 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s
The left/right lines are redundant, but at least now you can see the hex bytes.
There was a problem hiding this comment.
FWIW, we could get rid of the duplication by either using hex_display (which would get rid of the commas/spaces too) or with a similar newtype that calls f.write_fmt(format_args!("{:02x?}", self.0) in its Debug implementation.
There was a problem hiding this comment.
For me, having the bytes spaced out this way makes it easier to spot differences. I suppose the ideal would be some formatter or library that could diff them and then highlight the bytes that differ, maybe in colour, but I don't want to investigate that in this PR.
There was a problem hiding this comment.
Sounds good, I'll open an issue with a proposal.
|
I think we have the same problem in |
These messages had the same problem as `PrepareResp`.
Include the 4 byte length prefix when encoding ping-pong messages into a
PrepareRespResult.Closes #2466