hyperledger / fabric

Hyperledger Fabric is an enterprise-grade permissioned distributed ledger framework for developing solutions and applications. Its modular and versatile design satisfies a broad range of industry use cases. It offers a unique approach to consensus that enables performance at scale while preserving privacy.
https://wiki.hyperledger.org/display/fabric
Apache License 2.0
15.67k stars 8.82k forks source link

Use custom gomega matcher for protobuf equality #4966

Closed bestbeforetoday closed 1 month ago

bestbeforetoday commented 1 month ago

The standard gomega Equal matcher is not reliable when used with protobuf messages since they can be logically equal while containing different implementation field values. This change implements a custom ProtoEqual matcher that uses proto.Equal for reliable equality checks.

This implementation enables the following:

Expect(actual).To(ProtoEqual(expected))

which is a more idiomatic alternative to:

Expect(proto.Equal(expected, actual)).To(BeTrue())

In addition, a failed match produces output similar to the following, which is significantly more informative than a message simply stating that false is expected to be true:

[FAILED] Expected
        ChaincodeMessage{
                type:  TRANSACTION
                payload:  "\n\x04arg1\n\x04arg2"
                txid:  "tx-id"
                proposal:  {
                        proposal_bytes:  "signed-proposal-bytes"
                        signature:  "signature"
                }
                channel_id:  "channel-id"
        }
  to equal
        SignedProposal{
                proposal_bytes:  "signed-proposal-bytes"
                signature:  "signature"
        }
pfi79 commented 1 month ago

@bestbeforetoday Sorry, I see your change is still in the draft. But I'll ask you a question right away. Will you have a proposal for mixed structures, where there are both proto and non-proto fields.

For example: https://github.com/hyperledger/fabric/blob/main/core/ledger/kvledger/txmgmt/validation/batch_preparer_test.go#L453 https://github.com/hyperledger/fabric/blob/main/core/ledger/pvtdatastorage/retroactive_hashed_index_test.go#L78

bestbeforetoday commented 1 month ago

@pfi79 Covering both protobuf and non-protobuf elements in a single matcher is definitely possible. It would essentially mean duplicating the behaviour of the existing gomega Equal matcher and extending it to handle protobuf messages using proto.Equal. That is (much) more work than I was planning to tackle in this PR. Instead I was planning just what is implemented here — a matcher for protobuf messages to avoid the need for contributors to remember and use a pattern such as:

Expect(proto.Equal(value1, value2)).To(BeTrue())