Add initial specs for IO::Buffer #1297

trinistr · 2025-11-07T18:15:52Z

This class currently has no specs at all. This PR adds specs for the following methods:

.new,
#free, #resize, #transfer,
all predicate methods,
incidental tests for many other methods, including .map, .for, .strings and some instance methods.

Special remarks:

spec for #valid? is somewhat incomplete, as garbage-collecting a string is non-trivial (I don't know how to);
spec for #resize does 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:

.new allows any and all flags (https://bugs.ruby-lang.org/issues/21672).
Documentation is wrong (.map, for example, has very early text) (and #locked doesn't mention #transfer).
INTERNAL and EXTERNAL are unclear, not being opposites (both can be false at the same time) and not even forming an exclusive tri-state with MAPPED.
Slices feel undercooked:
- No flags are copied from the source, except READONLY.
- No way to detect a slice except to read #to_s (or call all predicate methods to check for all-false).
- Is calling #free even safe on a slice? (io_buffer_free seems to work fine only because no flags are copied, not actually checking "is this a slice?").
- Calling #free on the source buffer is not safe (https://bugs.ruby-lang.org/issues/21212).
- And #locked state 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?).
- Slices can be #resized at any point?! (Breaks the slice if size is 0, otherwise creates a new internal buffer. (Again, this is a case of "not checking for slice, and no flags are set".))
Is SHARED intended to be used manually or not?
Why is PRIVATE mapping neither INTERNAL or EXTERNAL?
.for and .string with 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)).
#resize on a MAPPED buffer changes it to INTERNAL on macOS and Windows.

trinistr · 2025-11-08T09:48:06Z

core/io/buffer/initialize_spec.rb

+    # 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


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.

core/io/buffer/initialize_spec.rb

andrykonchin · 2025-11-10T12:58:31Z

core/io/buffer/external_spec.rb

+      @buffer = IO::Buffer.new(4, IO::Buffer::MAPPED)
+      @buffer.external?.should be_false
+    end
+  end


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.

I see what you mean, but not sure what to even leave here then, especially when many cases of these flags are not intuituve. Hmm, maybe leave these specs as-is for now, but then simplify them when we have specs for all constructors?

As I mentioned above as for me it makes sense to have there a test case when a predicate returns true and a test case with false.

Yeah, it's OK to not make changes in this PR if you are going to add specs for constructors in a separate one.

andrykonchin · 2025-11-10T13:09:34Z

core/io/buffer/free_spec.rb

+    end
+    buffer.free
+    buffer.null?.should be_true
+  end


minor: seems it's duplicated in specs for #locked?.

This and other dupes: I thought it would be better to over-spec than to select which file these interactions shoud be in 😅 I guess it would make sense to remove them from #locked, as it's really a property of individual methods (like this https://github.com/ruby/ruby/blob/master/io_buffer.c#L1485)?

Yeah, excessive tests shouldn't be a problem. Yes, I agree that this specs would be better to put only here.

The main criterion as for me is to look from the user point of view - where a Ruby developer would look for these specs if he wants to checks the logic.

andrykonchin · 2025-11-10T13:15:57Z

core/io/buffer/free_spec.rb

+      slice.null?.should be_false
+      slice.valid?.should be_false
+      -> { slice.get_string }.should raise_error(IO::Buffer::InvalidatedError, "Buffer has been invalidated!")
+    end


minor: seems is duplicated in specs for #valid?

Couldn't really remove duplication, but cut off excess lines in this one and the one above.

andrykonchin · 2025-11-10T13:17:13Z

core/io/buffer/locked_spec.rb

+      # 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)


suggestion: it makes sense to specify exception messages everywhere

minor: I suppose other operations changing a buffer should be mentioned here as well (e.g. set_string or write).

To both reduce duplication and retain "what does this actually do?", I added simple "allowed" and "disallowed" tests.

andrykonchin · 2025-11-10T13:23:13Z

core/io/buffer/resize_spec.rb

+    @buffer.locked do
+      -> { @buffer.resize(10) }.should raise_error(IO::Buffer::LockedError, "Cannot resize locked buffer!")
+    end
+  end


minor: seems a duplicate

andrykonchin · 2025-11-11T20:45:54Z

Thank you!

trinistr force-pushed the add-io-buffer-spec branch 3 times, most recently from e0188af to 4ce76f1 Compare November 7, 2025 21:14

trinistr commented Nov 8, 2025

View reviewed changes

trinistr force-pushed the add-io-buffer-spec branch 5 times, most recently from c307df9 to 840258b Compare November 9, 2025 11:43

trinistr changed the title ~~Add specs for IO::Buffer~~ Add initial specs for IO::Buffer Nov 9, 2025

trinistr force-pushed the add-io-buffer-spec branch 3 times, most recently from d80c785 to 001bdd5 Compare November 9, 2025 12:15

trinistr marked this pull request as ready for review November 9, 2025 12:34

andrykonchin reviewed Nov 10, 2025

View reviewed changes

trinistr added 6 commits November 11, 2025 23:08

Add spec for IO::Buffer#initialize

bd098d7

Add specs for IO::Buffer#empty? and #null?

0500027

Add spec for IO::Buffer#locked and #locked?

f59b2a6

Add specs for other flag-checking methods of IO::Buffer

eaecca6

Add (partial) spec for IO::Buffer#valid?

848f44d

Add specs for IO::Buffer#free, #resize and #transfer

9e6959a

trinistr force-pushed the add-io-buffer-spec branch from 001bdd5 to 9e6959a Compare November 11, 2025 20:09

andrykonchin merged commit fc4cb62 into ruby:master Nov 11, 2025
12 checks passed

Uh oh!

Add initial specs for IO::Buffer #1297

Add initial specs for IO::Buffer #1297

Uh oh!

Conversation

trinistr commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trinistr Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrykonchin commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

trinistr commented Nov 7, 2025 •

edited

Loading

trinistr Nov 8, 2025 •

edited

Loading