Messages encoding test #13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for protobuf wire encoding of protocol messages across three layers: Go unit tests, Gherkin BDD specifications, and Cucumber integration tests that validate the Gherkin scenarios.
Key Changes:
- Adds serialization unit tests for all 16 protocol message types with expected hex wire encodings
- Introduces a Gherkin feature file defining cross-language test scenarios for message encoding
- Implements a Cucumber test harness that executes the Gherkin scenarios in Go
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gherkin/proto/wire.feature | Defines 14 BDD scenarios specifying expected protobuf wire encodings for each message type |
| compose/proto/protocol_messages_test.go | Implements Go unit tests validating message serialization against expected hex encodings |
| compose/proto/gherkin_cucumber_test.go | Cucumber test integration that parses Gherkin scenarios and validates message serialization using godog framework |
| compose/go.mod | Adds cucumber/godog dependencies for BDD testing support |
| compose/go.sum | Updates dependency checksums for newly added godog and related packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if f.Format != "nested" { | ||
| return nil, fmt.Errorf("Message.payload must be nested") | ||
| } | ||
| hr, err := parseHandshakeRequestNested(f.Value) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Message.payload: %w", err) | ||
| } | ||
| msg.Payload = &Message_HandshakeRequest{ | ||
| HandshakeRequest: hr, |
There was a problem hiding this comment.
The Message payload parsing only handles HandshakeRequest (lines 496-502). If additional Message scenarios with different payload types are added to the gherkin tests in the future, this code will need to be extended. Consider adding a comment noting this limitation or implementing a more generic payload parser that can detect the payload type from the nested value string.
| if f.Format != "nested" { | |
| return nil, fmt.Errorf("Message.payload must be nested") | |
| } | |
| hr, err := parseHandshakeRequestNested(f.Value) | |
| if err != nil { | |
| return nil, fmt.Errorf("Message.payload: %w", err) | |
| } | |
| msg.Payload = &Message_HandshakeRequest{ | |
| HandshakeRequest: hr, | |
| // NOTE: This code attempts to generically parse the payload type from the nested value string. | |
| // If new payload types are added to the Message proto, extend the switch below accordingly. | |
| if f.Format != "nested" { | |
| return nil, fmt.Errorf("Message.payload must be nested") | |
| } | |
| // Extract the payload type from the nested value string (assume first word is the type) | |
| payloadStr := strings.TrimSpace(f.Value) | |
| spaceIdx := strings.Index(payloadStr, " ") | |
| var payloadType string | |
| if spaceIdx == -1 { | |
| payloadType = payloadStr | |
| } else { | |
| payloadType = payloadStr[:spaceIdx] | |
| } | |
| switch payloadType { | |
| case "HandshakeRequest": | |
| hr, err := parseHandshakeRequestNested(f.Value) | |
| if err != nil { | |
| return nil, fmt.Errorf("Message.payload: %w", err) | |
| } | |
| msg.Payload = &Message_HandshakeRequest{ | |
| HandshakeRequest: hr, | |
| } | |
| case "Proof": | |
| proof, err := parseProofNested(f.Value) | |
| if err != nil { | |
| return nil, fmt.Errorf("Message.payload: %w", err) | |
| } | |
| msg.Payload = &Message_Proof{ | |
| Proof: proof, | |
| } | |
| case "NativeDecided": | |
| nd, err := parseNativeDecidedNested(f.Value) | |
| if err != nil { | |
| return nil, fmt.Errorf("Message.payload: %w", err) | |
| } | |
| msg.Payload = &Message_NativeDecided{ | |
| NativeDecided: nd, | |
| } | |
| case "WSDecided": | |
| wsd, err := parseWSDecidedNested(f.Value) | |
| if err != nil { | |
| return nil, fmt.Errorf("Message.payload: %w", err) | |
| } | |
| msg.Payload = &Message_WsDecided{ | |
| WsDecided: wsd, | |
| } | |
| default: | |
| return nil, fmt.Errorf("Message.payload: unknown payload type %q", payloadType) |
| func TestMain(m *testing.M) { | ||
| status := godog.TestSuite{ | ||
| Name: "wire-feature", | ||
| ScenarioInitializer: InitializeScenario, | ||
| TestSuiteInitializer: nil, | ||
| Options: &godog.Options{ | ||
| Format: "pretty", | ||
| Paths: []string{"../../gherkin/proto/wire.feature"}, | ||
| }, | ||
| }.Run() | ||
|
|
||
| if st := m.Run(); st > status { | ||
| status = st | ||
| } | ||
| os.Exit(status) | ||
| } |
There was a problem hiding this comment.
[nitpick] The TestMain function intercepts all test execution for this package. This means regular go test will run both the cucumber scenarios AND call m.Run() for regular unit tests. If the cucumber tests fail (non-zero status), but unit tests pass, the exit status could potentially be masked. Consider using a more explicit test function name like TestGherkinWireFeature with godog.TestSuite.Run() inside it, or ensure the status combination logic at lines 122-124 correctly prioritizes failures.
| github.com/cucumber/gherkin/go/v26 v26.2.0 // indirect | ||
| github.com/cucumber/godog v0.15.1 // indirect |
There was a problem hiding this comment.
The cucumber dependencies are marked as // indirect but they are directly imported and used in gherkin_cucumber_test.go. These should be direct dependencies. Run go mod tidy to correctly update the go.mod file, which should remove the // indirect comments for github.com/cucumber/godog and github.com/cucumber/gherkin/go/v26.
| github.com/cucumber/gherkin/go/v26 v26.2.0 // indirect | |
| github.com/cucumber/godog v0.15.1 // indirect | |
| github.com/cucumber/gherkin/go/v26 v26.2.0 | |
| github.com/cucumber/godog v0.15.1 |
| pubkey, _ = hex.DecodeString("abc") | ||
| signature, _ = hex.DecodeString("def") | ||
| nonce, _ = hex.DecodeString("123") | ||
| instanceid, _ = hex.DecodeString("01") | ||
| proof, _ = hex.DecodeString("156") | ||
| sbhash, _ = hex.DecodeString("1123768") | ||
| source, _ = hex.DecodeString("adecb123") | ||
| receiver, _ = hex.DecodeString("192bca") | ||
| data, _ = hex.DecodeString("aaabbb") | ||
| tx1, _ = hex.DecodeString("aa1") | ||
| tx2, _ = hex.DecodeString("aa2") |
There was a problem hiding this comment.
The odd-length hex strings ("abc", "def", "123", "aa1", "aa2", "156", "1123768") will cause hex.DecodeString to return hex.ErrLength along with partial decoded data. In Go, this error is silently ignored and the partial result is used. This behavior is implementation-specific and may not be portable to other languages implementing the gherkin tests. Consider using even-length hex strings (e.g., "0abc", "0def", "0123", "0aa1", "0aa2", "0156", "01123768") or explicitly documenting this as intentional test behavior.
| pubkey, _ = hex.DecodeString("abc") | |
| signature, _ = hex.DecodeString("def") | |
| nonce, _ = hex.DecodeString("123") | |
| instanceid, _ = hex.DecodeString("01") | |
| proof, _ = hex.DecodeString("156") | |
| sbhash, _ = hex.DecodeString("1123768") | |
| source, _ = hex.DecodeString("adecb123") | |
| receiver, _ = hex.DecodeString("192bca") | |
| data, _ = hex.DecodeString("aaabbb") | |
| tx1, _ = hex.DecodeString("aa1") | |
| tx2, _ = hex.DecodeString("aa2") | |
| pubkey, _ = hex.DecodeString("0abc") | |
| signature, _ = hex.DecodeString("0def") | |
| nonce, _ = hex.DecodeString("0123") | |
| instanceid, _ = hex.DecodeString("01") | |
| proof, _ = hex.DecodeString("0156") | |
| sbhash, _ = hex.DecodeString("01123768") | |
| source, _ = hex.DecodeString("adecb123") | |
| receiver, _ = hex.DecodeString("192bca") | |
| data, _ = hex.DecodeString("aaabbb") | |
| tx1, _ = hex.DecodeString("0aa1") | |
| tx2, _ = hex.DecodeString("0aa2") |
| Feature: Protobuf wire encoding | ||
| Each scenario defines a message instance and the expected | ||
| protobuf wire encoding as a hexadecimal string. | ||
| The implementation should construct the specified message and assert that | ||
| its serialized bytes match the given hex. | ||
|
|
||
| Scenario: HandshakeRequest basic serialization | ||
| When the following "HandshakeRequest" message is serialized: | ||
| | field | format | value | | ||
| | timestamp | int64 | 1234567890 | | ||
| | public_key | hex | abc | | ||
| | signature | hex | def | | ||
| | client_id | string | client-1 | | ||
| | nonce | hex | 123 | | ||
| Then the wire hex should be "08d285d8cc041201ab1a01de2208636c69656e742d312a0112" | ||
|
|
||
| Scenario: HandshakeResponse basic serialization | ||
| When the following "HandshakeResponse" message is serialized: | ||
| | field | format | value | | ||
| | accepted | bool | true | | ||
| | error | string | error-message | | ||
| | session_id | string | session-123 | | ||
| Then the wire hex should be "0801120d6572726f722d6d6573736167651a0b73657373696f6e2d313233" | ||
|
|
||
| Scenario: Ping timestamp 42 | ||
| When the following "Ping" message is serialized: | ||
| | field | format | value | | ||
| | timestamp | int64 | 42 | | ||
| Then the wire hex should be "082a" | ||
|
|
||
| Scenario: Pong timestamp 43 | ||
| When the following "Pong" message is serialized: | ||
| | field | format | value | | ||
| | timestamp | int64 | 43 | | ||
| Then the wire hex should be "082b" | ||
|
|
||
| Scenario: TransactionRequest with two transactions | ||
| When the following "TransactionRequest" message is serialized: | ||
| | field | format | value | | ||
| | chain_id | uint64 | 1 | | ||
| | transaction0 | hex | aa1 | | ||
| | transaction1 | hex | aa2 | | ||
| Then the wire hex should be "08011201aa1201aa" | ||
|
|
||
| Scenario: XTRequest with two transaction requests | ||
| When the following "XTRequest" message is serialized: | ||
| | field | format | value | | ||
| | transaction0 | nested | TransactionRequest(chain_id=10, tx=[aa1]) | | ||
| | transaction1 | nested | TransactionRequest(chain_id=11, tx=[aa2]) | | ||
| Then the wire hex should be "0a05080a1201aa0a05080b1201aa" | ||
|
|
||
| Scenario: StartInstance basic serialization | ||
| When the following "StartInstance" message is serialized: | ||
| | field | format | value | | ||
| | instance_id | hex | 01 | | ||
| | period_id | uint64 | 100 | | ||
| | sequence_number | uint64 | 7 | | ||
| | xt_request | nested | XTRequest(transaction_requests=[TR(1, [aa1])]) | | ||
| Then the wire hex should be "0a01011064180722070a0508011201aa" | ||
|
|
||
| Scenario: Vote basic serialization | ||
| When the following "Vote" message is serialized: | ||
| | field | format | value | | ||
| | instance_id | hex | 01 | | ||
| | chain_id | uint64 | 2 | | ||
| | vote | bool | true | | ||
| Then the wire hex should be "0a010110021801" | ||
|
|
||
| Scenario: Decided basic serialization | ||
| When the following "Decided" message is serialized: | ||
| | field | format | value | | ||
| | instance_id | hex | 01 | | ||
| | decision | bool | false | | ||
| Then the wire hex should be "0a0101" | ||
|
|
||
| Scenario: MailboxMessage with two data entries | ||
| When the following "MailboxMessage" message is serialized: | ||
| | field | format | value | | ||
| | session_id | uint64 | 123 | | ||
| | instance_id | hex | 01 | | ||
| | source_chain | uint64 | 1 | | ||
| | destination_chain | uint64 | 2 | | ||
| | source | hex | adecb123 | | ||
| | receiver | hex | 192bca | | ||
| | label | string | label-1 | | ||
| | data0 | hex | aaabbb | | ||
| Then the wire hex should be "087b120101180120022a04adecb1233203192bca3a076c6162656c2d314203aaabbb" | ||
|
|
||
| Scenario: StartPeriod basic serialization | ||
| When the following "StartPeriod" message is serialized: | ||
| | field | format | value | | ||
| | period_id | uint64 | 5 | | ||
| | superblock_number | uint64 | 42 | | ||
| Then the wire hex should be "0805102a" | ||
|
|
||
| Scenario: Rollback basic serialization | ||
| When the following "Rollback" message is serialized: | ||
| | field | format | value | | ||
| | period_id | uint64 | 6 | | ||
| | last_finalized_superblock_num | uint64 | 41 | | ||
| | last_finalized_superblock_hash | hex | 1123768 | | ||
| Then the wire hex should be "080610291a03112376" | ||
|
|
||
| Scenario: Proof basic serialization | ||
| When the following "Proof" message is serialized: | ||
| | field | format | value | | ||
| | period_id | uint64 | 7 | | ||
| | superblock_number | uint64 | 99 | | ||
| | proof_data | hex | 156 | | ||
| Then the wire hex should be "080710631a0115" | ||
|
|
||
| Scenario: NativeDecided basic serialization | ||
| When the following "NativeDecided" message is serialized: | ||
| | field | format | value | | ||
| | instance_id | hex | 01 | | ||
| | decision | bool | true | | ||
| Then the wire hex should be "0a01011001" | ||
|
|
||
| Scenario: WSDecided basic serialization | ||
| When the following "WSDecided" message is serialized: | ||
| | field | format | value | | ||
| | instance_id | hex | 01 | | ||
| | decision | bool | false | | ||
| Then the wire hex should be "0a0101" | ||
|
|
||
| Scenario: Message wrapper with HandshakeRequest payload | ||
| When the following "Message" wrapper is serialized: | ||
| | field | format | value | | ||
| | sender_id | string | sender-1 | | ||
| | payload | nested | HandshakeRequest(timestamp=987654321, public_key=abc, signature=def, client_id=client-in-message, nonce=123) | | ||
| Then the wire hex should be "0a0873656e6465722d31122208b1d1f9d6031201ab1a01de2211636c69656e742d696e2d6d6573736167652a0112" |
There was a problem hiding this comment.
The odd-length hex values ("abc", "def", "123", "aa1", "aa2", "156", "1123768") in test scenarios rely on Go's specific behavior where hex.DecodeString returns partial results for odd-length strings. This is handled by the decodeHexLikeGo function in the Cucumber test, but implementations in other languages may not have this behavior by default. Consider using even-length hex strings for better cross-language compatibility, or add a note in the feature file that odd-length hex strings should decode to their truncated even-length equivalents.
| // decodeHexLikeGo mimics the behavior of hex.DecodeString in the existing tests. | ||
| // For errors due to odd-length strings, the content is still returned with no errors. | ||
| // Else, the error is returned. | ||
| func decodeHexLikeGo(s string) ([]byte, error) { | ||
| b, err := hex.DecodeString(s) | ||
| if err != nil { | ||
| // Preserve the partial result for ErrLength but treat other errors as fatal. | ||
| if err == hex.ErrLength { | ||
| return b, nil | ||
| } | ||
| return nil, err | ||
| } | ||
| return b, nil | ||
| } |
There was a problem hiding this comment.
The decodeHexLikeGo function silently handles odd-length hex strings by returning partial decoded data when hex.ErrLength occurs. While this mimics Go's hex.DecodeString behavior, it could hide legitimate errors. Consider adding a comment explaining this is intentional to match test data expectations, or add logging/warnings when odd-length strings are encountered to make this behavior more visible during test execution.
Overview
This PR adds: