test: add metadata and metadataKey test coverage#1247
test: add metadata and metadataKey test coverage#1247rwalworth merged 2 commits intohiero-ledger:mainfrom
Conversation
…tTests Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
5e8fbec to
69946c2
Compare
|
Hey @Kubikusik 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1235 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for tackling this @Kubikusik! The test coverage additions are on the right track - the new GetSetMetadataFrozen, GetSetMetadataKey, and GetSetMetadataKeyFrozen tests and the protobuf deserialization assertions are exactly what #1235 asks for.
I left a few comments below. To answer your question from the PR description - there is a simpler way to set the metadata field on the protobuf body that avoids the helper function entirely. Details in the comment below!
Signed-off-by: kubikusik <jakubmical@student.agh.edu.pl>
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for the updates @Kubikusik - all three items from my previous review are addressed. The bytesToProtobuf helper is gone, GetSetMetadataKey matches the non-frozen pattern, and the trailing newline is back. Running the workflows now and will merge once they pass!
Description:
Cover test coverage gap for setMetadata and setMetadataKey in
TokenUpdateTransactionUnitTests.cc to match the pattern of all other setters.
mTestMetadataKeyprivate member andgetTestMetadataKey()accessor to test fixtureGetSetMetadataFrozentestGetSetMetadataKeyandGetSetMetadataKeyFrozentestsConstructTokenUpdateTransactionFromTransactionBodyProtobufto set andverify
metadataandmetadata_keyfieldsRelated issue(s):
Fixes #1235
Notes for reviewer:
Current implementation involves a helper function for setting
body->set_allocated_metadatain theTokenUpdateTransactionBodyprotobuf, if there is a more intended solution I have missed, please do propose it as a change here.Checklist