Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Update StartWith, EndWith to use start_with?, end_with? if available #1327

Closed
wants to merge 1 commit into from
+50 −12
Closed

Update StartWith, EndWith to use start_with?, end_with? if available #1327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/rspec/matchers/built_in/start_or_end_with.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ def initialize(*expected)
# @api private
# @return [String]
def failure_message
response_msg = ", but it does not respond to #{method} and cannot be indexed using #[]"
super.tap do |msg|
if @actual_does_not_have_ordered_elements
msg << ", but it does not have ordered elements"
elsif !actual.respond_to?(:[])
msg << ", but it cannot be indexed using #[]"
msg << response_msg
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted and a seperate part added to the conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I botched something here 😅 . I meant it to be:

response_msg = ", but it does not respond to #{method} and cannot be indexed using #[]"
super.tap do |msg|
  if @actual_does_not_have_ordered_elements
    msg << ", but it does not have ordered elements"
  elsif !actual.respond_to?(method) && !actual.respond_to?(:[])
    msg << response_msg
  end

Here's what I'm thinking: Suppose actual does have ordered elements. We should only fail if actual responds to neither start_with? nor indexing. If it responds to either, we don't need to append to our failure message.

Does that sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

Its more that there is two components to this failure, it either responds to the method and failed that check, or it fell back to the original check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think I follow, sorry. It seems to me that there are a few cases where we:

  1. does implement start_with?, doesn't implement []
  2. doesn't implement start_with?, does implement []
  3. implements both
  4. implements neither

Only 4 should produce an error message here, which is why I wrote up the combined error message. It sounds like your suggestion is to have something like:

super.tap do |msg|
  if @actual_does_not_have_ordered_elements
    msg << ", but it does not have ordered elements"
  elsif !actual.respond_to?(method)
    msg << "doesn't respond to #{method}"
  elsif !actual.respond_to?(:[])
    msg << "cannot be indexed using #[]"
  end

But that doesn't seem to handle case 4 correctly? Maybe I'm missing something!

end
end
end
Expand All @@ -33,7 +34,9 @@ def description

private

def match(_expected, actual)
def match(expected, actual)
# use an object's start_with? or end_with? as appropriate
Copy link
Member

Choose a reason for hiding this comment

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

See the comment on method but this should just refer to the name of the method called to determine what to call.

return actual.send(method, expected) if actual.respond_to?(method)
return false unless actual.respond_to?(:[])

begin
Expand Down Expand Up @@ -73,6 +76,10 @@ def subset_matches?
def element_matches?
values_match?(expected, actual[0])
end

def method
Copy link
Member

Choose a reason for hiding this comment

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

Don't override method, which is an existing Ruby method, use an alternative like operator_name or comparison_name

Copy link
Member

Choose a reason for hiding this comment

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

Let's call it method_name not to override Kernel#method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

:start_with?
end
end

# @api private
Expand All @@ -88,6 +95,10 @@ def subset_matches?
def element_matches?
values_match?(expected, actual[-1])
end

def method
:end_with?
end
end
end
end
Expand Down
47 changes: 37 additions & 10 deletions spec/rspec/matchers/built_in/start_and_end_with_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,25 @@ def ==(other)
end

context "with an object that does not respond to :[]" do
it "fails with a useful message" do
actual = Object.new
expect {
expect(actual).to start_with 0
}.to fail_with("expected #{actual.inspect} to start with 0, but it cannot be indexed using #[]")
context "with an object that responds to start_with?" do
it "relies on start_with?" do
my_struct = Struct.new(:foo) do
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

has_built_in_start_with = double(:start_with? => true)

Another approach would be to actually check if start_with? was called:

has_start_with_built_in = double
expect(has_start_with_built_in).to receive(:start_with?).with(0).and_return(true)
expect(has_start_with_built_in).to start_with(0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, this is much nicer than my Struct approach, thanks for pointing it out!

def start_with?(elem)
true
end
end

expect(my_struct.new("foo")).to start_with(0)
end
end

context "with an object that does not respond to start_with?" do
it "fails with a useful message" do
actual = Object.new
expect {
expect(actual).to start_with 0
}.to fail_with("expected #{actual.inspect} to start with 0, but it does not respond to start_with? and cannot be indexed using #[]")
end
Copy link
Member

Choose a reason for hiding this comment

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

Rather than removing the old spec and nesting additional contexts, these should be seperate cases with the original left as is

Copy link
Contributor Author

@bclayman-sq bclayman-sq Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I'd love to hear more about the rationale for this! I often set up my tests to be mutually exclusive and nesting makes that easier for me. So they often have this shape:

when A
  when B
  when not B
when not A
  when B
  when not B

I wasn't totally sure about leaving this case as is because now, simply knowing that actual responds to :[] doesn't fully specify what should happen. If actual doesn't respond to start_with?, it should have one behavior. If it does, it should have a different behavior.

Could you tell me more about how you were thinking of structuring this given the above? I'm happy to follow any convention here, mostly wanted to ask for my own understanding!

end
end

Expand Down Expand Up @@ -310,11 +324,24 @@ def ==(other)
end

context "with an object that does not respond to :[]" do
it "fails with a useful message" do
actual = Object.new
expect {
expect(actual).to end_with 0
}.to fail_with("expected #{actual.inspect} to end with 0, but it cannot be indexed using #[]")
context "with an object that responds to end_with?" do
it "relies on end_with?" do
my_struct = Struct.new(:foo) do
def end_with?(elem)
true
end
end
expect(my_struct.new("foo")).to end_with(0)
end
end

context "with an object that does not respond to end_with?" do
it "fails with a useful message" do
actual = Object.new
expect {
expect(actual).to end_with 0
}.to fail_with("expected #{actual.inspect} to end with 0, but it does not respond to end_with? and cannot be indexed using #[]")
end
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

end
end

Expand Down