Add initial specs for IO::Buffer #1297
Conversation
trinistr force-pushed the add-io-buffer-spec branch 3 times, most recently from e0188af to 4ce76f1 Compare 
core/io/buffer/initialize_spec.rb Outdated
| # This looks like a bug to me, these should be different memory models? | ||
| # Definitely looks like a bug, created issue: https://bugs.ruby-lang.org/issues/21672 | ||
| it "creates internal-mapped buffer if INTERNAL and MAPPED are both specified" do | ||
| @buffer = IO::Buffer.new(10, IO::Buffer::INTERNAL | IO::Buffer::MAPPED) | ||
| @buffer.should.internal? | ||
| @buffer.should.mapped? | ||
| @buffer.should_not.empty? | ||
| end |
There was a problem hiding this comment.
This is very clearly a bug, so I filed it as such. Not sure if the test needs to be here. Probably not? This does not seem like behavior that should be replicated. I will delete it for now.
trinistr force-pushed the add-io-buffer-spec branch from 4ce76f1 to 0e6e4e6 Compare trinistr force-pushed the add-io-buffer-spec branch from 0e6e4e6 to 8d1ae07 Compare trinistr force-pushed the add-io-buffer-spec branch 3 times, most recently from c307df9 to 840258b Compare trinistr force-pushed the add-io-buffer-spec branch from 840258b to e6fcb41 Compare trinistr force-pushed the add-io-buffer-spec branch from e6fcb41 to d80c785 Compare trinistr force-pushed the add-io-buffer-spec branch from d80c785 to 001bdd5 Compare 
| @buffer.size.should == size | ||
| @buffer.should.mapped? | ||
| @buffer.should_not.empty? | ||
| end |
There was a problem hiding this comment.
suggestion: if given size affects both "internal" and "mapped" state - it makes sense to check both of them in each of the two tests above.
| it "raises IO::Buffer::AllocationError with other values for flags" do | ||
| -> { IO::Buffer.new(10, IO::Buffer::READONLY) }.should raise_error(IO::Buffer::AllocationError, "Could not allocate buffer!") | ||
| -> { IO::Buffer.new(10, 0) }.should raise_error(IO::Buffer::AllocationError, "Could not allocate buffer!") | ||
| end |
There was a problem hiding this comment.
minor: This test case doesn't look accurate. It seems the exception is raised only if neither IO::Buffer::MAPPED nor IO::Buffer::MAPPED flag is given.
https://github.com/ruby/ruby/blob/7a5688e2a27668e48f8d6ff4af5b2208b98a2f5e/io_buffer.c#L202-L213
All the other known flags are applied
https://github.com/ruby/ruby/blob/7a5688e2a27668e48f8d6ff4af5b2208b98a2f5e/io_buffer.c#L47.
And it seems unknown flags are just ignored.
| @buffer = IO::Buffer.new(4, IO::Buffer::MAPPED) | ||
| @buffer.external?.should be_false | ||
| end | ||
| end |
There was a problem hiding this comment.
suggestion: It seems to me that this logic is closer to the constructor methods specs (new/map/for) than to the predicates. I would keep here just a couple of examples when a predicate returns either true or false.
| end | ||
| buffer.free | ||
| buffer.null?.should be_true | ||
| end |
There was a problem hiding this comment.
minor: seems it's duplicated in specs for #locked?.
| slice.null?.should be_false | ||
| slice.valid?.should be_false | ||
| -> { slice.get_string }.should raise_error(IO::Buffer::InvalidatedError, "Buffer has been invalidated!") | ||
| end |
There was a problem hiding this comment.
minor: seems is duplicated in specs for #valid?
| # See specs of individual methods for error messages. | ||
| -> { @buffer.free }.should raise_error(IO::Buffer::LockedError) | ||
| -> { @buffer.resize(8) }.should raise_error(IO::Buffer::LockedError) | ||
| -> { @buffer.transfer }.should raise_error(IO::Buffer::LockedError) |
There was a problem hiding this comment.
suggestion: it makes sense to specify exception messages everywhere
There was a problem hiding this comment.
minor: I suppose other operations changing a buffer should be mentioned here as well (e.g. set_string or write).
| @buffer.locked do | ||
| -> { @buffer.resize(10) }.should raise_error(IO::Buffer::LockedError, "Cannot resize locked buffer!") | ||
| end | ||
| end |
2 participants
This class currently has no specs at all. This PR adds specs for the following methods:
.new,#free,#resize,#transfer,.map,.for,.stringsand some instance methods.Special remarks:
#valid?is somewhat incomplete, as garbage-collecting a string is non-trivial (I don't know how to);#resizedoes not have tests for slices as currently it seems to be an undefined behavior, and it's not clear if it should be reproduced (yes, ruby/spec is all about reproducing even weird behavior, but bugs are a gray area still).Weird things discovered so far so I don't forget:
.newallows any and all flags (https://bugs.ruby-lang.org/issues/21672)..map, for example, has very early text).INTERNALandEXTERNALare unclear, not being opposites (both can be false at the same time) and not even forming an exclusive tri-state withMAPPED.READONLY.#to_s(or call all predicate methods to check for all-false).#freeeven safe on a slice? (io_buffer_free seems to work fine only because no flags are copied, not actually checking "is this a slice?").#freeon the source buffer is not safe (https://bugs.ruby-lang.org/issues/21212).#lockeding does not propagate in either direction.#freeing a String-backed buffer does not invalidate its slice (shouldn't slices always become invalid without a source buffer?).SHAREDintended to be used manually or not?PRIVATEmapping neither INTERNAL or EXTERNAL?.forand.stringwith a block are supposed to nullify buffer at the end, but buffer can be#transferred outside, is this intentional? (this creates a run-away buffer, able to change the string from wherever (but not a frozen string, at least)).#resizeon a MAPPED buffer changes it to INTERNAL on macOS and Windows.