Skip to content

Add tests for write! edge cases in Circular Buffer #124

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.

Sign up for GitHub

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

Merged
kytrinyx merged 1 commit into from
May 5, 2015
Merged

Add tests for write! edge cases in Circular Buffer #124

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions circular-buffer/circular_buffer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,26 @@ def test_overwriting_oldest_item_in_a_full_buffer
assert_raises(CircularBuffer::BufferEmptyException) { buffer.read }
end

def test_forced_writes_of_nil_should_not_occupy_buffer
skip
buffer = CircularBuffer.new(2)
(1..2).each { |i| buffer.write String(i) }
buffer.write! nil
Copy link
Copy Markdown
Member

@kytrinyx kytrinyx May 5, 2015

Choose a reason for hiding this comment

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

This is actually an interesting question overall... it seems odd to me now that a nil would be treated differently than any another value, but I see that we're already doing this for the write with a nil.

Let's do this for now, but also be open to having the discussion about what nil actually means in the context of a circular buffer.

Copy link
Copy Markdown
Contributor Author

@julianandrews julianandrews May 5, 2015

Choose a reason for hiding this comment

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

I had the a similar thought. It's notable that all of the test cases only use strings as values for the buffer. If the intent is a circular buffer for strings only, then probably instead of silently failing on a nil write, the buffer should throw an exception on any non-string value.

If instead the intent is a buffer that can accept arbitrary objects, then nil should be a valid thing to write. Of course, that would make the exercise a notch more difficult since the simple approach of using a nil filled array wouldn't work anymore, but it would also be more general an correct.

Since the buffer is suppose to raise an error if you try to read from it empty, there's no scenario where nil is used to represent an empty value or anything like that.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx May 7, 2015

Choose a reason for hiding this comment

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

Agreed, I like the idea of making this a circular string buffer, and to have any non-string values raise errors. We could have several tests to that effect (nil, fixnum, Object.new, whatever), or one test with several different types of things.

Copy link
Copy Markdown
Contributor Author

@julianandrews julianandrews May 7, 2015

Choose a reason for hiding this comment

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

I could write a fix to replace the nil write tests with tests that check a representative set of non-string values all throw an ArgumentError (or some custom exception). In that case it might also make sense to have some of the tests test longer strings (not just single character strings) just to make it clear that any strings (but only strings) are valid inputs.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx May 8, 2015

Choose a reason for hiding this comment

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

Yes, longer strings would help clarify.

assert_equal '1', buffer.read
assert_equal '2', buffer.read
assert_raises(CircularBuffer::BufferEmptyException) { buffer.read }
end

def test_forced_writes_to_non_full_buffer_should_behave_like_writes
skip
buffer = CircularBuffer.new(2)
buffer.write '1'
buffer.write! '2'
assert_equal '1', buffer.read
assert_equal '2', buffer.read
assert_raises(CircularBuffer::BufferEmptyException) { buffer.read }
end

# rubocop:disable Metrics/MethodLength
def test_alternate_read_and_write_into_buffer_overflow
skip
Expand Down