Open
Conversation
Reviewer's GuideAdds low-level Redis protocol helpers and expands integration tests to fully exercise Redis containers (including auth, commands, logs, and lifecycle), while scoping CI to run only the integration spec file. Sequence diagram for Redis full integration test executionsequenceDiagram
participant CI as GitHub_Actions_Job
participant Spec as RedisIntegrationSpec
participant Helper as RedisProtocolHelper
participant RC as RedisContainer
CI->>Spec: run crystal_spec_spec_integration_spec_cr
Spec->>RC: start_container
RC-->>Spec: container_ready
Spec->>Helper: connect host port password
Helper->>RC: open_tcp_connection
RC-->>Helper: connection_established
Spec->>Helper: send_auth password
Helper->>RC: send_auth_command
RC-->>Helper: auth_ok
Spec->>Helper: send_command command args
Helper->>RC: write_redis_protocol
RC-->>Helper: command_response
Helper-->>Spec: parsed_response
Spec->>RC: fetch_logs
RC-->>Spec: container_logs
Spec->>RC: stop_container
RC-->>Spec: container_stopped
Class diagram for Redis protocol helpers and integration specclassDiagram
class RedisProtocolHelper {
+String host
+Int32 port
+String password
+connect(String host, Int32 port, String password)
+send_auth(String password)
+send_command(String command, Array args)
+read_response()
}
class RedisAuthHelper {
+String password
+build_auth_command(String password)
}
class RedisCommandHelper {
+build_command(String command, Array args)
+encode_bulk_string(String value)
+encode_array(Array values)
}
class RedisLifecycleHelper {
+start_container()
+stop_container()
+restart_container()
+fetch_logs()
}
class RedisIntegrationSpec {
+setup()
+teardown()
+test_authentication()
+test_basic_commands()
+test_logging()
+test_container_lifecycle()
}
RedisIntegrationSpec --> RedisProtocolHelper : uses
RedisIntegrationSpec --> RedisLifecycleHelper : controls
RedisProtocolHelper --> RedisAuthHelper : delegates_auth_build
RedisProtocolHelper --> RedisCommandHelper : delegates_command_build
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe changes add Redis protocol helpers to integration tests and modify the CI workflow to explicitly target a named integration spec file. Redis RESP command/response helpers enable direct TCP interactions with Redis containers, expanding test coverage to validate Redis connectivity, URL parsing, and AUTH flows against containerized Redis instances. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant RC as redis_command()
participant TCP as TCP Socket
participant Redis as Redis Container
Test->>RC: redis_command(url, "PING")
RC->>RC: Parse URL & extract host, port, password
RC->>TCP: Socket.tcp(host, port)
TCP-->>RC: Connected
alt Password present
RC->>RC: send_redis_command(io, ["AUTH", password])
RC->>TCP: Write RESP AUTH command
TCP->>Redis: AUTH command
Redis-->>TCP: +OK response
RC->>RC: read_redis_response(io)
RC-->>RC: Parse +OK
end
RC->>RC: send_redis_command(io, ["PING"])
RC->>TCP: Write RESP PING command
TCP->>Redis: PING command
Redis-->>TCP: +PONG response
RC->>RC: read_redis_response(io)
RC-->>RC: Parse PONG
RC-->>Test: Return "PONG"
Test->>Test: Assert response matches expected value
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
sleep 1.secondin the Redis logs test is a fixed delay that may be flaky on slower environments; consider polling for the expected log line with a timeout instead of sleeping a fixed duration. - The CI step now runs only
spec/integration_spec.cr; if additional integration specs are added later they will be skipped, so consider keeping a pattern (e.g.spec/*_integration_spec.cror the full suite with an-Dintegrationfilter) to avoid unintentionally omitting tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `sleep 1.second` in the Redis logs test is a fixed delay that may be flaky on slower environments; consider polling for the expected log line with a timeout instead of sleeping a fixed duration.
- The CI step now runs only `spec/integration_spec.cr`; if additional integration specs are added later they will be skipped, so consider keeping a pattern (e.g. `spec/*_integration_spec.cr` or the full suite with an `-Dintegration` filter) to avoid unintentionally omitting tests.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4 +/- ##
=======================================
Coverage 61.14% 61.14%
=======================================
Files 14 14
Lines 350 350
=======================================
Hits 214 214
Misses 136 136
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/integration_spec.cr`:
- Around line 44-66: The redis_command helpers (both overloads) should use a
connect timeout and retry loop around the TCPSocket.open call to avoid transient
"Connection reset by peer" failures: wrap the TCPSocket.open(host, port) call in
a small retry loop (e.g., max attempts + backoff sleep) that rescues transient
socket errors (Errno::ECONNREFUSED, Errno::ECONNRESET, IO::Timeout, etc.) and
retries, and when opening the socket use a connect-with-timeout approach (or set
socket read/write timeouts) before calling send_redis_command and
read_redis_response; preserve existing AUTH logic in the
redis_command(redis_url, ...) overload so AUTH is retried only after a
successful socket open and still raises if auth_response != "OK".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b19e5a56-7ab9-4fe2-a458-fab850f77f6b
📒 Files selected for processing (2)
.github/workflows/ci.ymlspec/integration_spec.cr
Comment on lines
+44
to
+66
| def redis_command(host : String, port : Int32, *parts : String) : String | ||
| TCPSocket.open(host, port) do |socket| | ||
| send_redis_command(socket, parts.to_a) | ||
| read_redis_response(socket) | ||
| end | ||
| end | ||
|
|
||
| def redis_command(redis_url : String, *parts : String) : String | ||
| uri = URI.parse(redis_url) | ||
| host = uri.host || raise "Redis URL is missing host" | ||
| port = uri.port || 6379 | ||
|
|
||
| TCPSocket.open(host, port) do |socket| | ||
| if password = uri.password | ||
| send_redis_command(socket, ["AUTH", password]) | ||
| auth_response = read_redis_response(socket) | ||
| raise "Redis AUTH failed: #{auth_response}" unless auth_response == "OK" | ||
| end | ||
|
|
||
| send_redis_command(socket, parts.to_a) | ||
| read_redis_response(socket) | ||
| end | ||
| end |
There was a problem hiding this comment.
Add timeout and retry logic to prevent flaky tests.
The pipeline failures showing "Connection reset by peer" suggest a race condition between the container's log output and TCP port readiness. TCPSocket.open should include timeouts, and the helper should retry on transient failures.
🛠️ Proposed fix with timeout and retry logic
def redis_command(host : String, port : Int32, *parts : String) : String
- TCPSocket.open(host, port) do |socket|
- send_redis_command(socket, parts.to_a)
- read_redis_response(socket)
+ retries = 3
+ begin
+ TCPSocket.open(host, port, connect_timeout: 5.seconds) do |socket|
+ socket.read_timeout = 5.seconds
+ send_redis_command(socket, parts.to_a)
+ read_redis_response(socket)
+ end
+ rescue ex : IO::Error | Socket::ConnectError
+ retries -= 1
+ if retries > 0
+ sleep 0.5.seconds
+ retry
+ end
+ raise ex
end
end
def redis_command(redis_url : String, *parts : String) : String
uri = URI.parse(redis_url)
host = uri.host || raise "Redis URL is missing host"
port = uri.port || 6379
- TCPSocket.open(host, port) do |socket|
- if password = uri.password
- send_redis_command(socket, ["AUTH", password])
- auth_response = read_redis_response(socket)
- raise "Redis AUTH failed: #{auth_response}" unless auth_response == "OK"
+ retries = 3
+ begin
+ TCPSocket.open(host, port, connect_timeout: 5.seconds) do |socket|
+ socket.read_timeout = 5.seconds
+ if password = uri.password
+ send_redis_command(socket, ["AUTH", password])
+ auth_response = read_redis_response(socket)
+ raise "Redis AUTH failed: #{auth_response}" unless auth_response == "OK"
+ end
+
+ send_redis_command(socket, parts.to_a)
+ read_redis_response(socket)
end
-
- send_redis_command(socket, parts.to_a)
- read_redis_response(socket)
+ rescue ex : IO::Error | Socket::ConnectError
+ retries -= 1
+ if retries > 0
+ sleep 0.5.seconds
+ retry
+ end
+ raise ex
end
end📝 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.
Suggested change
| def redis_command(host : String, port : Int32, *parts : String) : String | |
| TCPSocket.open(host, port) do |socket| | |
| send_redis_command(socket, parts.to_a) | |
| read_redis_response(socket) | |
| end | |
| end | |
| def redis_command(redis_url : String, *parts : String) : String | |
| uri = URI.parse(redis_url) | |
| host = uri.host || raise "Redis URL is missing host" | |
| port = uri.port || 6379 | |
| TCPSocket.open(host, port) do |socket| | |
| if password = uri.password | |
| send_redis_command(socket, ["AUTH", password]) | |
| auth_response = read_redis_response(socket) | |
| raise "Redis AUTH failed: #{auth_response}" unless auth_response == "OK" | |
| end | |
| send_redis_command(socket, parts.to_a) | |
| read_redis_response(socket) | |
| end | |
| end | |
| def redis_command(host : String, port : Int32, *parts : String) : String | |
| retries = 3 | |
| begin | |
| TCPSocket.open(host, port, connect_timeout: 5.seconds) do |socket| | |
| socket.read_timeout = 5.seconds | |
| send_redis_command(socket, parts.to_a) | |
| read_redis_response(socket) | |
| end | |
| rescue ex : IO::Error | Socket::ConnectError | |
| retries -= 1 | |
| if retries > 0 | |
| sleep 0.5.seconds | |
| retry | |
| end | |
| raise ex | |
| end | |
| end | |
| def redis_command(redis_url : String, *parts : String) : String | |
| uri = URI.parse(redis_url) | |
| host = uri.host || raise "Redis URL is missing host" | |
| port = uri.port || 6379 | |
| retries = 3 | |
| begin | |
| TCPSocket.open(host, port, connect_timeout: 5.seconds) do |socket| | |
| socket.read_timeout = 5.seconds | |
| if password = uri.password | |
| send_redis_command(socket, ["AUTH", password]) | |
| auth_response = read_redis_response(socket) | |
| raise "Redis AUTH failed: #{auth_response}" unless auth_response == "OK" | |
| end | |
| send_redis_command(socket, parts.to_a) | |
| read_redis_response(socket) | |
| end | |
| rescue ex : IO::Error | Socket::ConnectError | |
| retries -= 1 | |
| if retries > 0 | |
| sleep 0.5.seconds | |
| retry | |
| end | |
| raise ex | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/integration_spec.cr` around lines 44 - 66, The redis_command helpers
(both overloads) should use a connect timeout and retry loop around the
TCPSocket.open call to avoid transient "Connection reset by peer" failures: wrap
the TCPSocket.open(host, port) call in a small retry loop (e.g., max attempts +
backoff sleep) that rescues transient socket errors (Errno::ECONNREFUSED,
Errno::ECONNRESET, IO::Timeout, etc.) and retries, and when opening the socket
use a connect-with-timeout approach (or set socket read/write timeouts) before
calling send_redis_command and read_redis_response; preserve existing AUTH logic
in the redis_command(redis_url, ...) overload so AUTH is retried only after a
successful socket open and still raises if auth_response != "OK".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Brief description of the changes made in this PR.
Type of Change
Checklist
Testing
Describe the tests you ran to verify your changes:
Additional Notes
Add any other context about the PR here.
Summary by Sourcery
Expand Redis integration coverage and tighten container lifecycle validation.
Tests:
Summary by CodeRabbit
Tests
Chores