Skip to content

Commit

Permalink
Add support for optional count arg in rpop (#308)
Browse files Browse the repository at this point in the history
`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!
  • Loading branch information
EmilioCristalli committed Sep 13, 2024
1 parent 3cd9501 commit 9df4b4f
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 17 deletions.
9 changes: 7 additions & 2 deletions lib/mock_redis/list_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
35 changes: 33 additions & 2 deletions spec/commands/rpop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
14 changes: 8 additions & 6 deletions spec/support/shared_examples/does_not_cleanup_empty_strings.rb
Original file line number Diff line number Diff line change
@@ -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
17 changes: 10 additions & 7 deletions spec/support/shared_examples/only_operates_on_lists.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 9df4b4f

Please sign in to comment.