Implementing client commands #5 #52
Conversation
Signed-off-by: Pratheep Kumar <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR implements client connection commands for the Valkey Ruby client, adding comprehensive client management functionality to the library.
Key changes:
- Implements 20+ connection-related commands including
client_id,client_set_name,client_kill,hello,reset, and various client configuration methods - Adds comprehensive test coverage with cluster-aware testing that handles cluster mode limitations
- Introduces helper methods to detect cluster mode in test environments
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/valkey/commands/connection_commands.rb | Implements all client connection commands with proper parameter handling and documentation |
| test/lint/connection_commands.rb | Comprehensive test suite with cluster-aware tests and skip conditions for unimplemented commands |
| test/valkey/connection_commands_test.rb | Test class that includes the connection commands lint tests |
| test/support/helper/client.rb | Adds cluster_mode? helper method returning false for standalone mode |
| test/support/helper/cluster.rb | Adds cluster_mode? helper method returning true for cluster mode |
| test/cluster/cluster_commands_test.rb | Includes connection commands tests in cluster test suite |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def test_select_database | ||
| assert_equal "OK", r.select(0) | ||
|
|
||
| # In cluster mode, only database 0 is supported | ||
| if cluster_mode? | ||
| assert_raises(Valkey::CommandError) do | ||
| r.select(1) | ||
| end | ||
| else | ||
| assert_equal "OK", r.select(1) | ||
| assert_equal "OK", r.select(0) # Switch back to default | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank line on line 28 creates inconsistent spacing. Remove the blank line after r.select(0) to maintain consistent formatting with other test methods.
| def test_client_set_get_name | ||
| name = "lint_test_client" | ||
|
|
||
| # Use the server commands interface that's known to work | ||
| r.client(:set_name, name) | ||
| assert_equal name, r.client(:get_name) | ||
|
|
||
| # Clear client name | ||
| r.client(:set_name, "") | ||
| assert_nil r.client(:get_name) | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank lines on lines 47 and 51 create inconsistent spacing. Remove these blank lines to maintain consistent formatting with other test methods.
| def test_client_set_info | ||
| # Use the server commands interface that's known to work | ||
| assert_equal "OK", r.client(:set_info, "lib-name", "valkey-ruby") | ||
|
|
||
| assert_raises(Valkey::CommandError) do | ||
| r.client(:set_info, "invalid-attr", "value") | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank line on line 86 creates inconsistent spacing. Remove the blank line before the assert_raises block to maintain consistent formatting.
| def test_client_no_evict | ||
| # Use the server commands interface that's known to work | ||
| assert_equal "OK", r.client_no_evict(:on) | ||
| assert_equal "OK", r.client_no_evict(:off) | ||
|
|
||
| assert_raises(Valkey::CommandError) do | ||
| r.client_no_evict(:invalid) | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank line on line 103 creates inconsistent spacing. Remove the blank line before the assert_raises block to maintain consistent formatting.
| def test_client_no_touch | ||
| # Use the server commands interface that's known to work | ||
| assert_equal "OK", r.client_no_touch(:on) | ||
| assert_equal "OK", r.client_no_touch(:off) | ||
|
|
||
| assert_raises(Valkey::CommandError) do | ||
| r.client_no_touch(:invalid) | ||
| end | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank line on line 113 creates inconsistent spacing. Remove the blank line before the assert_raises block to maintain consistent formatting.
| def test_hello_with_setname | ||
| skip("HELLO command not implemented in backend yet") | ||
|
|
||
| client_name = "hello_lint_test" | ||
| result = r.hello(3, setname: client_name) | ||
| assert_kind_of Hash, result | ||
| assert_equal client_name, r.client_get_name | ||
| end |
There was a problem hiding this comment.
[nitpick] Extra blank line on line 144 creates inconsistent spacing. Remove the blank line after the skip statement to maintain consistent formatting.
|
|
||
| # Set some state | ||
| r.client_set_name("before_reset") | ||
|
|
||
| # In cluster mode, we can only use database 0 | ||
| unless cluster_mode? | ||
| r.select(1) | ||
| end | ||
|
|
||
| # Reset | ||
| result = r.reset | ||
| assert_equal "RESET", result | ||
|
|
There was a problem hiding this comment.
[nitpick] Multiple extra blank lines throughout the method (lines 153, 156, 161, 165) create inconsistent spacing. Remove these blank lines to maintain consistent formatting with other test methods.
| # Set some state | |
| r.client_set_name("before_reset") | |
| # In cluster mode, we can only use database 0 | |
| unless cluster_mode? | |
| r.select(1) | |
| end | |
| # Reset | |
| result = r.reset | |
| assert_equal "RESET", result | |
| # Set some state | |
| r.client_set_name("before_reset") | |
| # In cluster mode, we can only use database 0 | |
| unless cluster_mode? | |
| r.select(1) | |
| end | |
| # Reset | |
| result = r.reset | |
| assert_equal "RESET", result |
|
|
||
| def test_client_caching | ||
| skip("CLIENT CACHING command not implemented in backend yet") | ||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line on line 172 creates inconsistent spacing. Remove the blank line after the skip statement to maintain consistent formatting.
|
|
||
| def test_client_tracking | ||
| skip("CLIENT TRACKING command not implemented in backend yet") | ||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line on line 179 creates inconsistent spacing. Remove the blank line after the skip statement to maintain consistent formatting.
|
|
||
| def test_client_tracking_info | ||
| skip("CLIENT TRACKING command not implemented in backend yet") | ||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line on line 186 creates inconsistent spacing. Remove the blank line after the skip statement to maintain consistent formatting.
No description provided.