Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redis-clustering gem to pass the test with latest dependencies #1222

Merged
merged 1 commit into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ jobs:
env:
TIMEOUT: "15"
LOW_TIMEOUT: "0.14"
REDIS_BRANCH: "7.0"
REDIS_BRANCH: "7.2"
BUNDLE_GEMFILE: cluster/Gemfile
steps:
- name: Check out code
Expand Down
4 changes: 2 additions & 2 deletions cluster/lib/redis/cluster/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ def initialize(*)
ruby2_keywords :initialize if respond_to?(:ruby2_keywords, true)

def id
@router.node.node_keys.join(' ')
@router.node_keys.join(' ')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

def server_url
@router.node.node_keys
@router.node_keys
end

def connected?
Expand Down
2 changes: 1 addition & 1 deletion cluster/redis-clustering.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,5 @@ Gem::Specification.new do |s|
s.required_ruby_version = '>= 2.7.0'

s.add_runtime_dependency('redis', s.version)
s.add_runtime_dependency('redis-cluster-client', '>= 0.3.7')
s.add_runtime_dependency('redis-cluster-client', '>= 0.6.1')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end
1 change: 0 additions & 1 deletion cluster/test/commands_on_keys_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ def test_migrate
def test_object
redis.lpush('mylist', 'Hello World')
assert_equal 1, redis.object('refcount', 'mylist')
assert_equal 'quicklist', redis.object('encoding', 'mylist')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed to 'listpack' in Redis 7.2. We have no advantages for testing redis minor behavior, so just removed.

assert(redis.object('idletime', 'mylist') >= 0)

redis.set('foo', 1000)
Expand Down
8 changes: 2 additions & 6 deletions cluster/test/commands_on_server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,8 @@ def test_client_kill

def test_client_list
a_client_info = redis.client(:list).first
actual = a_client_info.keys.sort
expected = %w[addr age cmd db events fd flags id idle multi name obl oll omem psub qbuf qbuf-free sub]
expected << 'user' << 'argv-mem' << 'tot-mem' if version >= '6'
expected << 'laddr' << 'redir' if version >= '6.2'
expected << "multi-mem" << "rbp" << "rbs" << "resp" << "ssub" if version >= '7.0'
assert_equal expected.sort, actual.sort
assert_instance_of Hash, a_client_info
assert_includes a_client_info, 'addr'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • It looks like server side changes for reply type from Array to Hash.
  • I simplified this test case rather than meticulous assertion because it is weak against server side minor changes.

end

def test_client_getname
Expand Down
Loading