Change matcher keeps object reference #41

Closed
felipeelias opened this Issue Nov 15, 2010 · 9 comments

Comments

Projects
None yet
5 participants

I'm trying to do this:

require 'rubygems'
require 'rspec'

class TestBug
  attr_reader :array

  def initialize
    @array = []
  end

  def add(n)
    @array << n
  end
end

describe TestBug do
  it "should NOT fail" do
    @bug = TestBug.new

    expect {
      @bug.add(1)
    }.to change {
      @bug.array
    }.from([]).to([1])
  end
end

I think this spec should not fail. I debbuged the matchers/change.rb file and noticed that after the line 17 gets executed, the value of @before changes to the same value as @after

  # lib/rspec/matchers/change.rb
  def matches?(event_proc)
    raise_block_syntax_error if block_given?

    @before = evaluate_value_proc
    event_proc.call # <- HERE
    @after = evaluate_value_proc

    (!change_expected? || changed?) && matches_before? && matches_after? && matches_amount? && matches_min? && matches_max?
  end

Is this a bug or I'm trying to do something wrong?

Owner

dchelimsky commented Nov 16, 2010

Is this a bug or I'm trying to do something wrong?

Maybe a bit of both? The problem is that the underlying value of @bug.array is not the array itself, but rather a pointer the array on the heap. The block { @bug.add(1) } doesn't change that pointer, it changes the contents of the referenced array. If you agree with that interpretation, then the right thing to do is say:

   expect {
     @bug.add(1)
   }.to change {
     @bug.array.dup # added dup here to store a snapshot
   }.from([]).to([1])
 end

Admittedly this does seem a bit counter-intuitive, but I don't think RSpec should try to resolve this by dup'ing the value internally because that might lead to breaking specs that exist out in the wild. I'm open to suggestions, if anybody has one.

The #dup method works fine... I agree, RSpec shouldn't try to resolve this.
Anyway, is up to you to close this issue or not. Thanks

Owner

dchelimsky commented Nov 16, 2010

I'll leave this open until there is some documentation about it. It's a case that might come up more than once.

Thx.

I just had the same problem. Theoretically the code of matches? could be changed to compute the error message, somewhat like this:

if(evaluate_value_proc != @from)
    # handle error
end
event_proc.call
if(evaluate_value_proc != @to)
    # handle error
end

But with the current implementation this could get ugly and screw up some things.

Owner

dchelimsky commented Mar 21, 2011

Dup arrays and hashes in change matcher.

This fixes a bug in which expecting the contents of an array or hash to
change due to some action would fail (false negative) because the before
and after values were actually the same object.

dchelimsky closed this Mar 21, 2011

Owner

myronmarston commented Dec 10, 2012

As reported by @akicho8 in rspec/rspec-core#746:

Case to fail

$ ruby -r rspec/autorun -e 'describe{it{v="a"; expect{v << "b"}.to change{v}.to("ab")}}'
F

Failures:

  1)
     Failure/Error: Unable to find matching line from backtrace
       result should have changed, but is still "ab"
     # /usr/local/rvm/gems/ruby-1.9.3-p286/gems/rspec-expectations-2.12.0/lib/rspec/expectations/fail_with.rb:33:in `fail_with'
     # /usr/local/rvm/gems/ruby-1.9.3-p286/gems/rspec-expectations-2.12.0/lib/rspec/expectations/handler.rb:33:in `handle_matcher'
     # /usr/local/rvm/gems/ruby-1.9.3-p286/gems/rspec-expectations-2.12.0/lib/rspec/expectations/expectation_target.rb:34:in `to'

Finished in 0.00141 seconds
1 example, 1 failure

Case through

$ ruby -r rspec/autorun -e 'describe{it{v="a"; expect{v += "b"}.to change{v}.to("ab")}}'
.

Finished in 0.0014 seconds
1 example, 0 failures

The failure did not change the object_id, I go through the change

$ ruby -r rspec -e 'p RSpec::Version::STRING'
"2.12.0"

myronmarston reopened this Dec 10, 2012

Owner

myronmarston commented Dec 10, 2012

The problem is that in ae1c3f8 we added special handling for Array and Hash as the main container types. Looks like we should do the same for String.

@alindeman / @dchelimsky -- any concerns that doing so might break existing specs?

Contributor

alindeman commented Dec 10, 2012

@alindeman / @dchelimsky -- any concerns that doing so might break existing specs?

I think it's OK to add String to the list since it is more common to mutate them in place. I worry that it'll continue to be surprising for other types of objects, but I don't think we want to be in the business of duping everything.

Owner

myronmarston commented Dec 10, 2012

I worry that it'll continue to be surprising for other types of objects, but I don't think we want to be in the business of duping everything.

I'd be worried about that, too, but I'm encouraged by the fact that @dchelimsky fixed it for Array and Hash 2 years ago and we haven't had any reports about problems with other types until now. Also, thinking about it some more...I think that there are 2 things an object needs to suffer from this problem:

  • It needs to be a container type. For all other situations (i.e. an object holding a single reference to another object) this doesn't seem to be an issue.
  • It needs a simple literal syntax so the before/after can easily be expressed in to and from.

There aren't many types in ruby that meet both criteria.

@akicho8 -- care to take a stab at fixing this by updating the logic in the change matcher so that it dups Strings as well?

@kchien kchien pushed a commit to kchien/rspec-expectations that referenced this issue Mar 7, 2014

@dchelimsky dchelimsky Dup arrays and hashes in change matcher.
This fixes a bug in which expecting the contents of an array or hash to
change due to some action would fail (false negative) because the before
and after values were actually the same object.

- Closes #41
ae1c3f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment