improve grpc converter tests #8208
Conversation
Add comprehensive tests for all previously untested conversion and validation functions.
mts1715
requested review from
AndriiDiachuk,
UlyanaAndrukhiv,
illia-malachyn and
peterargue
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
UlyanaAndrukhiv
left a comment
UlyanaAndrukhiv
left a comment
There was a problem hiding this comment.
Overall, looks good! I’ve left a few suggestions for improvements.
| EndHeight: endHeight, | ||
| } | ||
|
|
||
| msg := convert.CompatibleRangeToMessage(comparableRange) |
There was a problem hiding this comment.
I think we can remove the "convert range to message" subtest, since the "roundtrip conversion" subtest now covers the same behavior.
| assert.Equal(t, seal.BlockID, converted.BlockID) | ||
| assert.Equal(t, seal.ResultID, converted.ResultID) | ||
| assert.Equal(t, seal.FinalState, converted.FinalState) | ||
| assert.Equal(t, seal.AggregatedApprovalSigs, converted.AggregatedApprovalSigs) |
There was a problem hiding this comment.
we could use the ID() method here and in other cases to ensure that the objects are cryptographically identical.
| assert.Equal(t, seal.BlockID, converted.BlockID) | |
| assert.Equal(t, seal.ResultID, converted.ResultID) | |
| assert.Equal(t, seal.FinalState, converted.FinalState) | |
| assert.Equal(t, seal.AggregatedApprovalSigs, converted.AggregatedApprovalSigs) | |
| assert.Equal(t, seal.ID(), converted.ID()) |
| assert.Equal(t, seal.BlockID, converted[i].BlockID) | ||
| assert.Equal(t, seal.ResultID, converted[i].ResultID) | ||
| assert.Equal(t, seal.FinalState, converted[i].FinalState) | ||
| assert.Equal(t, seal.AggregatedApprovalSigs, converted[i].AggregatedApprovalSigs) |
There was a problem hiding this comment.
| assert.Equal(t, seal.BlockID, converted[i].BlockID) | |
| assert.Equal(t, seal.ResultID, converted[i].ResultID) | |
| assert.Equal(t, seal.FinalState, converted[i].FinalState) | |
| assert.Equal(t, seal.AggregatedApprovalSigs, converted[i].AggregatedApprovalSigs) | |
| assert.Equal(t, seal.ID(), converted[i].ID()) |
| seals := []*flow.Seal{ | ||
| unittest.Seal.Fixture(), | ||
| unittest.Seal.Fixture(), | ||
| unittest.Seal.Fixture(), | ||
| } |
There was a problem hiding this comment.
| seals := []*flow.Seal{ | |
| unittest.Seal.Fixture(), | |
| unittest.Seal.Fixture(), | |
| unittest.Seal.Fixture(), | |
| } | |
| seals := unittest.Seal.Fixtures(3) |
| assert.Equal(t, block.Payload.Guarantees, payload.Guarantees) | ||
| assert.Equal(t, block.Payload.Seals, payload.Seals) | ||
| assert.Equal(t, block.Payload.Receipts, payload.Receipts) | ||
| assert.Equal(t, block.Payload.Results, payload.Results) | ||
| assert.Equal(t, block.Payload.ProtocolStateID, payload.ProtocolStateID) |
There was a problem hiding this comment.
| assert.Equal(t, block.Payload.Guarantees, payload.Guarantees) | |
| assert.Equal(t, block.Payload.Seals, payload.Seals) | |
| assert.Equal(t, block.Payload.Receipts, payload.Receipts) | |
| assert.Equal(t, block.Payload.Results, payload.Results) | |
| assert.Equal(t, block.Payload.ProtocolStateID, payload.ProtocolStateID) | |
| assert.Equal(t, block.Payload.Hash(), payload.Hash()) |
| } | ||
|
|
||
| // TestConvertBlockEventsToMessage tests converting a single flow.BlockEvents to a protobuf message. | ||
| func TestConvertBlockEventsToMessage(t *testing.T) { |
There was a problem hiding this comment.
Looks like the TestConvertBlockEventsToMessage is redundant. The TestConvertMessageToBlockEvents, already performs a full round-trip conversion and checks the same fields, so we could simplify by keeping only the round-trip test.
and rename the TestConvertMessageToBlockEvents to TestConvertBlockEvent.
| require.Equal(t, blockEvents.BlockHeight, converted.BlockHeight) | ||
| require.Equal(t, blockEvents.BlockID, converted.BlockID) | ||
| require.Equal(t, blockEvents.BlockTimestamp.Unix(), converted.BlockTimestamp.Unix()) | ||
| require.Len(t, converted.Events, len(blockEvents.Events)) |
There was a problem hiding this comment.
| require.Len(t, converted.Events, len(blockEvents.Events)) | |
| require.Len(t, converted.Events, len(blockEvents.Events)) | |
| for i, event := range blockEvents.Events { | |
| require.Equal(t, event.ID(), converted.Events[i].ID()) | |
| } |
or
we can compare all object, cause flow.BlockEvents does not have the ID() method.
require.Equal(t, blockEvents, converted)
| ) | ||
|
|
||
| _, err := convert.CcfEventToJsonEvent(invalidEvent) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Please verify the error and that the response is nil.
| ) | ||
|
|
||
| _, err := convert.CcfEventToJsonEvent(emptyEvent) | ||
| require.Error(t, err, "empty payload should result in error from flow.NewEvent") |
There was a problem hiding this comment.
Same here and in other cases
| // Add some variation to exercise different code paths | ||
| if i%2 == 0 { | ||
| arg, err := jsoncdc.Encode(cadence.NewAddress(unittest.AddressFixture())) | ||
| require.NoError(t, err) | ||
| tx.Arguments = append(tx.Arguments, arg) | ||
| } | ||
|
|
||
| if i%3 == 0 { | ||
| tx.EnvelopeSignatures = append(tx.EnvelopeSignatures, unittest.TransactionSignatureFixture()) | ||
| } |
There was a problem hiding this comment.
I recommend just using unittest.TransactionBodyListFixture, but renaming it to unittest.TransactionFixtures (since unittest.TransactionBodyFixture is deprecated , and we have only three usages of TransactionBodyListFixture, so no need to create a new one).
Alternatively, we could add With... options to TransactionFixture and use them. But I think it not worth in such tests. Reason: we prevent mutations of struct types marked as immutable in code and tries to avoid such mutations in new tests at least.
Closes #8127
test: achieve 100% test coverage for engine/common/rpc/convert package
Add comprehensive tests for all previously untested conversion and validation
functions.
Changes: