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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bclayman-sq
Copy link
Contributor

This addresses issue #1025.

With this change, the StartWith matcher will rely on an object's start_with? method if available. Similarly, the EndWith matcher will rely on an object's end_with? method if available.

This is especially useful when a class implements start_with? but not the indexing operator, or end_with? but not the indexing operator.

This addresses issue rspec#1025.

With this change, the StartWith matcher will rely on an object's
start_with? method if available.  Similarly, the EndWith matcher
will rely on an object's end_with? method if available.

This is especially useful when a class implements start_with?
but not the indexing operator, or end_with? but not the indexing
operator.
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks great!
Wondering why it wasn't done like that from the very beginning.

Wondering if we need to check if there are multiple args:

"abc".start_with?('a', 'c') # => true

https://ruby-doc.org/core-2.7.1/String.html#method-i-start_with-3F seems to allow all prefixes, while the matcher's semantic is different.

@@ -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.

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!

}.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!

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Nice work, but this can't be merged into main as its a breaking change in behaviour, so would have to be for rspec 4.

@@ -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

@@ -34,6 +35,8 @@ def description
private

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.

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!

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

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!

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

@bclayman-sq
Copy link
Contributor Author

bclayman-sq commented Oct 14, 2021

Looks great! Wondering why it wasn't done like that from the very beginning.

Wondering if we need to check if there are multiple args:

"abc".start_with?('a', 'c') # => true

https://ruby-doc.org/core-2.7.1/String.html#method-i-start_with-3F seems to allow all prefixes, while the matcher's semantic is different.

Yeah, that's a great point. If we do this:

expect("abc").to start_with("a", "c")

Then expected is ["a", "c"] and my implementation doesn't handle this correctly: "abc".start_with?(["a", "c"]) fails with an error.

I think it's OK to assume that any class that implements start_with? allows you to pass multiple args (like in your string example). In light of that, I think I can splat expected like this:

return actual.send(method_name, *expected) if actual.respond_to?(method_name)

As you and @JonRowe pointed out, this is a breaking change, so totally get that this would have to be in the 4.0 release 👍

@pirj
Copy link
Member

pirj commented Oct 15, 2021

I'm not convinced to make a breaking change.
There is be_start_with dynamic matcher that works similarly to start_with?. And we can keep the known behaviour for start_with intact.

To still be able to benefit from this change, I'd suggest to only delegate to start_with? if the matcher gets a single argument. For multiple - fallback to the previous behaviour.

On a side note - adding deprecation messages is not much fun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants