Add CircularBuffer class and tests #501
Conversation
spec/utility/circular_buffer_spec.rb
Outdated
| require 'spec_helper' | ||
|
|
||
| describe Bugsnag::Utility::CircularBuffer do | ||
| describe "buffer size" do |
There was a problem hiding this comment.
For unit tests, I prefer to have the 2nd level describes use the method names as the doc strings (e.g. this would be describe '#max_items', lines 19-24 would be in its own describe '#max_items=')
There was a problem hiding this comment.
Personally I prefer tests to be based around expectations of functionality rather than necessarily method testing, as I find the buffer size can be changed is more useful when reading the tests than max_items=. However, this is mostly semantics and I don't mind changing the test focus if it helps others read them.
There was a problem hiding this comment.
I tend to prefer the describe blocks with method names because it makes some aspects of organization better. For example, if someone changes a public method, the area of the corresponding spec file that needs changing tends to be obvious. It also helps when reviewing a particular public method because the reviewer will have an easier time checking that all the corresponding unit tests for the method cover and test the method sufficiently.
However, there are cases where using describe blocks with method names as the doc strings is awkward, such as some data structure classes (for example, in this class, there can be a debate on whether the "can be set during initialization" case should belong in describe '#initialize' or describe '#max_items' under the method name doc string approach). Perhaps we should seek another opinion?
There was a problem hiding this comment.
I'll restructure around methods, as I agree it may be preferable.
There was a problem hiding this comment.
As for the max_items being set in initialize, I don't think it matters where it is, as long as the doc string accurately reflects the test being performed. For now I've popped it in #max_items.
spec/utility/circular_buffer_spec.rb
Outdated
|
|
||
| require 'spec_helper' | ||
|
|
||
| describe Bugsnag::Utility::CircularBuffer do |
There was a problem hiding this comment.
For new spec files, we should probably use RSpec.describe for the top-level one (so we can eventually turn off the global DSL described by https://relishapp.com/rspec/rspec-core/v/3-0/docs/configuration/global-namespace-dsl )
There was a problem hiding this comment.
Is there a particular reason to do this?
There was a problem hiding this comment.
Yes, RSpec 4.0 might disable all or a lot of the unnecessary monkey patching by default (search for "RSpec 4.0 will have zero monkey patching out of the box" in http://rspec.info/blog/2013/07/the-plan-for-rspec-3/ ), so it might be preferable to start getting in the habit of avoiding some things like the top-level global describe, should, should_not, should_receive, etc.
There was a problem hiding this comment.
Gotcha, I'll change it over.
spec/utility/circular_buffer_spec.rb
Outdated
| expect(buffer.to_a).to eq([]) | ||
| end | ||
|
|
||
| it "can be added to with <<" do |
There was a problem hiding this comment.
Lines 34-60 should probably be in a describe '#<<' block instead (and maybe lines 62-74 in describe '#each', and lines 76-89 in the same describe '#max_items=' described by a previous comment)
spec/utility/circular_buffer_spec.rb
Outdated
|
|
||
| require 'spec_helper' | ||
|
|
||
| describe Bugsnag::Utility::CircularBuffer do |
There was a problem hiding this comment.
Yes, RSpec 4.0 might disable all or a lot of the unnecessary monkey patching by default (search for "RSpec 4.0 will have zero monkey patching out of the box" in http://rspec.info/blog/2013/07/the-plan-for-rspec-3/ ), so it might be preferable to start getting in the habit of avoiding some things like the top-level global describe, should, should_not, should_receive, etc.
spec/utility/circular_buffer_spec.rb
Outdated
| require 'spec_helper' | ||
|
|
||
| describe Bugsnag::Utility::CircularBuffer do | ||
| describe "buffer size" do |
There was a problem hiding this comment.
I tend to prefer the describe blocks with method names because it makes some aspects of organization better. For example, if someone changes a public method, the area of the corresponding spec file that needs changing tends to be obvious. It also helps when reviewing a particular public method because the reviewer will have an easier time checking that all the corresponding unit tests for the method cover and test the method sufficiently.
However, there are cases where using describe blocks with method names as the doc strings is awkward, such as some data structure classes (for example, in this class, there can be a debate on whether the "can be set during initialization" case should belong in describe '#initialize' or describe '#max_items' under the method name doc string approach). Perhaps we should seek another opinion?
spec/utility/circular_buffer_spec.rb
Outdated
|
|
||
| describe Bugsnag::Utility::CircularBuffer do | ||
| describe "buffer size" do | ||
| it "should be 25 by default" do |
There was a problem hiding this comment.
I prefer it "is 25 by default" ( http://www.betterspecs.org/#should ) as it is more concise and avoids a lot of "should" in doc strings.
Goal
Adds CircularBuffer class and tests with a view to support breadcrumbs.