`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.

Widgets
  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.