rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 357 forks source link

hash_including deviates from a_hash_including #1558

Open jasonkarns opened 8 months ago

jasonkarns commented 8 months ago

hash_including does not duck-type

a_hash_including from rspec-expectations (via the underlying include matcher) supports matching expected hashes against hash-like things (so long as they implement #to_hash). However, hash_including argument matcher from rspec-mocks does not support this hash-like comparison. This is primarily a problem because, it's not immediately obvious that hash_including and a_hash_including are different matchers with different implementations. (owing to the fact that rspec supports such a wide array of aliased matchers, one could be forgiven for assuming the a_ prefix is simply an alias)

Since there have been previous discussion about unifying the matchers between rspec-mocks and rspec-expectations, it seems to me that there may be an expectation that these two matchers should behave similarly?

a_hash_including gained duck-typing in https://github.com/rspec/rspec-expectations/pull/1012

and currently casts to Hash via: https://github.com/rspec/rspec-expectations/blob/562d327cb951575d13db2a43b232c9ca1456ea6a/lib/rspec/matchers/built_in/include.rb#L78

Even if hash_including and a_hash_including are not expected to be inter-changeable, I think it would be valuable for hash_including to duck-type. The ability of rspec's matchers to nest is one of the most powerful features, IMO. Being able to leverage the nestable matchers against objects that are convertable to Hash makes tests very clean (and the diffs are great!).

The current workaround in our test suites, of course, is to properly use a_hash_including instead of hash_including. However, as mentioned before, lots of devs likely believe (as I did) these to be aliases. I've seen devs run into the no-method :key? error with hash_including and immediately bail to some rather gross assertions instead of thinking to try a_hash_including.

Your environment

Steps to reproduce

where foo and bar are custom objects that implement #to_hash

expect([foo,bar]).to contain_exactly(
hash_including(thing: "val"),
hash_including(thing: "other")
)

Expected behavior

I'd expect the hash_including matcher to send #to_hash to the actuals before fuzzy matching.

Actual behavior

       expected collection contained:  [hash_including(thing: "val"), hash_including(thing: "other")]
       actual collection contained:    [#<GraphitiSpecHelpers::Node:0x000000011b037958 @attributes={"id"=>"130", "jsonapi_type"=>"versions",... not create a versions record for document attachments" (./spec/api/v1/services/show_spec.rb:256)>>]
       the missing elements were:      [hash_including(thing: "val"), hash_including(thing: "other")]
       the extra elements were:        [#<GraphitiSpecHelpers::Node:0x000000011b037958 @attributes={"id"=>"130", "jsonapi_type"=>"versions",... not create a versions record for document attachments" (./spec/api/v1/services/show_spec.rb:256)>>]

(Another implementation of the custom classes would raise

     GraphitiSpecHelpers::Errors::NoAttribute:
       No attribute 'key?' in JSON response node!

because the GraphitiSpecHelpers::Node class implements method_missing, but not respond_to_missing properly.)

JonRowe commented 7 months ago

Sorry for the delayed response, we never did really settle on anything beyond "behaviour in mocks without expectations" is to be considered "lite" and really argument matchers are expected to leverage their expectations companions if possible, if you want to either bring mocks up to par with expectations or actualy check for the prescence of the expectations matcher and use that if possible I'd review either PR