Rspec-rails: `change` doesn't handle ActiveRecord objects that well

[cupakromer]

I ran into this today while trying to use the new have_attributes matcher:

widget = create(Widget, name: 'widget', package: 'single')
form_attributes = {
  id: widget.to_param,
  widget: {
    name: 'A new name',
    package: 'bulk',
  }
}

expect {
  put :update, form_attributes, valid_session
}.to change{ Widget.find(widget.id) }.to have_attributes(
  name: 'A new name',
  package: 'bulk',
)

Running this fails with:

expected result to have changed to have attributes { ... }, but did not change

Root Cause

The root of this issue is this line from the RSpec::Matchers::BuiltIn::ChangeDetails internal implementation used by the change matcher:

@actual_before != @actual_after

It's attempting to check that the objects aren't equal, thus not changing. However, ActiveRecord overrides == to mean do these objects have the same id value. If so, then they are the same object. This causes change to fail.

Workarounds

• Call dup on ActiveRecord objects in the change block. This will remove the id resulting in ActiveRecord to fall back on a more intensive attribute match.
• Call ActiveRecord#attributes to get the attribute list and use RSpec's standard hash matchers such as include.

[cupakromer]

I'm not sure of the right way for rspec-rails to handle this. Other than attempting to shim / monkey patch the default change matcher; something I'd like to try to avoid if at all possible.

[myronmarston]

Hmm, this reminds me of rspec/rspec-expectations#617. The root problem is that you intend to specify that it didn't have certain attributes and now it does (the before/after matcher check) where as change, historically, used == and by that definition, there wasn't a change.

Not sure what the solution is. One simple thing we can do for now is to improve the failure message so that it's clear what's going on...maybe the message could be something like this?

expected result to have changed to have attributes { ... }, but did not change (checked using `before_value != `after_value`)

This would at least make it clear that == was used.

[cupakromer]

@myronmarston I do think it is a bit related. Though it's also a little different. As part of the issue has to do more with how change works under the hood.

Historically, change allowed two forms:

• change(obj, :property)
• change { some_return_value }

Prior to the introduction of composable matchers with RSpec 3, these two forms made a lot of sense. They were encouraging the "test one thing" guideline and attempting to make sure that what change was testing was a particular value.

Using change to do something like change { some_object } was a hack to try to work around the change(obj, :property) limitation. It wasn't really intended to be used this way. This hack isn't seen very often outside of the Rails world.

We made a few exceptions with this limitation though, when it came to objects which were Enumerable or strings. Part of the reasoning here was people often returned the same internal array instance, or modified string, when accessing a property, thus: change(library, :books) could very much return the same modified array object each time.

As a side effect, this also means, if I just had access to an array or hash I could use the "hack" of change { my_hash } and things just worked as expected.

Adding on now that we have composable matchers, and for discussion purposes I'm going to consider have_attributes as a nicely bundled composed matcher, it's possible to roll-up what were once multiple specs into a single change spec. This also means, how change gets used is a bit different:

Consider this set of contrived specs which demonstrates this outside of rails:

Book = Struct.new(:title, :author)

class Author
  attr_accessor :name, :pseudonym

  def initialize(name = nil, pseudonym = nil)
    @name = name
    @pseudonym = pseudonym
  end
end

RSpec.describe 'working with have_attributes and change' do

  context 'mutating an Enumerable' do
    it 'handles changes on the same object' do
      widgets = [:one, :two, :three]

      expect { widgets.shift }.to change { widgets }.to eq [:two, :three]
    end
  end

  context 'using a Struct' do
    it 'handles changes on the same object' do
      book = Book.new('Title1', 'Some Author')

      expect { book.title = 'Title2' }.to change{ book }.to have_attributes(
        title: 'Title2',
        author: 'Some Author',
      )
    end
  end

  context 'using a normal Ruby class' do
    def set_attributes(obj, attributes)
      attributes.each_pair { |k, v| obj.public_send("#{k}=", v) }
    end

    def update(author)
      set_attributes author, name: "Real Name", pseudonym: "Pen Name"
    end

    context 'change can really only check a single attribute at a time' do
      it 'changed the name' do
        author = Author.new
        expect { update author }.to change(author, :name).to 'Real Name'
      end

      it 'changed the pseudonym' do
        author = Author.new
        expect { update author }.to change { author.pseudonym }.to 'Pen Name'
      end
    end

    it 'handles changes on the same object', pending: 'fails b/c object equality' do
      author = Author.new
      expect { update author }.to change { author }.to have_attributes(
        name: "Real Name",
        pseudonym: "Pen Name",
      )
    end
  end

end

A Struct works because it implements == to check all of it's attributes. Since the default Ruby == implementation just means, "are these two objects exactly the same", a normal class will fail the != check. This is true even if an explicit from condition is provided; which makes it a little different from the other issue.

I think writing all of this out has given me a new perspective on both change and the other issue. When I re-read the other expectations issue, I read it as: "Fail even if the object changes, but the expected condition was the same on the before object." This issue is more: "Pass even if the object doesn't change, but the expectation does (and was not met before)."

I believe these issues are compliments of the other. Though I think at the end of the day, it's very likely there isn't a solution here due to the complexity of composable matchers.

[myronmarston]

Prior to the introduction of composable matchers with RSpec 3, these two forms made a lot of sense. They were encouraging the "test one thing" guideline and attempting to make sure that what change was testing was a particular value.

Using change to do something like change { some_object } was a hack to try to work around the change(obj, :property) limitation. It wasn't really intended to be used this way. This hack isn't seen very often outside of the Rails world.

I don't agree with you here. I personally have always used the change { ... } form. (I'm not even sure if I've ever used the change(obj, :property) form outside of rspec-expectations' specs). IMO, the block form is more natural. I don't consider it a hack at all. The obj, :property form can produce a bit nicer output (since it knows the :property name), but it's always seemed simpler/more consistent to me to just use the block form and the output wasn't enough nicer to warrant using the obj, :property form.

We made a few exceptions with this limitation though, when it came to objects which were Enumerable or strings. Part of the reasoning here was people often returned the same internal array instance, or modified string, when accessing a property, thus: change(library, :books) could very much return the same modified array object each time.

As a side effect, this also means, if I just had access to an array or hash I could use the "hack" of change { my_hash } and things just worked as expected.

The issues with enumerables and strings have nothing to do with which form of change is used. The problem was the same with either form. The problem was that the before/after values were the exact same string/hash/array/whatever object. The contents had changed, but since we did the != check after the change, it was comparing the exact same object against itself.

Anyhow, I think that the underlying issue is that composable matcher allow users to express new things (specifically, that a particular object has changed to match a matcher where it didn't before), and the != check we do no longer is what the user is trying to express. Doing something like rspec/rspec-expectations#617 will solve this, I think.

[cupakromer]

The problem was that the before/after values were the exact same string/hash/array/whatever object. The contents had changed, but since we did the != check after the change, it was comparing the exact same object against itself.

Hmm, I wasn't aware that was the original intent of the special case duping. What I wrote was my prior understanding of expected intent. I'm glad to have this cleared up. Though I wonder why we only special cased those objects, I didn't see anything obvious from the git history (granted I didn't spend too much time digging into it).

This is what the issue here represents. The contents of the model has changed but it's "the same object" from =='s point of view.

I think that the underlying issue is that composable matcher allow users to express new things (specifically, that a particular object has changed to match a matcher where it didn't before)

I realize I primed this conclusion, but this issue existed before. If have_attributes was around in 2.x the problem would still have been there. It also existed for anyone who wrote a custom matcher which did something similar.

Given these new insights on expected behavior, I would have originally thought that change was fundamentally broken. Though I think this brings up an interesting distinction between the meaning of change: a change in value, a change in state, or both.

[JonRowe]

Is it worth rspec-rails overriding the change matcher and doing some special case logic for ActiveRecord objects?

[cupakromer]

I'm not sure it is. I'm also not sure what the proper behavior would be for that. Calling dup on an AR object removes id from the object. While that likely would appear to fix the issue, I'm sure I can come up with a situation where that would cause other problems. Additionally, with the complexity of composable matchers, I'm not sure that gains us that much either.

[JonRowe]

Actually I was suggesting something modifying the match logic not to use == when it's an ActiveRecord object, it would require some modification of the expectations api to take that though.

[myronmarston]

Though I wonder why we only special cased those objects, I didn't see anything obvious from the git history (granted I didn't spend too much time digging into it).

You can read prior discussions in rspec/rspec-expectations#41 and rspec/rspec-expectations#273.

Basically, we didn't know of any other types this issue was a problem with at the time.

I realize I primed this conclusion, but this issue existed before. If have_attributes was around in 2.x the problem would still have been there. It also existed for anyone who wrote a custom matcher which did something similar.

Passing matchers to change (or any other matcher) wasn't really well supported in RSpec 2.x, so while you are right that the problem existed, in practice, users didn't seem to run into it.

Anyhow, I think that the right path forward is far from clear, so let's stew on this for a bit.

[pirj]

Wondering if this has been fixed with the recent changes to change here and here. Wondering if this is still a problem?

[benoittgt]

I looked into it and was able to reproduce the issue. See last specs, it is still failing.

  PUT update
    update Widget
    update Widget
    update Widget (FAILED - 1)

Failures:

  1) Widgets PUT update update Widget
     Failure/Error:
       expect {
         patch(widget_url(widget), params: form_attributes)
       }.to change{ Widget.find(widget.id) }.to have_attributes(
         name: 'A new name',
         package: 'bulk',
       )

       expected `Widget.find(widget.id)` to have changed to have attributes {:name => "A new name", :package => "bulk"}, but did not change
     # ./spec/requests/widgets_request_spec.rb:44:in `block (3 levels) in <top (required)>'

Finished in 0.11003 seconds (files took 1.11 seconds to load)
3 examples, 1 failure

I also wrote two workarounds if someone needs help on this.

[emilyst]

I encountered this issue today and lost about an hour on it. My workaround was to use #dup as suggested above.

[pirj]

So the hash for any model is a XOR or class hash and id hash. No matter the attributes, it's a constant for the same row:

# activerecord-6.0.2.2/lib/active_record/core.rb:446:
def hash
  if id
    self.class.hash ^ id.hash
  else
    super
  end
end

and == for models is defined as:

# activerecord-6.0.2.2/lib/active_record/core.rb:429
def ==(comparison_object)
  super ||
    comparison_object.instance_of?(self.class) &&
    !id.nil? &&
    comparison_object.id == id
end

In the change matcher, we check:

      class ChangeDetails
        def changed?
          @actual_before != @actual_after || @before_hash != @actual_hash

and both those comparisons return false.

It's not really fair, not only in the context of RSpec Rails:

  specify do # this example fails
    a = []
    b = 1
    expect { a << b }
      .to change { b }
      .to satisfy { |value| a.include?(value) }
  end

  specify do # this example passes
    a = []
    b = 1
    # expect(b)
    #   .to satisfy { |value| a.include?(value) } # this would fail
    a << b
    expect(b)
      .to satisfy { |value| a.include?(value) }
  end

it's a pretty contrived example, but still.

So, in my opinion, if change is given a qualifier, it should not ignore the qualifier as it does now:

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

if change_details.changed? evaluates to false as it is in the abovementioned cases.

I'm going to address this in rspec-expectations, as this is not rspec-rails specific.

[pirj]

@emilyst there's an unreleased change in rspec-expectations that might fix this case. Do you mind to give it a shot?

group :development, :test do
  %w(core rails expectations support mocks).each do |repo|
    gem "rspec-#{repo}", github: "rspec/rspec-#{repo}"
  end
end

Please be aware that it may break negated expectations like:

expect { }.not_to change { Model.find(id) }

and

expect { }.not_to change { model.reload }

[pirj]

I guess since existing approaches:

• check instance variables along with the equality check and hash equality check
• ignore the change { ... } part equality if qualifiers are chained
• extend the private internal implementation of change in rspec-rails

we may consider another approach - open a Rails ticket pointing out that two objects are not necessarily equal if their class and their id are equal. This totally makes sense now with multiple databases support, think multi-tenant application, when the same model can be fetched from different databases, have the same id, but have completely different attributes.

Do you think this reasoning enough to convince the Rails Core team that the implementation of == should be reconsidered?
This way we have a chance that somewhere in 6.1 out problem with change ... .to have_attributes will be fixed.

[myronmarston]

Asking the rails team to change the definition of equality of AR model objects is unlikely to succeed. There would be a ton of repercussions of that, and it's likely to create more problems than it solves.

It seems like with AR model objects we want the change matcher to detect changes on the basis of model.attributes. So, one idea is to have rspec-rails customize the change matcher so that when the object returned by the change { ... } block is_a?(ActiveRecord::Base) it uses model.attributes to detect changes instead of the model instance itself. This could be done by having rspec-rails override the change method to return some kind of alternate matcher implementation or by having rspec-expectations expose a customization API that rspec-rails uses to register ActiveRecord::Base to behave this way.

That said, I think it could create even more confusion to make change behave differently for different kinds of objects. The solution might be documentation improvements. We can document that change relies on == and hash to detect changes, and explain that objects that implement those to not change based on internal state will not work properly with this matcher. We can also call out AR model objects as being in this category and document model.attributes as being a work around.

[emilyst]

That said, I think it could create even more confusion to make change behave differently for different kinds of objects. The solution might be documentation improvements. We can document that change relies on == and hash to detect changes, and explain that objects that implement those to not change based on internal state will not work properly with this matcher. We can also call out AR model objects as being in this category and document model.attributes as being a work around.

What if the problem is that change is too general and too vague? Perhaps some new matchers named, e.g., change_attributes or change_model or similar could make this situation more ergonomic and self-documenting.

[myronmarston]

What if the problem is that change is too general and too vague? Perhaps some new matchers named, e.g., change_attributes or change_model or similar could make this situation more ergonomic and self-documenting.

I like this idea a lot -- and it is pretty easy to implement a working version:

def change_attributes(&block)
  change { block.call.attributes }
end

That said, there's some edge cases that would need to be handled for a version of this offered by RSpec, such as change_attributes(post, :owner) (vs change_attributes { post.owner }) and if we're going to call it change_attributes it should probably handle _any_ object, not just AR models, given that we already have have_attributes which handles any object. The implementation to handle arbitrary objects would have to be more involved (but is doable) or we could call this change_model_attributes and restrict it to AR model objects.

[pyromaniac]

I would like to add to the discussion a bit. I believe that this == behavior of change matcher should be used only when there is no from or to values given. Otherwise, at least for me, it is natural that we should compare only the values provided to from and to.

expect { update }.to change { something }
@value_before == @value_after

expect { update }.to change { something }.from(from_value)
match(@value_before, @from_value) && !match(@value_after, @from_value) if @from_given

expect { update }.to change { something }.to(to_value)
!match(@value_before, @to_value) && match(@value_after, @to_value) if @to_given

And so on. I believe if we improve this part, the matcher will do exactly what developer wants. WDYT? Does it have downsides?

The only possible downside I see is that is values in from and to appear to be equal - it will be a false positive. In this case we might also want to compare those values additionally and raise a warning, for example. Is it possible to compare matcher instances by any chance? :)

[pirj]

@pyromaniac I clearly see what you mean.

class Keeper
  def initialize(value)
    @value = value
  end

  attr_accessor :value
end

RSpec.describe Keeper do
  subject(:keeper) { Keeper.new(1) }

  it 'expect to change keeper' do
    expect {
      keeper.value = 2
    }.to change { keeper }
  end

  it "expect to change keeper's value" do
    expect {
      keeper.value = 2
    }.to change { keeper.value }
  end

  it "expect to change keeper from" do
    expect {
      keeper.value = 2
    }.to change { keeper }
      .from(have_attributes(value: 1))
  end

  it "expect to change keeper to" do
    expect {
      keeper.value = 2
    }.to change { keeper }
      .to(have_attributes(value: 2))
  end

  it "expect to change keeper to" do
    expect {
      keeper.value = 2
    }.to change { keeper }
      .from(have_attributes(value: 1))
      .to(have_attributes(value: 2))
  end
end

```
5 examples, 4 failures

(except the one that has `change { keeper.value }`).

But how about this:
```ruby
class Keeper
  def initialize(value, other_attr)
    @value = value
    @other_attr = other_attr
  end

  attr_accessor :value, :other_attr
end

RSpec.describe Keeper do
  $keeper = Keeper.new(1, 'hello')

  it "expect not to change keeper's value" do
    expect {
      $keeper = Keeper.new(1, "oh, I've changed a lot actually")
    }.not_to change { $keeper }
      .from(1)
  end
end

Should it be considered changed or not?
From my point of view, change should tell that the object has changed, while a hypothetical change_attributes would only consider attributes (and only those that it has been told to consider).

[pyromaniac]

Your last example is going to work with the proposed implementation without .from(1), right? And with a from argument it is also going to work as expected. And if the initial object is not equal to the from value, it is clearly a failure. Can't see what's wrong with it tbh. Here we have an example of a variable (and it is a pointer) has changed to point to another object. Isn't this how it works now? Isn't this how it is going to work with the proposed adjustments?