Conversation
WalkthroughThe updates introduce a new assertion utility for validating Redis XREAD command responses in RESP format, adding types and normalization logic for structured comparison. The XREAD-related test function is refactored to use this new assertion framework, replacing manual checks with structured test cases and assertion objects. Additionally, the RESP array decoder is refined to explicitly handle the Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase as Test Case
participant RedisClient as Redis Client
participant Assertion as XReadResponseAssertion
TestCase->>RedisClient: Send XADD command
RedisClient-->>TestCase: Acknowledge entry added
TestCase->>RedisClient: Send XREAD command (possibly in goroutine)
RedisClient-->>TestCase: Return stream entry
TestCase->>Assertion: Run(expected, actual)
Assertion-->>TestCase: Compare normalized responses
TestCase->>RedisClient: Send another XADD command
RedisClient-->>TestCase: Acknowledge entry added
TestCase->>RedisClient: Send blocking XREAD command
RedisClient-->>TestCase: Return nil (timeout)
TestCase->>Assertion: Run(expected, actual)
Assertion-->>TestCase: Validate nil response
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/resp_assertions/stream_assertion.go (2)
41-44: Simplify the expected value preparationThe conversion from ExpectedValue to expected seems unnecessary and could be simplified.
- expected := make([]interface{}, len(a.ExpectedValue)) - for i, v := range a.ExpectedValue { - expected[i] = v - } + expected := a.ExpectedValue
19-39: Consider handling nil arraysThe conversion function doesn't explicitly handle the NIL type. While it might work via the default case, it would be more robust to handle it explicitly.
func (a StreamAssertion) Run(value resp_value.Value) error { if value.Type != resp_value.ARRAY { return fmt.Errorf("Expected array, got %s", value.Type) } var convertToSlice func(v resp_value.Value) interface{} convertToSlice = func(v resp_value.Value) interface{} { switch v.Type { case resp_value.BULK_STRING: return v.String() case resp_value.ARRAY: result := make([]interface{}, len(v.Array())) for i, elem := range v.Array() { result[i] = convertToSlice(elem) } return result + case resp_value.NIL: + return nil default: return v.String() } } actual := convertToSlice(value).([]interface{})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/resp/decoder/decode_array.go(1 hunks)internal/resp_assertions/stream_assertion.go(1 hunks)internal/test_streams_xread_block_max_id.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/resp/decoder/decode_array.go (1)
internal/resp/value/value.go (1)
NewNilValue(71-75)
🔇 Additional comments (6)
internal/resp/decoder/decode_array.go (1)
34-37: Correctly handling nil arrays in RESP protocolThis change properly implements the RESP protocol's handling of arrays with a length of -1, which represent nil arrays. The explicit distinction between -1 (valid nil array) and other negative values (errors) clarifies the special case handling and aligns with the protocol specification.
internal/resp_assertions/stream_assertion.go (1)
11-59: StreamAssertion implementation looks goodThe new StreamAssertion type provides a clean way to validate Redis stream responses with properly structured error messages. The recursive conversion from RESP values to Go slices is well implemented.
internal/test_streams_xread_block_max_id.go (4)
24-104: Well-structured test with proper synchronizationThe refactored test uses a more declarative approach with structured test cases, improving readability and maintainability. The synchronization between goroutines using channels is well-implemented, and the use of dedicated assertion types makes the test expectations clear.
34-39: LGTM: Clear test case structureUsing SendCommandTestCase with explicit assertions creates a maintainable test pattern.
49-62: StreamAssertion effectively validates complex RESP dataThe StreamAssertion is used well here to validate the complex nested structure of Redis stream responses. The expected value structure matches Redis stream format, with key, stream ID, and field-value pairs.
96-101: Appropriate use of NilAssertion for blocking XREAD timeoutUsing NilAssertion for the blocking XREAD with timeout correctly validates that the command returns nil when no new entries arrive within the timeout period.
…e StreamAssertion for response validation
andy1li
added
regenerate-fixtures
rohitpaulk
left a comment
rohitpaulk
left a comment
There was a problem hiding this comment.
Added notes around avoiding interface{}{} usage for known structures
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/resp_assertions/xread_response_assertion.go (2)
75-76: Comment is misleading – clarify the nesting level
// Special handling for stream entries (second element of the outer array)
The “outer array” here is each individual stream response, not the top-level XREAD reply. A future maintainer could mis-read this and inadvertently break the logic. Suggest re-wording:- // Special handling for stream entries (second element of the outer array) + // Stream response shape: [ key, <entries> ]. When we’re on index 1 + // inside that 2-element array we need to normalise the nested entries.
66-97: Nil-array handling is incomplete for*-1responses
v.Array()returnsnilfor a RESP*-1(null array).
Inside the “special handling” branch the code immediately doesentries := elem.Array()and thenlen(entries)without checking fornil. This is safe but semantically wrong: a null array carries a different meaning to an empty array.Consider preserving the distinction:
- entries := elem.Array() - typedEntries := make([][]interface{}, len(entries)) + if elem.IsNull() { // add IsNull helper in resp_value + result[i] = nil // keep null vs empty distinction + continue + } + entries := elem.Array() + typedEntries := make([][]interface{}, len(entries))Adding the helper on
Value(e.g.func (v Value) IsNull() bool) lets callers test the case explicitly and avoids conflating “no entries yet” with “key does not exist”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
internal/resp_assertions/xread_response_assertion.go(1 hunks)internal/test_streams_xread_block_max_id.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/test_streams_xread_block_max_id.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/resp_assertions/xread_response_assertion.go (1)
internal/resp/value/value.go (3)
Value(17-24)ARRAY(12-12)BULK_STRING(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
…roved response validation in testStreamsXreadBlockMaxID
…d response assertion in testStreamsXreadBlockMaxID
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/resp_assertions/xread_response_assertion.go (2)
37-47: Switch fromreflect.DeepEqualtocmp.Equalto handle nil/empty comparison
reflect.DeepEqualtreatsnil≠[]any{}and will fail when Redis returns*-1(decoded as anilslice) while the expectation was an empty array.-import "reflect" +import ( + "reflect" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) ... - if !reflect.DeepEqual(expected, actual) { + if !cmp.Equal(expected, actual, cmpopts.EquateEmpty()) {
59-61: Add validation for FieldValuePairs to prevent panicsThe current implementation assumes that every pair has exactly two elements, which could lead to an out-of-range panic if the data is malformed.
- for _, pair := range entry.FieldValuePairs { - flatPairs = append(flatPairs, pair[0], pair[1]) + for _, pair := range entry.FieldValuePairs { + if len(pair) != 2 { + return fmt.Errorf("expected field/value pair of length 2, got %d: %v", len(pair), pair) + } + flatPairs = append(flatPairs, pair[0], pair[1]) }
🧹 Nitpick comments (2)
internal/resp_assertions/xread_response_assertion.go (2)
11-14: Consider following Go naming conventions for ID fieldIn Go, acronyms should be consistently capitalized in identifiers.
type StreamEntry struct { - Id string + ID string FieldValuePairs [][]string }
1-102: Add package documentation and examplesThis is a new assertion type that would benefit from package-level documentation and examples to help users understand how to use it correctly.
+// Package resp_assertions provides utilities for testing Redis RESP protocol responses +// in Go tests. It includes specialized assertions for various Redis commands. package resp_assertions ... +// StreamEntry represents a single entry in a Redis stream with an ID and field-value pairs. type StreamEntry struct { ... +// StreamResponse represents a Redis stream with its key and a collection of entries. type StreamResponse struct { ... +// XReadResponseAssertion verifies that Redis XREAD responses match the expected structure. +// It handles the normalization of both expected and actual values for comparison. type XReadResponseAssertion struct { ... +// NewXReadResponseAssertion creates a new assertion object for Redis XREAD responses. +// The expectedValue should contain the stream responses that are expected to be returned by Redis. func NewXReadResponseAssertion(expectedValue []StreamResponse) RESPAssertion { ... +// Run executes the assertion by comparing the expected stream responses with the actual RESP value. +// It returns an error if the actual value doesn't match the expected structure. func (a XReadResponseAssertion) Run(value resp_value.Value) error { ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
internal/resp_assertions/xread_response_assertion.go(1 hunks)internal/test_streams_xread_block_max_id.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/test_streams_xread_block_max_id.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
internal/resp_assertions/xread_response_assertion.go (1)
1-9: New Redis XREAD assertion capability - looks good!This new package adds the ability to assert against Redis XREAD command responses. The imports look appropriate for the functionality being implemented.
| if i == 1 && len(arr) == 2 { | ||
| entries := elem.Array() | ||
| typedEntries := make([][]interface{}, len(entries)) | ||
| for j, entry := range entries { | ||
| entryArr := entry.Array() | ||
| if len(entryArr) == 2 { | ||
| fvPairs := entryArr[1].Array() | ||
| strPairs := make([]string, len(fvPairs)) | ||
| for k, pair := range fvPairs { | ||
| strPairs[k] = pair.String() | ||
| } | ||
| typedEntries[j] = []interface{}{normalizeActual(entryArr[0]), strPairs} | ||
| } | ||
| } | ||
| result[i] = typedEntries | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for stream entry structure in normalizeActual
The current implementation assumes specific indices in the array structures without proper validation, which could lead to panics if the RESP data has an unexpected structure.
if i == 1 && len(arr) == 2 {
entries := elem.Array()
typedEntries := make([][]interface{}, len(entries))
for j, entry := range entries {
entryArr := entry.Array()
+ if len(entryArr) != 2 {
+ // Handle unexpected array size with a default value or log a warning
+ typedEntries[j] = []interface{}{"invalid_entry", []string{}}
+ continue
+ }
if len(entryArr) == 2 {
fvPairs := entryArr[1].Array()
strPairs := make([]string, len(fvPairs))
for k, pair := range fvPairs {
strPairs[k] = pair.String()
}
typedEntries[j] = []interface{}{normalizeActual(entryArr[0]), strPairs}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if i == 1 && len(arr) == 2 { | |
| entries := elem.Array() | |
| typedEntries := make([][]interface{}, len(entries)) | |
| for j, entry := range entries { | |
| entryArr := entry.Array() | |
| if len(entryArr) == 2 { | |
| fvPairs := entryArr[1].Array() | |
| strPairs := make([]string, len(fvPairs)) | |
| for k, pair := range fvPairs { | |
| strPairs[k] = pair.String() | |
| } | |
| typedEntries[j] = []interface{}{normalizeActual(entryArr[0]), strPairs} | |
| } | |
| } | |
| result[i] = typedEntries | |
| continue | |
| } | |
| if i == 1 && len(arr) == 2 { | |
| entries := elem.Array() | |
| typedEntries := make([][]interface{}, len(entries)) | |
| for j, entry := range entries { | |
| entryArr := entry.Array() | |
| if len(entryArr) != 2 { | |
| // Handle unexpected array size with a default value or log a warning | |
| typedEntries[j] = []interface{}{"invalid_entry", []string{}} | |
| continue | |
| } | |
| if len(entryArr) == 2 { | |
| fvPairs := entryArr[1].Array() | |
| strPairs := make([]string, len(fvPairs)) | |
| for k, pair := range fvPairs { | |
| strPairs[k] = pair.String() | |
| } | |
| typedEntries[j] = []interface{}{normalizeActual(entryArr[0]), strPairs} | |
| } | |
| } | |
| result[i] = typedEntries | |
| continue | |
| } |
| arr := v.Array() | ||
| result := make([]interface{}, len(arr)) | ||
| for i, elem := range arr { | ||
| // Special handling for stream entries (second element of the outer array) |
There was a problem hiding this comment.
Looks good overall but I don't understand why we need this - let's go over on today's call!
…or improved serialization of response values
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/resp_assertions/xread_response_assertion.go (2)
45-47: Replace string comparison with semantic diffUsing string comparison of JSON is brittle - differences in field order or whitespace can cause false negatives. Consider using a semantic comparison library, as previously suggested.
- if string(expectedJSON) != string(actualJSON) { - return fmt.Errorf("XREAD response mismatch:\nExpected:\n%s\nGot:\n%s", expectedJSON, actualJSON) + import ( + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + ) + + if !cmp.Equal(expected, actual, cmpopts.EquateEmpty()) { + return fmt.Errorf("XREAD response mismatch:\nExpected:\n%s\nGot:\n%s", expectedJSON, actualJSON)
73-77: Guard against malformed FieldValuePairs to prevent panicsThere's no validation that every pair in FieldValuePairs has exactly two elements. If the structure is malformed, this could lead to unexpected behavior.
func (e StreamEntry) toRESPValue() resp_value.Value { var fieldValues []resp_value.Value for _, pair := range e.FieldValuePairs { + if len(pair) != 2 { + // Either return an error or log a warning + continue + } for _, v := range pair { fieldValues = append(fieldValues, resp_value.NewBulkStringValue(v)) } }
🧹 Nitpick comments (3)
internal/resp/value/value.go (2)
129-143: Inspect method receiver type for consistencyThe
ToSerializable()method uses a value receiver (func (v Value)) whereas most other methods in this file use pointer receivers (func (v *Value)). While functionally correct, consider using a consistent approach for method receivers.-func (v Value) ToSerializable() interface{} { +func (v *Value) ToSerializable() interface{} {
129-143: Improve handling of INTEGER and NIL typesThe current implementation treats all non-BULK_STRING and non-ARRAY types with the default case, returning the string representation. This may not be ideal for INTEGER and NIL types.
Consider handling INTEGER and NIL types explicitly for more appropriate serialization:
func (v Value) ToSerializable() interface{} { switch v.Type { case BULK_STRING: return v.String() case ARRAY: arr := v.Array() result := make([]interface{}, len(arr)) for i, elem := range arr { result[i] = elem.ToSerializable() } return result + case INTEGER: + return v.Integer() + case NIL: + return nil default: return v.String() } }internal/resp_assertions/xread_response_assertion.go (1)
20-26: Consider adding documentation comments for public types and functionsThe new types and functions are missing documentation comments. Adding these would help users understand their purpose and expected usage.
+// StreamEntry represents a single entry in a Redis stream with an ID and field-value pairs. type StreamEntry struct { Id string FieldValuePairs [][]string } +// StreamResponse represents a Redis stream with a key and multiple entries. type StreamResponse struct { Key string Entries []StreamEntry } +// XReadResponseAssertion holds the expected stream responses for asserting XREAD command results. type XReadResponseAssertion struct { ExpectedStreamResponses []StreamResponse } +// NewXReadResponseAssertion creates a new assertion for validating Redis XREAD command responses. func NewXReadResponseAssertion(expectedValue []StreamResponse) RESPAssertion { return XReadResponseAssertion{ExpectedStreamResponses: expectedValue} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
internal/resp/value/value.go(1 hunks)internal/resp_assertions/xread_response_assertion.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/resp_assertions/xread_response_assertion.go (2)
internal/resp_assertions/assertion.go (1)
RESPAssertion(7-9)internal/resp/value/value.go (4)
Value(17-24)ARRAY(12-12)NewArrayValue(64-69)NewBulkStringValue(33-38)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
3 participants
instrumented_resp_connection.NewFromAddrNew fixture:
Error case:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
New Features