fix: go e2e test fail #168
Conversation
WalkthroughThe pull request introduces a consistent modification to error handling across multiple Redis client test files. The primary change involves updating test assertions to be more flexible when checking for errors, particularly when dealing with non-existent keys or operations. Instead of strictly checking for Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (1)
tests/consistency_test.go (1)
909-912: Excellent improvement in test resilience.The systematic change to use
SatisfyAny(BeNil(), MatchError(redis.Nil))across test files improves the test suite's robustness by:
- Accommodating different Redis server states
- Maintaining consistent error handling patterns
- Better reflecting real-world Redis behavior
This architectural improvement helps prevent flaky tests while maintaining correctness.
Also applies to: 83-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/admin_test.go(2 hunks)tests/consistency_test.go(1 hunks)tests/hash_test.go(1 hunks)tests/key_test.go(5 hunks)tests/list_test.go(1 hunks)tests/string_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (7)
tests/admin_test.go (2)
114-117: Consistent with the error handling changes in string_test.go.The modification follows the same pattern of accepting both
nilandredis.Nilerrors, maintaining consistency across test files.
160-160: Error message updated to match Redis behavior.The error message for invalid authentication has been updated from "ERR Client sent AUTH, but no password is set" to "ERR invalid password", which better reflects the actual Redis server response.
tests/list_test.go (1)
316-319: Consistent error handling for out-of-range list index.The modification aligns with the pattern of accepting both
nilandredis.Nilerrors across the test suite.tests/key_test.go (2)
242-245: Consistent error handling for expired keys.The modifications maintain consistency with the flexible error handling pattern seen across other test files.
Also applies to: 318-321, 341-344, 362-365, 423-426
419-419: Improved PTTL assertion accuracy.The PTTL assertions have been updated to properly compare time.Duration values, making the tests more accurate.
The change from:
-Expect(client.PTTL(ctx, DefaultKey).Val()).NotTo(Equal(time.Duration(-2))) +Expect(client.PTTL(ctx, DefaultKey).Val()).NotTo(Equal(time.Duration(-2 * time.Second)))correctly specifies the duration unit.
Also applies to: 422-422
tests/consistency_test.go (1)
909-912: LGTM! More flexible error handling.The change improves error handling by accepting both
nilandredis.Nilerrors, which makes the test more robust against different Redis server states.tests/hash_test.go (1)
83-86: LGTM! Consistent error handling improvement.The change aligns with the project-wide improvement in error handling, making the test more resilient by accepting both
nilandredis.Nilerrors for non-existent hash fields.
| Expect(e).To(SatisfyAny( | ||
| BeNil(), | ||
| MatchError(redis.Nil), | ||
| )) |
There was a problem hiding this comment.
💡 Codebase verification
Consider standardizing error handling to use only redis.Nil
The codebase consistently uses redis.Nil for non-existent keys across different Redis operations. The flexible error handling (accepting both nil and redis.Nil) in this test is an exception to this pattern and might mask underlying issues.
- Consider updating lines 88-91 and 97-100 to only match
redis.Nil, consistent with other tests in:- list_test.go
- key_test.go
- hash_test.go
- admin_test.go
🔗 Analysis chain
Consider documenting the rationale for flexible error handling.
The change to accept both nil and redis.Nil errors makes the tests more resilient, but it might be masking inconsistent behavior in the Redis client implementation.
Let's verify if this is a known behavior in the Redis client:
Also applies to: 97-100
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this behavior is documented in the Redis client
rg -A 5 "redis.Nil" README.md
# Check for similar patterns in other Redis client implementations
ast-grep --pattern 'Expect($x.Err()).To(MatchError(redis.Nil))'
Length of output: 95
Script:
#!/bin/bash
# Find Redis-related files
fd -t f redis
# Search for Redis error handling patterns
rg -i "redis.*nil" -A 3
# Check docs directory and other common documentation locations
fd -t f -e md
Length of output: 2516
| @@ -215,7 +221,7 @@ | |||
|
|
|||
| mGet := client.MGet(ctx, "key1", "key2", "_") | |||
| Expect(mGet.Err()).NotTo(HaveOccurred()) | |||
| Expect(mGet.Val()).To(Equal([]interface{}{"hello1", "hello2", nil})) | |||
| Expect(mGet.Val()).To(Equal([]interface{}{"hello1", "hello2", ""})) | |||
There was a problem hiding this comment.
💡 Codebase verification
Test suite contains inconsistent expectations for MGet behavior with non-existent keys
The implementation in cmd_kv.cc explicitly returns empty string ("") for non-existent keys, which matches the change under review. However, there's an inconsistency in the test suite where the MSetnx test still expects nil for non-existent keys. The MSetnx test case should be updated to expect empty strings instead of nil values to match the implementation.
tests/string_test.go: Update MSetnx test case expectations fromnilto""for non-existent keys
🔗 Analysis chain
Verify the change in MGet's behavior for non-existent keys.
The expectation for non-existent keys in MGet results has been changed from nil to empty string (""). This change should be verified against Redis specifications.
Let's verify this behavior against Redis documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MGET documentation and behavior
rg -A 5 "MGET.*nil" docs/
# Check for similar patterns in other test files
rg -A 5 'MGet.*nil'
Length of output: 95
Script:
#!/bin/bash
# Search for MGet test cases and implementation
rg -l 'MGet'
# Search for test expectations with MGet
rg -A 3 'Expect.*MGet'
# Look for Redis client configuration or setup
rg -A 5 'type.*Redis.*Client'
Length of output: 413
Script:
#!/bin/bash
# Check implementation and comments in source files
rg -A 10 'MGet' src/cmd_kv.cc
# Look for other test cases in the same test file
cat tests/string_test.go
# Check for any related configuration or constants
rg -B 2 -A 2 'nil|empty.*string' tests/string_test.go
Length of output: 9231
#152
Summary by CodeRabbit
nilandredis.Nilerrors.nilto an empty string"".These changes improve test robustness by providing more flexible error handling and result validation across different Redis client operations.