[rspec-users] Should change not comparing arrays how I expected

Michael Latta lattam at mac.com
Sun Sep 28 14:44:49 EDT 2008


Is your patch AR proxy specific?  If it is for any collection, it  
prevents two collections from being compared for equality.  I have had  
many examples of collections that are not simple containers, and only  
comparing the contents would be equally invalid as the simple equality  
on AR proxies.  If you added an AR specific method / test that would  
have less impact on other cases, but of course be less clean.  That is  
why I suggested separate matchers, since I know of cases where your  
patch would be incorrect.

Michael


On Sep 28, 2008, at 11:35 AM, David Chelimsky wrote:

> On Sun, Sep 28, 2008 at 1:18 PM, Michael Latta <lattam at mac.com> wrote:
>> David,
>>
>> It seems to me that the root of the problem is that the  
>> specification is
>> incorrect.  Since Rails returns association proxies the  
>> specification fails
>> because it does not specify what the behavior should be.  I would  
>> suggest
>> that instead of patching the change matcher, that you should add a
>> change_contents matcher that matches the contents of a collection  
>> vs. the
>> contents of a collection.  That way the framework is not guessing  
>> what was
>> meant, but relying on the specification to be correct.  Since that  
>> is really
>> what you want to specify (the contents have changed).  I think this  
>> is
>> cleaner.
>
> I think what you propose would be objectively more explicit, but
> "cleanliness" is a very subjective thing.
>
> This is tricky because AR is simultaneously violating the principle of
> least surprise by giving you a proxy instead of the real collection
> AND adhering to principles of encapsulation and duck-typing. The fact
> that team.players is a proxy should not matter to its consumer one way
> or the other as long as it can treat it just like the collection of
> players.
>
> Now, in the case of what rspec is doing in the change matcher, I would
> say that rspec is getting bitten by the violation of the principle of
> least surprise because the matcher is doing something that AR is not
> expecting - further delaying evaluation of AR::A::AP's delayed
> evaluation. But I think rspec can deal w/ that and alleviate the
> burden on the rspec-rails user to know two different matchers and when
> to use them.
>
> I've already implemented this so it's all transparent for the user:
> http://rspec.lighthouseapp.com/projects/5645-rspec/tickets/545.
>
> I'm open to re-opening this but I'd like some feedback from other
> people before I do. Anybody else have any opinions?
>
> David
>
>
>
>
>> Michael
>>
>>
>> On Sep 28, 2008, at 11:13 AM, David Chelimsky wrote:
>>
>>> On Sun, Sep 28, 2008 at 11:01 AM, David Chelimsky <dchelimsky at gmail.com 
>>> >
>>> wrote:
>>>>
>>>> On Sun, Sep 28, 2008 at 10:43 AM, David Chelimsky <dchelimsky at gmail.com 
>>>> >
>>>> wrote:
>>>>>
>>>>> On Sun, Sep 28, 2008 at 9:47 AM, Ashley Moran
>>>>> <ashley.moran at patchspace.co.uk> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Just had a surprising result:
>>>>>>
>>>>>> it "should not appear in the Story.unposted list" do
>>>>>> @story.save
>>>>>> lambda {
>>>>>>   @story.post_to_twitter(@twitter_client)
>>>>>> }.should change { Story.unposted }.from([@story]).to([])
>>>>>> end
>>>>>>
>>>>>> 'Story#post_to_twitter should not appear in the Story.unposted  
>>>>>> list'
>>>>>> FAILED
>>>>>> result should have been changed to [], but is now []
>>>>>>
>>>>>> Anyone know why this fails?  I've looked in change.rb but I can't
>>>>>> figure it
>>>>>> out.
>>>>>
>>>>> Whenever I've seen output like "should have been foo, but was  
>>>>> foo" it
>>>>> has boiled down to AR Assocation Proxies, which don't align in  
>>>>> their
>>>>> response to == and inspect.
>>>>>
>>>>> I'm looking at seeing if there's a way we can make "should change"
>>>>> work in spite.
>>>>
>>>> Wow.
>>>>
>>>> OK - here's what I figured out. Talk about insidious bugs! This is
>>>> actually quite a bit different from what I thought.
>>>>
>>>> There are two lambdas involved here:
>>>>
>>>> lambda {
>>>> 1st lambda: expression that should cause the change
>>>> }.should change{
>>>> 2nd lambda: expression that returns the object that should change
>>>> }.to(expected value)
>>>>
>>>> The matcher executes the 1st lambda and stores the result as  
>>>> @before.
>>>> In your example this is a Rails association proxy for the
>>>> Story.unposted collection.
>>>>
>>>> The matcher then executes the 2nd lambda.
>>>>
>>>> The matcher then executes the 1st lambda again and stores the  
>>>> result
>>>> as @after. In your example, this is, again, a Rails association  
>>>> proxy
>>>> for the Story.unposted collection.
>>>>
>>>> At this point, @before and @after point to the same object - the  
>>>> same
>>>> Rails association proxy!!!!!!
>>>>
>>>> The matcher passes if @before != @after and fails if @before ==
>>>> @after. Because they are actually the same association proxy, the
>>>> example fails.
>>>>
>>>> Now rspec asks the matcher to print out the reason why it failed,
>>>> which does this:
>>>>
>>>> "#{result} should have been changed to #{@to.inspect}, but is now
>>>> #{@after.inspect}"
>>>>
>>>> @to is the expected value []
>>>> @after is the association proxy, which proxies to an empty  
>>>> collection.
>>>> Now, when the matcher calls @after.inspect, is the first time  
>>>> that the
>>>> proxy is actually evaluated!!!!
>>>>
>>>> Does this make sense?
>>>>
>>>> I was able to get a similar example to pass by doing this  
>>>> immediately
>>>> after storing the proxy in the @before variable:
>>>>
>>>> @before = @before.collect{|item|item} if @before.respond_to? 
>>>> (:collect)
>>>>
>>>> Ugly, ugly, ugly. But perhaps necessary to deal w/ this problem.
>>>>
>>>> I think I'll restructure things so the the change matcher handles  
>>>> this
>>>> in rails, but not in core rspec.
>>>>
>>>> Thoughts?
>>>
>>> FYI - ticket added and problem resolved:
>>> http://rspec.lighthouseapp.com/projects/5645-rspec/tickets/545
>>>
>>>>
>>>>>
>>>>>
>>>>>> I can make it work with:
>>>>>> should change { Story.unposted.length }.from(1).to(0)
>>>>>>
>>>>>> But that's a weaker test.
>>>>>>
>>>>>> Thanks
>>>>>> Ashley
>>>>>>
>>>>>> --
>>>>>> http://www.patchspace.co.uk/
>>>>>> http://aviewfromafar.net/
>>>
>>> _______________________________________________
>>> rspec-users mailing list
>>> rspec-users at rubyforge.org
>>> http://rubyforge.org/mailman/listinfo/rspec-users
>>
>> _______________________________________________
>> rspec-users mailing list
>> rspec-users at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/rspec-users
>>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users



More information about the rspec-users mailing list