From 9df4b4f0431f40012496e2a6ef6561496e7b1e49 Mon Sep 17 00:00:00 2001 From: Emilio Cristalli Date: Fri, 13 Sep 2024 18:04:46 -0300 Subject: [PATCH] Add support for optional `count` arg in `rpop` (#308) `rpop` accepts an optional `count` argument to indicate how many elements should be removed and returned from the list See https://github.com/redis/redis-rb/blob/9938411bd44383b795e05df900abce4df66daaef/lib/redis/commands/lists.rb#L114 Also had to change the shared examples a little bit to be able to pass the arguments they use and make a more accurate expectation on the error. I think the `args_for_method` is making an assumption when the `arity < 0` and always using `[1, 2]` (+ the key), but that doesn't work in all cases. In particular, `rpop` now has `arity` `-2` (because it has 1 required arg + 1 optional) so calling `rpop(key, 1, 2)` was causing an argument error instead of `Redis::CommandError` (which we expect because of the redis value not being a list). At first I tried to change `args_for_method` but it made other tests fail. And i suspect it won't be possible to have a generic args generator only based on arity (because some methods for example accept `*args` but the logic requires 1 or 2 args) That's why i thought it might be a good idea for each test that includes the shared example to indicate what the correct args to make a valid call should be, but let me know what you think! --- lib/mock_redis/list_methods.rb | 9 +++-- spec/commands/rpop_spec.rb | 35 +++++++++++++++++-- .../does_not_cleanup_empty_strings.rb | 14 ++++---- .../shared_examples/only_operates_on_lists.rb | 17 +++++---- 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/lib/mock_redis/list_methods.rb b/lib/mock_redis/list_methods.rb index 71f9ef4..36342a7 100644 --- a/lib/mock_redis/list_methods.rb +++ b/lib/mock_redis/list_methods.rb @@ -186,8 +186,13 @@ def ltrim(key, start, stop) end end - def rpop(key) - with_list_at(key) { |list| list&.pop } + def rpop(key, count = nil) + return with_list_at(key) { |list| list&.pop } if count.nil? + + record_count = llen(key) + return nil if record_count.zero? + + [record_count, count].min.times.map { with_list_at(key, &:pop) } end def rpoplpush(source, destination) diff --git a/spec/commands/rpop_spec.rb b/spec/commands/rpop_spec.rb index 9176da6..cf7d084 100644 --- a/spec/commands/rpop_spec.rb +++ b/spec/commands/rpop_spec.rb @@ -30,6 +30,37 @@ expect(@redises.get(@key)).to be_nil end - let(:default_error) { RedisMultiplexer::MismatchedResponse } - it_should_behave_like 'a list-only command' + context 'when count != nil' do + it 'returns array with one element if count == 1' do + @redises.rpush(@key, %w[one two three four five]) + + expect(@redises.rpop(@key, 1)).to eq(%w[five]) + expect(@redises.lrange(@key, 0, -1)).to eq(%w[one two three four]) + end + + it 'returns the number of records requested' do + @redises.rpush(@key, %w[one two three four five]) + + expect(@redises.rpop(@key, 2)).to eq(%w[five four]) + expect(@redises.lrange(@key, 0, -1)).to eq(%w[one two three]) + end + + it 'returns nil for nonexistent key' do + expect(@redises.rpop(@key, 2)).to be_nil + end + + it 'returns all records when requesting more than list length' do + @redises.rpush(@key, %w[one two three]) + + expect(@redises.rpop(@key, 10)).to eq(%w[three two one]) + expect(@redises.lrange(@key, 0, -1)).to eq([]) + end + end + + it_should_behave_like 'a list-only command' do + let(:args) { [1] } + let(:error) do + [Redis::CommandError, 'WRONGTYPE Operation against a key holding the wrong kind of value'] + end + end end diff --git a/spec/support/shared_examples/does_not_cleanup_empty_strings.rb b/spec/support/shared_examples/does_not_cleanup_empty_strings.rb index d6b145c..bac2bbc 100644 --- a/spec/support/shared_examples/does_not_cleanup_empty_strings.rb +++ b/spec/support/shared_examples/does_not_cleanup_empty_strings.rb @@ -1,14 +1,16 @@ RSpec.shared_examples_for 'does not remove empty strings on error' do - it 'does not remove empty strings on error' do |example| - key = 'mock-redis-test:not-a-string' + let(:method) { |example| method_from_description(example) } + let(:args) { args_for_method(method) } + let(:error) { defined?(default_error) ? default_error : RuntimeError } - method = method_from_description(example) - args = args_for_method(method).unshift(key) + it 'does not remove empty strings on error' do + key = 'mock-redis-test:not-a-string' + key_and_args = args.unshift(key) @redises.set(key, '') expect do - @redises.send(method, *args) - end.to raise_error(defined?(default_error) ? default_error : RuntimeError) + @redises.send(method, *key_and_args) + end.to raise_error(*error) expect(@redises.get(key)).to eq('') end end diff --git a/spec/support/shared_examples/only_operates_on_lists.rb b/spec/support/shared_examples/only_operates_on_lists.rb index 442cc7d..966a387 100644 --- a/spec/support/shared_examples/only_operates_on_lists.rb +++ b/spec/support/shared_examples/only_operates_on_lists.rb @@ -1,15 +1,18 @@ RSpec.shared_examples_for 'a list-only command' do - it 'raises an error for non-list values' do |example| - key = 'mock-redis-test:list-only' + let(:method) { |example| method_from_description(example) } + let(:args) { args_for_method(method) } + let(:error) { defined?(default_error) ? default_error : RuntimeError } - method = method_from_description(example) - args = args_for_method(method).unshift(key) + it 'raises an error for non-list values' do + key = 'mock-redis-test:list-only' + key_and_args = args.unshift(key) @redises.set(key, 1) + expect do - @redises.send(method, *args) - end.to raise_error(defined?(default_error) ? default_error : RuntimeError) + @redises.send(method, *key_and_args) + end.to raise_error(*error) end - it_should_behave_like 'does not remove empty strings on error' + include_examples 'does not remove empty strings on error' end