Subset matching broken on objects implementing to_hash and include?

[jgraichen]

Subject of the issue

1073 seems to break subset hash matching for objects implementing #to_hash and #include?. Previously these objects were converted using #to_hash on actual. Due to not converting to a Hash the check in comparing_hash_to_a_subset? will always be false and subset check won't be performed anymore (unless the objects #include? implements subset matching).

We had lots of breaking tests after the rspec-expectations patch update due to having specs on e.g. ActionDispatch::Response::Header like this:

expect(response.headers).to include 'Location' => 'http://test.host/...'

These expectations produced a nice diff when failing showing all send headers. We currently fixed it by explicitly calling #to_hash on these objects.

It would be nice if such use case could still be supported.

Your environment

• Ruby version: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux]
• rspec-expectations version: 3.8.2

Steps to reproduce

Run the following test case:

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rspec-expectations', '3.8.2'
  gem 'minitest'
end

require 'rspec/matchers'

require 'minitest/autorun'

class CollectionLikeObject
  def to_hash
    {'Header' => 'Value'}
  end

  def include?(key)
    to_hash.include?(key)
  end
end

class IncludeTest < Minitest::Test
  def test_subset_match
    matcher = RSpec::Matchers::BuiltIn::Include.new('Header' => 'Value')
    actual  = CollectionLikeObject.new

    assert matcher.matches?(actual)
  end
end

Running the test case with 3.8.1 passes.

Expected behavior

I'd expect the expectations to still pass after a patch level upgrade.

Actual behavior

All expectations failed while showing a nice diff showing no difference.

[JonRowe]

I'm not sure whats the correct behaviour here, if headers were a hash, it should fail:

 { 'Location' => 'xyz'}.include?('Location' => 'xyz')
 => false

So was headers previously behaving like an array? Or is it the individual header object not working... This feels like it's a bug that should be addressed either by Rails, or by rspec-rails providing a matcher that produces the diff, not here..

[jgraichen]

If actual is a Hash the matcher itself would do a partial match as comparing_hash_to_a_subset? is true.

Previously the headers object was converted by calling to_hash and the matcher did a partial match by itself because comparing_hash_to_a_subset? returned true. This never was implemented in Hash#include? or the headers object but only in the matcher (here).

The matcher has always and still does handle Hashs in a special way. So it's not always only actual#include? anyway.

[jgraichen]

Subset matching on hashes is well documented too.

[JonRowe]

Yes I'm aware, the issue is that response.headers is not a hash, its a hash like object. Previously we relied on an undocumented call to to_hash but documented that we were using include? this broke for objects that were legitimately responding to include? in a hash like way (which was documented as working) but didn't implement to_hash in a similar fashion. I'm open to a fix for your issue, provided it doesn't regress the fix for the correct behaviour.

[benoittgt]

@jgraichen Do you think you could have time to try to submit a patch?

[jgraichen]

I'm still thinking about a solution. I do understand to use #include? if available and how it has been documented.

Given the requirement to not change the behavior of using include? if available (and unless actual is Hash) there is no solution to our problem. Even using #to_hash only when expected is a Hash would break that behavior (because one might test that there is a specific hash included).

Apparently we kindly profited from that bug and the bug fix suddenly broke our workflow. ;-)

I do not see any easy patch not breaking the requirement of using include? if available. We will probably change our specs to explicitly calling #to_hash upfront or build another matcher. Personally I would call objects described by @JonRowe as breaking the contract of #to_hash, because #to_hash, like #to_str, means the object behaves like a Hash (in difference to #to_h). But that is a matter of perspective to duck typing (and documentation of the include matcher). Luckily Hashes are still handled special.

Thanks for your work!

[benoittgt]

Thanks @jgraichen for this detailed answer. I'm closing the issue for now because as you mention:

Apparently we kindly profited from that bug and the bug fix suddenly broke our workflow. ;-)

I will let @JonRowe comment if wanted.

[JonRowe]

I'm still interested in doing something with this to better support hash shaped objects

[dvandersluis]

I'm pretty sure this change broke my test which uses ActionController::Parameters

expect(policy.permitted(params)).to include('foo' => 'bar')

I fixed it for now by adding a .to_h in the expect, but this worked before updating from 3.8.1 to 3.8.3.