Skip to content

Provide a clear failure message when using `{}.should =~ {}`. #193

Merged
merged 1 commit into from Dec 6, 2012

2 participants

@myronmarston
RSpec member

The match_array matcher (delegated to by =~ for enumerables) is
not meant to be used for hashes, but the should =~ syntax doesn't
make that obvious. Previously, you would get a failure like:

 Failure/Error: actual.should =~ actual
   expected: {:foo=>"bar"}
        got: {:foo=>"bar"} (using =~)

...which is pretty confusing. Our match_array matcher already
includes handling for invalid arguments (such as hashes) to return
a clear failure message, but it wasn't being used for an expression
like {}.should =~ {} because it was only registered as an operator
matcher for Array. This changes it so that it is registered as an
operator matcher for any Enumerable. This improves the failure message
for enumerable types like hash and set, but on its own, it could cause
breakage for things like 1.8 strings that are Enumerable but also
define a reasonable =~. The fix here changes the operator matcher
delegation logic so that it only delegates to the registered matcher
if the object has the generic Kernel implementation of the operator.
If it has a more specific implementation, we assume the user actually
wants to match using the given operator itself.

Fixes #191.

Can I get a code review? /cc @alindeman @phiggins

@myronmarston myronmarston Provide a clear failure message when using `{}.should =~ {}`.
The match_array matcher (delegated to by `=~` for enumerables) is
not meant to be used for hashes, but the `should =~` syntax doesn't
make that obvious. Previously, you would get a failure like:

     Failure/Error: actual.should =~ actual
       expected: {:foo=>"bar"}
            got: {:foo=>"bar"} (using =~)

...which is pretty confusing. Our `match_array` matcher already
includes handling for invalid arguments (such as hashes) to return
a clear failure message, but it wasn't being used for an expression
like `{}.should =~ {}` because it was only registered as an operator
matcher for `Array`. This changes it so that it is registered as an
operator matcher for any Enumerable. This improves the failure message
for enumerable types like hash and set, but on its own, it could cause
breakage for things like 1.8 strings that are Enumerable but also
define a reasonable `=~`.  The fix here changes the operator matcher
delegation logic so that it only delegates to the registered matcher
if the object has the generic Kernel implementation of the operator.
If it has a more specific implementation, we assume the user actually
wants to match using the given operator itself.

Fixes #191.
c7a91af
@alindeman alindeman commented on the diff Dec 6, 2012
lib/rspec/matchers/operator_matcher.rb
@@ -62,6 +62,22 @@ def description
private
+ if Method.method_defined?(:owner) # 1.8.6 lacks Method#owner :-(
@alindeman
alindeman added a note Dec 6, 2012

Other places we have checked RUBY_VERSION >= '1.8.7' to make this code easier to find and delete ifever/whenever we drop support for 1.8.6. Thoughts?

@myronmarston
RSpec member
myronmarston added a note Dec 6, 2012

If I was going to check the RUBY_VERSION, I'd want to check if it equaled 1.8.6 -- the point in my mind is to make it easy to ack/grep for 1.8.6. and find lots of places where we can remove code.

I actually considered checking the version in that manner, but decided to use feature detection plus a comment that mentions 1.8.6...that still gives us the search-ability, while still performing the check in a more semantic manner. Also, if anyone on 1.8.6 loads backports, it defines Method#owner and they could potentially use this first implementation of uses_generic_implementation_of?.

@alindeman
alindeman added a note Dec 6, 2012

That's good rationale. Makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alindeman alindeman commented on the diff Dec 6, 2012
lib/rspec/matchers/operator_matcher.rb
@@ -62,6 +62,22 @@ def description
private
+ if Method.method_defined?(:owner) # 1.8.6 lacks Method#owner :-(
+ def uses_generic_implementation_of?(op)
+ @actual.method(op).owner == ::Kernel
+ end
+ else
+ def uses_generic_implementation_of?(op)
+ # This is a bit of a hack, but:
+ #
+ # {}.method(:=~).to_s # => "#<Method: Hash(Kernel)#=~>"
+ #
+ # In the absence of Method#owner, this is the best we
+ # can do to see if the method comes from Kernel.
+ @actual.method(op).to_s.include?('(Kernel)')
@alindeman
alindeman added a note Dec 6, 2012

Nice :) Gotta do what you gotta do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@alindeman alindeman commented on the diff Dec 6, 2012
spec/rspec/matchers/match_array_spec.rb
@@ -135,4 +146,8 @@ def ==(other)
it "fails with a string and the expected error message is given" do
expect { "I like turtles".should match_array([1,2,3]) }.to fail_with(/expected an array/)
end
+
+ it 'fails with a clear message when given a hash using the `should =~` syntax' do
+ expect { {}.should =~ {} }.to fail_with(/expected an array/)
+ end
@alindeman
alindeman added a note Dec 6, 2012

Even though I know it will pass, does it make sense to test a String instance on 1.8.7 .. since that is a bit of a weird case? I'm thinking regression prevention if the code is ever refactored or reworked a bit. I'm not sure it's a good idea, but I did have the thought.

@myronmarston
RSpec member
myronmarston added a note Dec 6, 2012

I wanted to make sure my intuition that this was necessary for String on 1.8.7 was correct...so I developed this on 1.8.7, and changed the operator registration from Array to Enumerable as the first step. It broke a bunch of tests, and demonstrated this was necessary. For example, this spec failed:

https://github.com/rspec/rspec-expectations/blob/v2.12.0/spec/rspec/matchers/operator_matcher_spec.rb#L107-L111

@alindeman
alindeman added a note Dec 6, 2012

Also I see that the string case was already tested. I had skimmed over the non-added part. This looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston merged commit cf52fe6 into master Dec 6, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.