Composed change matcher does not always detect internal mutations

[mkrfowler]

Subject of the issue

When invoking the change matcher composed with another matcher (here have_attributes), actions that change neither subject equality nor #hash do not result in detection of a change at all. This is not necessarily true of the matcher with which it is composed.

Your environment

• Ruby version: 2.6.4
• rspec-expectations version:

Steps to reproduce

# frozen_string_literal: true

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", "3.7.0" # Activate the gem and version you are reporting the issue against.
end

puts "Ruby version is: #{RUBY_VERSION}"
require 'rspec/autorun'

RSpec.describe 'changes with composed matchers are detectable' do
  class SomethingExpected
    attr_accessor :some_value
  end
  
  it 'detects that the attribute has changed when composed' do
    obj = SomethingExpected.new
      
    expect { obj.some_value = 1 }.to change { obj }.
      from( having_attributes(some_value: nil) ).
      to( have_attributes(some_value: 1) )
  end

  it 'detects that the attribute has changed when using the matcher directly' do
    obj = SomethingExpected.new

    expect(obj).to have_attributes(some_value: nil)
    obj.some_value = 1
    expect(obj).to have_attributes(some_value: 1)
  end
end

Expected behavior

Both of these tests should pass.

Actual behavior

The test with direct use of have_attributes passes; the test using change fails.

Notes

I recognise that #== not respecting these attributes is potentially problematic. It seems worth noting that I encountered this in a Rails app, given #1100.

There seem to be some semantic difficulties with change and composition. This might suggest documenting this restriction over breaking it. I'd be happy to take a stab at implementing either of these (if documenting, I'd like to see a matcher that does allow for this sort of approach)

[JonRowe]

👋 This has nothing to do with the composition, the hashes are just equal before / after mutation of the attributes, so we need to improve the detection of changes here.

[pirj]

Another way to fix this is to implement hash for those value objects, e.g. with the following definition of the subject the spec passes just fine.

  class SomethingExpected
    attr_accessor :some_value

    def hash
      some_value.hash
    end
  end

It's a temporary solution. It may also be problematic if you happen to use those objects as Hash keys (will have to rehash the Hash).

Agree that even though that if the hash has not changed is not an 100% indication that the object itself didn't change, since the set of allowed values of a hash is limited.

[JonRowe]

The solution I've been working up involves using the instance attributes of an object as an additional hash signature, see #1132

[JonRowe]

Closed by #1132

[maximeg]

Hi @JonRowe,

We should reopen this issue as #1132 has been reverted, and as of rspec 3.10.0, this is still an annoying and unexpected issue.

Annoying an unexpected considering the given message that makes us think that that should work and the issue is in our code:

expected `model` to have changed from having attributes {:foo => "a"} to having attributes {:foo => "b"}, but did not change

For lurkers, a possible workaround being turning:

expect {
  # things
}.to change { model }
  .from(having_attributes(foo: "a"))
  .to(having_attributes(foo: "b"))

into:

expect {
  # things
}.to change(model, :attributes)
  .from(a_hash_including("foo" => "a"))
  .to(a_hash_including("foo" => "b"))

Thanks!

[pirj]

Tracked in rspec/rspec-rails#1173

[JonRowe]

Yeah I think this is better off as a rspec-rails concern, it could extend the base change matcher to detect models and check attributes instead, but the main rspec gems don't know about rails

[antulik]

As mentioned in rspec/rspec-rails#1173 another workaround is to call model.dup, it will keep all fields except id

expect {
  # things
}.to change { model.dup }
  .from(having_attributes(foo: "a"))
  .to(having_attributes(foo: "b"))