-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
circular-buffer: Update the tests following the canonical data #1047
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
Conversation
@@ -7,40 +7,37 @@ | |||
) | |||
|
|||
|
|||
# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting point, but can you add a blank line after the version string for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
def test_read_just_written_item(self): | ||
buf = CircularBuffer(1) | ||
buf.write('1') | ||
self.assertEqual('1', buf.read()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention, we use assertEqual(actual, expected)
for our tests - can you switch the argument order in all of these tests?
You can check the rest of our conventions in the main README.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed then. Thanks to point it out
buf.write('3') | ||
self.assertEqual(buf.read(), '2') | ||
with self.assertRaises(BufferFullException): | ||
buf.write('2') | ||
|
||
def test_alternate_write_and_read(self): | ||
buf = CircularBuffer(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this test has been changed in the canonical data - it should be a CircularBuffer(1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed that. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but there are a few changes that need to be made before merging.
@N-Parsons can you check again? Thanks |
Looks good. Thanks, @elyssonmr! |
…ism#1047) * circular-buffer: Update the tests following the canonical data
Update the tests following the
canonical data
from issue #991