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

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.

About this issue

  • Original URL
  • State: open
  • Created 10 years ago
  • Comments: 22 (17 by maintainers)

Commits related to this issue

Most upvoted comments

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.

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.

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.