Improve change matcher detection of objects that have changed.

[JonRowe]

If an object is == and it's hash results are == it can still have changed, as it seems instance variable contents are not considered changes. One solution to this problem is to also compare the hashes of the instance attributes. This might have some performance problems so I'm not totally convinced this is the go but it does solve the problem.

Fixes #1131

[benoittgt]

This might have some performance problems so I'm not totally convinced this is the go but it does solve the problem.

Maybe we can write a benchmark?

If there is a cost. Maybe we should make this change available via configuration flag?

[pirj]

I'm still trying to grasp change matcher implementation, and it seems that in the case when used with qualifiers (e.g. to or from), it's not really necessary to even compare for hashes or equality.

The following naïve change:

def matches?(event_proc)
-  perform_change(event_proc) && @change_details.changed? && @matches_before && matches_after?
+  perform_change(event_proc) && @matches_before && matches_after?
end

only breaks a few of the examples, that leads me to a thought it might be possible to use @change_details only when no qualifiers are specified.

Not sure if it's technically feasible though.

[JonRowe]

The change matcher itself has the semantic of the result under test having had to have changed, there are a lot of qualifiers that could pass without the underlying object changing so I don't want to rely on them

[JonRowe]

I ran benchmarks on the hash calculation method

Calculating -------------------------------------
Get hashes for object of size: 10
                        158.585k (± 5.2%) i/s -    792.660k in   5.012663s
Calculating -------------------------------------
Get hashes for object of size: 100
                         17.041k (± 9.0%) i/s -     84.864k in   5.036735s
Calculating -------------------------------------
Get hashes for object of size: 1000
                          1.808k (± 4.6%) i/s -      9.169k in   5.082355s
Calculating -------------------------------------
Get hashes for object of size: 10000
                        176.298  (± 3.4%) i/s -    896.000  in   5.089070s

So even with a really large object with 10'000 instance variables we can gather the hash in under 10 ms so I'm happy merging this if you are

[JonRowe]

/cc @benoittgt

[benoittgt]

Thanks a lot for the benchmark!

LGTM

[pirj]

This seems to break some existing specs.
Debugging:

[10] pry(#<RSpec::Matchers::BuiltIn::ChangeDetails>)> @actual_before.instance_variables.select { |v| @actual_before.instance_variable_get(v) != @actual_after.instance_variable_get(v) }
=> [:@attributes,
 :@association_cache,
 :@transaction_state,
 :@_new_record_before_last_commit,
 :@errors,
 :@_already_called,
 :@_database_validations_fallback,
 :@new_record_before_save,
 :@mutations_before_last_save,
 :@attributes_changed_by_setter]
[11] pry(#<RSpec::Matchers::BuiltIn::ChangeDetails>)> @actual_before.instance_variable_get :@transaction_state
=> #<ActiveRecord::ConnectionAdapters::TransactionState:0x00007fad88571100
 @children=
  [#<ActiveRecord::ConnectionAdapters::TransactionState:0x00007fad898d88d8
    @children=[],
    @state=:committed>],
 @state=:committed>
[12] pry(#<RSpec::Matchers::BuiltIn::ChangeDetails>)> @actual_after.instance_variable_get :@transaction_state
=> nil

Those are service instance variables that are semantically unrelated to the object itself and doesn't really change its state.

I'm not certain if it improves things, since now some of the negated change expectations start failing:

expect { some_operation_that_might_or_might_not_change }.not_to change { user.profile }

[benoittgt]

Do we miss regression tests?

[JonRowe]

No, its just more accurate than it was before, I mean the object is changing, its just a question of if this is a semantically change object or an actually changed one

[pirj]

@JonRowe For some reason, this change:

          @actual_before != @actual_after ||
          @before_hash != @actual_hash ||
          @before_attribute_hashes != @after_attribute_hashes

didn't make it into 3.9.2:

          @actual_before != @actual_after || @before_hash != @actual_hash

even though it's there on master.

I've tested, it does resolve rspec/rspec-rails#1173.

There's an edge case - when no qualifier is chained, it passes.
Nothing in the object has changed apart @inspection_filter's hash and @attributes's hash.
@attributes for a model != model.attributes, the former is an ActiveModel::AttributeSet, while the latter is a hash.

  specify do
    widget = Widget.create(name: 'widget', package: 1)

    expect {
      # widget.update(name: 'A new name', package: 2)
    }.to change { Widget.find(widget.id) }
  end

I'm still concerned if this change should be released, as it may break Rails expect { ... }.not_to change { Model.find(id) } specs because of this.

[JonRowe]

This whole PR is unreleased due to concerns about bugs / performance issues.

[JonRowe]

Really I should revert it from master.

[JonRowe]

This has been reverted due to performance concerns, if someone wants to tackle this again (even using this as a base) please do!