Add tests of enum serialisation#315
Conversation
emdash-ie
added
the
maintenance
We kind of test this already with the ItemResponse test, but it’s worth adding extra ones since we’re specifically worried. I generated the binary values with the thriftToFile helper on thrift v0.15.0 (by downgrading and depending on a local version of fezziwig which was also downgraded), so that we can be confident updating to latest thrift hasn’t changed anything.
As pointed out in code review, since the tests only checked that a tag has decoded, it was possible for a tag to decode successfully but change tag type, e.g. if two types swapped their IDs. This commit updates the code to assert on the tag type and make sure they haven’t. This also exposes the same weakness in the existing tests, in that they don’t exhaustively check that the binary content deserialises to what we’d expect it to. I guess we could change that? Co-authored-by: Grahame Oakland <grahame.oakland@guardian.co.uk>
This reverts commit 1bb3c1e.
I want to produce the binary spec files for the enum tests using thrift 0.15.0. To do this, this commit downgrades our thrift dependency back to 0.15.0, and depends on a local version of fezziwig in which I’ve downgraded thrift back to 0.15.0 as well.
(And also remove the new call I added for tags) This reverts commit 8f680a9.
d7759d7 to
7c93489
Compare
This reverts commit 25dce88.
groakland
left a comment
groakland
left a comment
There was a problem hiding this comment.
The expected TagType values are currently discarded (_tagType), so this test only verifies that each fixture can be decoded and re-encoded to the same bytes. It does not confirm that the fixture decodes to the intended enum value.
We could pass an assertion to checkRoundTrip for each entry, e.g. asserting that the decoded Tag.type equals the expected TagType? That would ensure the test catches an enum mapping or wire-encoding issue rather than only validating a byte-preserving round trip.
Why this matters:
A round-trip can pass even if decoding maps an enum to the wrong value, because the same incorrect value may be serialized back into the original bytes. Asserting the decoded TagType makes the test validate the actual semantic mapping that this PR is intended to protect.
groakland
left a comment
groakland
left a comment
There was a problem hiding this comment.
LGTM with the latest changes. Approved
2 participants
What does this change?
We kind of test this already with the ItemResponse test, but it’s worth adding extra ones since we’re specifically worried. I generated the binary values with the thriftToFile helper on thrift v0.15.0 (by downgrading and depending on a local version of fezziwig which was also downgraded), so that we can be confident updating to latest thrift hasn’t changed anything.
This should give us extra confidence for https://github.com/guardian/articles-and-publishing/issues/517