Fix Include Matcher For Ranges #1324

bclayman-sq · 2021-10-04T20:36:49Z

This PR addresses issue #1191 for ruby >= 2.1.9.

Previously, a few parts of the Include matcher assumed all Ranges were iterable. This caused it to raise errors like TypeError: Can't iterate from [Float|Time]

This happens because Ranges require that their beginning element implement succ. Float doesn't, which causes the error. Time is different because it does implement succ but

a) Range#succ is deprecated as of ruby 1.9.2 and
b) Some Ruby implementations raise an exception when trying to iterate through a range of Time objects

This PR does a few things:

Fixes the Include matcher to handle Ranges that don't support iteration, while continuing to support Ranges that do
Adds specs for both types of Ranges in 1). There weren't any for the Include matcher used with Ranges.

lib/rspec/matchers/built_in/include.rb

pirj · 2021-10-06T10:03:51Z

Does it make sense to rebase this on 4-0-dev and get rid of Ruby < 2.3 checks? @JonRowe do we have a timeline for 4.0 release?

bclayman-sq · 2021-10-09T12:58:00Z

Does it make sense to rebase this on 4-0-dev and get rid of Ruby < 2.3 checks? @JonRowe do we have a timeline for 4.0 release?

👋 Hi @JonRowe, I wanted to follow up on this and to get your review!

bclayman-sq · 2021-10-11T03:14:11Z

@pirj Thanks for your help on that other PR!

I'm not sure what a normal review timeline is for RSpec; is there something you could do to help get this one approved + merged?

I'd love to get this merged in before the 4.0 release if possible. Thanks again for your help!

pirj · 2021-10-11T08:33:37Z

It may take a while. We're making our best effort, but often times life and daily job intervenes, and it may take longer. Processing has improved this year, though:

bclayman-sq · 2021-10-11T14:20:52Z

It may take a while. We're making our best effort, but often times life and daily job intervenes, and it may take longer. Processing has improved this year, though:

Ah that's really helpful; I mostly wanted to be sure it didn't get lost in the shuffle but sounds like these things, understandably, take time. Thanks for the info!

bclayman-sq · 2022-03-28T19:19:22Z

@JonRowe @pirj 👋 Anything I can do to get this PR merged?

pirj

Thank you!

What do you think of re-targeting it for RSpec 4 where we've dropped support for Ruby < 2.2? It would require rebasing on top of 4-0-dev and chaning the base branch of the PR.

JonRowe · 2022-04-22T16:25:31Z

spec/rspec/matchers/built_in/include_spec.rb

+
+ shared_context "only runs for modern rubies" do
+ around(:example) do |example|
+ example.run if RUBY_VERSION >= "2.1.9"


Can we change this for an expected error?

@JonRowe Definitely!

I just pushed a PR that runs examples for all ruby versions so I can see the error across older rubies (the retention window for this PR's build logs has elapsed).

Mind kicking off another build, I'll see which errors crop up and figure out where to go from there?

Done, I wish Github remembered approvals, I assume it goes off "merged commiters"

@JonRowe Awesome, I just pushed those changes (it looks like old rubies raise a TypeError so that's what I now expect to get raised for those versions).

Mind kicking off one more build?

Also, I'm not 100% sure that my naming for the shared examples is best. Open to suggestion there!

JonRowe · 2022-04-22T16:25:43Z

spec/rspec/matchers/built_in/include_spec.rb

+ let(:range) { (start..finish) }
+ end
+
+ shared_context "only runs for modern rubies" do


If we do the below this should be renamed

JonRowe

I'm happy to merge this once we've addressed what happens on older Rubies, I don't mind it "not working", I'd just like the specs to show what happens on older Rubies, even if thats just this (or equivalent):

if RUBY_VERSION >= "2.1.9"
  example.run 
else
  expect { example.run }.to raise_error(WhateverErrorIsRaised)
end

bclayman-sq · 2022-04-25T23:04:26Z

I'm happy to merge this once we've addressed what happens on older Rubies, I don't mind it "not working", I'd just like the specs to show what happens on older Rubies, even if thats just this (or equivalent):
if RUBY_VERSION >= "2.1.9"
  example.run 
else
  expect { example.run }.to raise_error(WhateverErrorIsRaised)
end

Hi @JonRowe,

I'd like to suggest going with the approach that only runs these new tests for ruby >= 2.1.9. This covers all ruby versions in the last 9 years and fixes the bug. How would you feel about that?

Here's a bit of technical discussion about why I don't think the proposed approach quite works:

I don't think there's a straightforward way to expect examples to raise a TypeError and only a TypeError. This code:

if RUBY_VERSION >= "2.1.9"
  example.run 
else
  expect { example.run }.to raise_error(WhateverErrorIsRaised)
end

actually causes 2 failures for specs run on pre-2.1.9 ruby:

Got 1 failure and 1 other error:

20.1) Failure/Error: expect(range).not_to include(2.0).twice

TypeError:
  can't iterate from Float
# ./spec/rspec/matchers/built_in/include_spec.rb:797:in `block (7 levels) in <top (required)>'
# ./spec/rspec/matchers/built_in/include_spec.rb:127:in `block (4 levels) in <top (required)>'
# ./spec/rspec/matchers/built_in/include_spec.rb:127:in `block (3 levels) in <top (required)>'

20.2) Failure/Error: expect { example.run }.to raise_error(TypeError)
  expected TypeError but nothing was raised
# ./spec/rspec/matchers/built_in/include_spec.rb:127:in `block (3 levels) in <top (required)>'

This is because the original expectation is evaluated first and isn't met (it raises a TypeError). Then the second expectation also isn't met because it expects a TypeError to be raised and this has already happened.

Do you know a way around this? If not, I'd love to merge as is since it still represents a solid improvement. Let me know what you think!

This is issue rspec#1191. Previously, a few parts of the Include matcher assumed all Ranges were iterable. This caused it to raise errors like: TypeError: Can't iterate from [Float|Time] This happens because Ranges require that their beginning element implement succ. Float doesn't which causes the error. Time is different because it does implement succ but a) it's deprecated as of Ruby 1.9.2 and b) some Ruby implementations raise an exception when trying to iterate through a range of Time objects. This PR does a few things: 1) Fixes the Include matcher to handle Ranges that don't support iteration, while continuing to support Ranges that do 2) Adds specs for both types of Ranges in 1). There weren't any for the Include matcher used with Ranges.

spec/rspec/matchers/built_in/include_spec.rb

bclayman-sq force-pushed the bclayman/issue-1191-fix-include-matcher-for-ranges branch 2 times, most recently from 06b29d1 to df827ac Compare October 5, 2021 19:44

JonRowe reviewed Oct 5, 2021

View reviewed changes

lib/rspec/matchers/built_in/include.rb Outdated Show resolved Hide resolved

bclayman-sq force-pushed the bclayman/issue-1191-fix-include-matcher-for-ranges branch from df827ac to 47d409f Compare October 5, 2021 19:58

pirj requested a review from JonRowe October 6, 2021 10:15

pirj approved these changes Mar 29, 2022

View reviewed changes

JonRowe reviewed Apr 22, 2022

View reviewed changes

JonRowe requested changes Apr 22, 2022

View reviewed changes

bclayman-sq force-pushed the bclayman/issue-1191-fix-include-matcher-for-ranges branch from fb1a144 to b070eb9 Compare April 22, 2022 20:32

bclayman-sq added 2 commits December 28, 2023 22:56

Update tests to expect to raise a TypeError on old ruby versions

e61ca2b

pirj force-pushed the bclayman/issue-1191-fix-include-matcher-for-ranges branch from b070eb9 to e61ca2b Compare December 28, 2023 19:58

pirj reviewed Dec 28, 2023

View reviewed changes

Expect TypeError failure on old rubies

211e733

pirj force-pushed the bclayman/issue-1191-fix-include-matcher-for-ranges branch from 5dc0c99 to 211e733 Compare December 28, 2023 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Include Matcher For Ranges #1324

Fix Include Matcher For Ranges #1324

bclayman-sq commented Oct 4, 2021 •

edited

pirj commented Oct 6, 2021

bclayman-sq commented Oct 9, 2021

bclayman-sq commented Oct 11, 2021 •

edited

pirj commented Oct 11, 2021

bclayman-sq commented Oct 11, 2021

bclayman-sq commented Mar 28, 2022

pirj left a comment

JonRowe Apr 22, 2022

bclayman-sq Apr 22, 2022

JonRowe Apr 22, 2022

bclayman-sq Apr 22, 2022

JonRowe Apr 22, 2022

JonRowe left a comment

bclayman-sq commented Apr 25, 2022

Fix Include Matcher For Ranges #1324

Are you sure you want to change the base?

Fix Include Matcher For Ranges #1324

Conversation

bclayman-sq commented Oct 4, 2021 • edited

pirj commented Oct 6, 2021

bclayman-sq commented Oct 9, 2021

bclayman-sq commented Oct 11, 2021 • edited

pirj commented Oct 11, 2021

bclayman-sq commented Oct 11, 2021

bclayman-sq commented Mar 28, 2022

pirj left a comment

Choose a reason for hiding this comment

JonRowe Apr 22, 2022

Choose a reason for hiding this comment

bclayman-sq Apr 22, 2022

Choose a reason for hiding this comment

JonRowe Apr 22, 2022

Choose a reason for hiding this comment

bclayman-sq Apr 22, 2022

Choose a reason for hiding this comment

JonRowe Apr 22, 2022

Choose a reason for hiding this comment

JonRowe left a comment

Choose a reason for hiding this comment

bclayman-sq commented Apr 25, 2022

bclayman-sq commented Oct 4, 2021 •

edited

bclayman-sq commented Oct 11, 2021 •

edited