Skip to content

Improve the match_array failure message when a non-array object is passed #186

Closed
wants to merge 2 commits into from

3 participants

@samphippen
RSpec member

Related to #185

I've fixed this up, by checking if the passed object returns true from is_a? Array otherwise an error message is given stating that the object isn't an array.

Also for some reason I've got a failing spec on my local computer (both master and this branch):

/Users/sam/.rvm/rubies/ruby-1.9.3-p194/bin/ruby -w -S rspec ./spec/rspec/expectations/differ_spec.rb ./spec/rspec/expectations/expectation_target_spec.rb ./spec/rspec/expectations/extensions/kernel_spec.rb ./spec/rspec/expectations/fail_with_spec.rb ./spec/rspec/expectations/handler_spec.rb ./spec/rspec/expectations/syntax_spec.rb ./spec/rspec/matchers/base_matcher_spec.rb ./spec/rspec/matchers/be_close_spec.rb ./spec/rspec/matchers/be_instance_of_spec.rb ./spec/rspec/matchers/be_kind_of_spec.rb ./spec/rspec/matchers/be_spec.rb ./spec/rspec/matchers/be_within_spec.rb ./spec/rspec/matchers/change_spec.rb ./spec/rspec/matchers/configuration_spec.rb ./spec/rspec/matchers/cover_spec.rb ./spec/rspec/matchers/description_generation_spec.rb ./spec/rspec/matchers/dsl_spec.rb ./spec/rspec/matchers/eq_spec.rb ./spec/rspec/matchers/eql_spec.rb ./spec/rspec/matchers/equal_spec.rb ./spec/rspec/matchers/exist_spec.rb ./spec/rspec/matchers/has_spec.rb ./spec/rspec/matchers/have_spec.rb ./spec/rspec/matchers/include_spec.rb ./spec/rspec/matchers/match_array_spec.rb ./spec/rspec/matchers/match_spec.rb ./spec/rspec/matchers/matcher_spec.rb ./spec/rspec/matchers/matchers_spec.rb ./spec/rspec/matchers/method_missing_spec.rb ./spec/rspec/matchers/operator_matcher_spec.rb ./spec/rspec/matchers/raise_error_spec.rb ./spec/rspec/matchers/respond_to_spec.rb ./spec/rspec/matchers/satisfy_spec.rb ./spec/rspec/matchers/start_with_end_with_spec.rb ./spec/rspec/matchers/throw_symbol_spec.rb ./spec/rspec/matchers/yield_spec.rb
Run options: include {:focused=>true}

All examples were filtered out; ignoring {:focused=>true}
.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F........................................................................................................................................................................................................................................................................................................................................

Failures:

  1) RSpec::Matchers::Configuration#backtrace_formatter defaults to a null formatter when rspec-core is not loaded
     Failure/Error: hide_const("RSpec::Core::BacktraceFormatter")
     NoMethodError:
       undefined method `hide_const' for #<RSpec::Core::ExampleGroup::Nested_66::Nested_1:0x007f8b925616c8>
     # ./lib/rspec/matchers/method_missing.rb:9:in `method_missing'
     # ./spec/rspec/matchers/configuration_spec.rb:34:in `block (3 levels) in <module:Matchers>'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example.rb:114:in `instance_eval'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example.rb:114:in `block in run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example.rb:254:in `with_around_each_hooks'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example.rb:111:in `run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:388:in `block in run_examples'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:384:in `map'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:384:in `run_examples'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:369:in `run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:370:in `block in run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:370:in `map'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/example_group.rb:370:in `run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/reporter.rb:34:in `report'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/runner.rb:69:in `run'
     # /Users/sam/dev/rspec/rspec-core/lib/rspec/core/runner.rb:8:in `block in autorun'

Finished in 0.28982 seconds
810 examples, 1 failure

Failed examples:

rspec ./spec/rspec/matchers/configuration_spec.rb:33 # RSpec::Matchers::Configuration#backtrace_formatter defaults to a null formatter when rspec-core is not loaded

Randomized with seed 26263
@samphippen samphippen Make the error message more useful when using match_array
Now the error message will let you know if you're trying to match an
array against something that isn't an array.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
90df181
@myronmarston myronmarston and 2 others commented on an outdated diff Oct 26, 2012
lib/rspec/matchers/built_in/match_array.rb
@@ -3,16 +3,23 @@ module Matchers
module BuiltIn
class MatchArray < BaseMatcher
def match(expected, actual)
+ return false unless actual.is_a? Array
@myronmarston
RSpec member
myronmarston added a note Oct 26, 2012

I actually don't think we want to limit this to arrays only. Anything that duck-types to the parts of the array interface we need here should be sufficient.

@samphippen
RSpec member
samphippen added a note Oct 26, 2012
@samphippen
RSpec member
samphippen added a note Oct 26, 2012

I'm not sure if I prefer this over catching method missing. Thoughts?

@alindeman
alindeman added a note Oct 26, 2012

My gut reaction is that we should check #to_ary and use Array.new(actual) instead of actual.dup later. Array.new will take in a thing that responds to #to_ary.

What do you guys thing?

@myronmarston
RSpec member
myronmarston added a note Oct 26, 2012

Yeah, I think catching a NoMethodError is fine. That way, if we don't have to maintain a list of methods this depends on. I think it'd be nice for the failure message to say something like expected #{object} to be array-like, but it did not respond to #{no_method_error.name}.

@samphippen
RSpec member
samphippen added a note Oct 26, 2012

I like this.

@myronmarston
RSpec member
myronmarston added a note Oct 26, 2012

My gut reaction is that we should check #to_ary and use Array.new(actual) instead of actual.dup later. Array.new will take in a thing that responds to #to_ary.

That sounds fine, although we should think through the cases where the provided object is not actually an array. For example, if it is a Set would it work properly?

@alindeman
alindeman added a note Oct 26, 2012

Set doesn't appear to respond to to_ary, but it also doesn't respond to other methods we currently need for this matcher like #index:

 Failure/Error: expect(Set.new([1, 2, 3])).to match_array([1, 2, 3])
     NoMethodError:
       undefined method `index' for #<Set: {1, 2, 3}>

We could consider accepting objects that can be converted to arrays via #to_a? A Set would work in that case.

@samphippen
RSpec member
samphippen added a note Oct 26, 2012

I just tried Set also. I think this to_ary thing probably gives us the best behaviour. See also the behaviour with to_str we discussed at length over the message must be string pull request. Presumably to_ary is also our protocol for if a thing quacks like an array?

@myronmarston
RSpec member
myronmarston added a note Oct 26, 2012

That makes some sense...or we could use Array() to convert array. Although, Array is pretty lenient...Array(any_object) returns [any_object]...is that what we want?

@samphippen
RSpec member
samphippen added a note Oct 26, 2012

I don't think it is. For example if we have a spec like

my_method.should match_array([17,3,2])

and my_method returns nil then Array(nil) will say [nil] doesn't match [17,3,2], which may confuse the user and make them think the method returned [nil] instead of nil. Imo it should say that the result wasn't an array like object.

@alindeman
alindeman added a note Oct 26, 2012

Although, Array is pretty lenient...Array(any_object) returns [any_object]...is that what we want?

I think that's too lenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samphippen samphippen Check if an actual responds to to_array for match_array
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
84e8d52
@alindeman

I'm going to bias towards merging this, making use of #to_ary, as that should keep the implementation roughly as strict as it is today.

However, I'm definitely open to discussing making it more lenient at some point (e.g., accepting things that respond to #to_a or certain things that Kernel#Array can deal with).

@alindeman alindeman added a commit that closed this pull request Oct 27, 2012
@samphippen samphippen Make the error message more useful when using match_array
* Now the error message will let you know if you're trying to match an
  array against something that isn't an array-like thing.
* Closes #186
* Fixes #185
e2a9e3f
@alindeman alindeman closed this in e2a9e3f Oct 27, 2012
@alindeman

Tweaked, squashed and merged. Thanks as usual @samphippen :rainbow:

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.