gemmellr commented on pull request #263: URL: https://github.com/apache/qpid-proton/pull/263#issuecomment-656728966
I would be inclined to add some tests that verify the encoded bytes are exactly as expected, and not just encode+decode things to check that the pn_data that goes into encode is what comes out of decode. The latter is still useful, but it mainly verifies there isnt a different understanding between the encoder and decoder, and not really that they actually encoded/decoded things correctly. Indeed, as you have pointed out the formatting stuff being used in the test is actually currently encoding String/Symbol values differently that its described behaviour (which as Andrew noted elsewhere may just be a internal doc issue, but every single use of those values now needs to be checked to ensure that). The same could be[come] true in other cases either now or at a later point, along with other encode/decode bugs, in a way the test simply wouldn't notice (something thats happened before). I'd also suggest some obvious test cases that are worth adding at the same time (unless they already exist elsewhere) based on the changes being made, like an array of only empty lists (which may encode quite differently if it uses list0 type; no list count/size), and an array where an empty list isnt the first element. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
