Skip to content
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

add spec for Range#=== to behave like Range#cover? in Ruby 2.7 #797

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

lxxxvi
Copy link
Contributor

@lxxxvi lxxxvi commented Oct 10, 2020

Hello again 👋

From #745 :

  •  Range#=== now uses Range#cover? for String arguments, too (in Ruby 2.6, it was
    changed from Range#include? for all types except strings). Bug #15449

To cover that (pun intended), I basically just added this to Range#=== specs

it_behaves_like :range_cover_and_include, :cover?
it_behaves_like :range_cover, :cover?

Do you think this is sufficient? 🙈

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Sounds good, sorry for the late review.

core/range/case_compare_spec.rb Outdated Show resolved Hide resolved
@lxxxvi lxxxvi force-pushed the add-spec-for-Range-=== branch from 6feb48b to 20516e6 Compare October 16, 2020 13:46
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 16, 2020

@eregon Thanks for your review!

Interestingly some tests started to fail now (see bottom). Apparently :=== doesn't work the same as cover? when comparing with number ranges. Example

# Ruby v2.7.2

(2..5).cover?((3..4)) # => true
(2..5) === (3..4)     # => false

I am not sure if this is expected, hence if I understand the Bug #15449 description correctly.

Should :=== work exactly like .cover? or just particularly for strings?

What do you think?

Failing tests

Range#=== range argument accepts range argument FAILED
Expected false to be true
/.../core/range/shared/cover.rb:97:in `block (4 levels) in <top (required)>'
/.../core/range/case_compare_spec.rb:5:in `<top (required)>'
(0..10).send(@method, (3..7)).should be_true

Range#=== range argument supports boundaries of different comparable types FAILED
Expected false to be true
/.../core/range/shared/cover.rb:120:in `block (4 levels) in <top (required)>'
/.../core/range/case_compare_spec.rb:5:in `<top (required)>'
(0..10).send(@method, (3.1..7.9)).should be_true

Range#=== range argument honors exclusion of right boundary (:exclude_end option) FAILED
Expected false to be true
/.../core/range/shared/cover.rb:132:in `block (4 levels) in <top (required)>'
/.../core/range/case_compare_spec.rb:5:in `<top (required)>'
(0..10).send(@method, (0..10)).should be_true

@eregon
Copy link
Member

eregon commented Oct 16, 2020

It's expected (0..10).send(@method, (3..7)) is only true for cover? itself.
=== only has the "does the Range include that element?", not the "is this Range a superset of that Range?" semantics.

So let's move

ruby_version_is "2.6" do
context "range argument" do
it "accepts range argument" do
(0..10).send(@method, (3..7)).should be_true
(0..10).send(@method, (3..15)).should be_false
(0..10).send(@method, (-2..7)).should be_false
(1.1..7.9).send(@method, (2.5..6.5)).should be_true
(1.1..7.9).send(@method, (2.5..8.5)).should be_false
(1.1..7.9).send(@method, (0.5..6.5)).should be_false
('c'..'i').send(@method, ('d'..'f')).should be_true
('c'..'i').send(@method, ('d'..'z')).should be_false
('c'..'i').send(@method, ('a'..'f')).should be_false
range_10_100 = RangeSpecs::TenfoldSucc.new(10)..RangeSpecs::TenfoldSucc.new(100)
range_20_90 = RangeSpecs::TenfoldSucc.new(20)..RangeSpecs::TenfoldSucc.new(90)
range_20_110 = RangeSpecs::TenfoldSucc.new(20)..RangeSpecs::TenfoldSucc.new(110)
range_0_90 = RangeSpecs::TenfoldSucc.new(0)..RangeSpecs::TenfoldSucc.new(90)
range_10_100.send(@method, range_20_90).should be_true
range_10_100.send(@method, range_20_110).should be_false
range_10_100.send(@method, range_0_90).should be_false
end
it "supports boundaries of different comparable types" do
(0..10).send(@method, (3.1..7.9)).should be_true
(0..10).send(@method, (3.1..15.9)).should be_false
(0..10).send(@method, (-2.1..7.9)).should be_false
end
it "returns false if types are not comparable" do
(0..10).send(@method, ('a'..'z')).should be_false
(0..10).send(@method, (RangeSpecs::TenfoldSucc.new(0)..RangeSpecs::TenfoldSucc.new(100))).should be_false
end
it "honors exclusion of right boundary (:exclude_end option)" do
# Integer
(0..10).send(@method, (0..10)).should be_true
(0...10).send(@method, (0...10)).should be_true
(0..10).send(@method, (0...10)).should be_true
(0...10).send(@method, (0..10)).should be_false
(0...11).send(@method, (0..10)).should be_true
(0..10).send(@method, (0...11)).should be_true
# Float
(0..10.1).send(@method, (0..10.1)).should be_true
(0...10.1).send(@method, (0...10.1)).should be_true
(0..10.1).send(@method, (0...10.1)).should be_true
(0...10.1).send(@method, (0..10.1)).should be_false
(0...11.1).send(@method, (0..10.1)).should be_true
(0..10.1).send(@method, (0...11.1)).should be_false
end
end
end
into its own describe :range_cover_subrange, shared: true do, that way we only test that for Range#cover? itself.

PR ruby#797 introduces specs for `:===` to behave like `:cover?`, except for subranges.
In order to reuse the `describe :range_cover, shared: true` group for `:===`, it's been suggested to move specs related to subranges to its own group.
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 19, 2020

@eregon Thank you for your explanation. I added a new commit according your suggestion.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you, looks good!

@eregon eregon merged commit 947c8bb into ruby:master Oct 20, 2020
@lxxxvi lxxxvi deleted the add-spec-for-Range-=== branch January 12, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants