[rspec-devel] [PATCH] should_change

Brandon Keepers bkeepers at gmail.com
Thu Nov 16 10:06:20 EST 2006


I forgot to add support for chaining #to and #from.  Here's an  
updated patch.  Any thoughts?

Brandon

-------------- next part --------------
A non-text attachment was scrubbed...
Name: should_change.patch
Type: application/octet-stream
Size: 6176 bytes
Desc: not available
Url : http://rubyforge.org/pipermail/rspec-devel/attachments/20061116/a8bc4306/attachment-0001.obj 
-------------- next part --------------

On Nov 16, 2006, at 1:52 AM, Brandon Keepers wrote:

> Rubyforge appears to be down at the moment, so I'll just put this  
> in an email for now.
>
> Wilson, sorry, but I couldn't wait.  I went ahead and came up with  
> an implementation of should_change. It was actually really  
> simple...once I got acquainted with the rspec internals.  Attached  
> is a patch.
>
> Let me know what you think.  The messages could use some love, and  
> are we worried about what happens if someone calls #to, #from, or  
> #by without having first called #should_change?
>
> Brandon
>
> <should_change.patch>
>
> On Nov 15, 2006, at 11:54 PM, <noreply at rubyforge.org>  wrote:
>
>> Patches item #6417, was opened at 2006-10-31 16:36
>> You can respond by visiting:
>> http://rubyforge.org/tracker/? 
>> func=detail&atid=3151&aid=6417&group_id=797
>>
>> Category: None
>> Group: None
>> Status: Open
>> Resolution: None
>> Priority: 3
>> Submitted By: Wilson Bilkovich (wilson)
>> Assigned to: Nobody (None)
>> Summary: [PATCH] Support @object.some_method_should_be_changed_by  
>> {block}
>>
>> Initial Comment:
>> I was toying with how to implement the RSpec equivalent of  
>> "assert_difference" from test/unit.
>>
>> In case you haven't used it, it looks something like this:
>>
>> assert_difference some_object, :some_method { some code here }
>>
>>
>>
>> It calls some_object.some_method before and after the block, and  
>> fails if there is no difference.
>>
>>
>>
>> This patch implements something similarly generic:
>>
>>
>>
>> @some_object.some_method_should_be_changed_by {|obj| code here}
>>
>> ..and its inverse:
>>
>> @some_object.some_method_should_not_be_changed_by {|obj| code here}
>>
>>
>>
>> some_method is stripped from the beginning of the message, and  
>> passed forward for use inside the Should module.
>>
>>
>>
>> Has specs, and all other existing specs still pass.
>>
>>
>>
>> A real 'use case' example:
>>
>>
>>
>> User.count_should_be_changed_by do
>>
>>   User.create :login => 'quentin', :email => 'blah at example.com'...
>>
>> end
>>
>>
>>
>> User.count will be different before and after, so this expectation  
>> will be met.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 23:54
>>
>> Message:
>> One piece of trickery I haven't managed to think my way
>>
>> through yet is..
>>
>> If we want to support should_change().to("val")
>>
>> ..how can we support the bare should_change() call?
>>
>> If it needs to proxy for the 'to' and 'from' calls, how do
>>
>> we know there aren't more messages changed onto the end?
>>
>>
>>
>> The semi-equivalent 'Have' syntax seems to have avoided this
>>
>> by requiring a suffix method in every case.
>>
>> There's no:
>>
>> @thing.should_have(5)
>>
>> ..only @thing.should_have(5).things
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 23:46
>>
>> Message:
>> I agree with David. I actually really like this syntax. It feels  
>> very natural.
>>
>>
>>
>> lambda { ... }.should_change(obj, :method).to("val")
>>
>>
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 23:31
>>
>> Message:
>> Yeah, that and call_stack doesn't seem to work the way I'd
>>
>> expect:
>>
>> http://rpaste.com/pastes/575
>>
>>
>>
>> I made some progress with set_trace_func, but oh lord is it
>>
>> slow.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 23:24
>>
>> Message:
>> I don't think the syntax is *much* nicer, but that's really a  
>> subjective thing :)
>>
>>
>>
>> I'd prefer the previous options because they align better w/ other  
>> constructs like should_raise and should_throw that we already use  
>> for blocks. I'd also like to avoid any dependencies unless they  
>> buy us a lot.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 22:57
>>
>> Message:
>> Hrm. Just saw this:
>>
>> http://eigenclass.org/hiki.rb?call_stack
>>
>>
>>
>> It is possible that I can use that to implement the much
>>
>> nicer syntax:
>>
>>
>>
>> User.count.should_be_changed.by {block}
>>
>> User.count.should_be_changed.from(x).to(y).by {block}
>>
>>
>>
>> How do we feel about a dependency on that, or something similar?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 21:12
>>
>> Message:
>> The proc will be @target, so just do this:
>>
>>
>>
>> @target.call
>>
>>
>>
>> Syntax? I like .to(x), .from(y) and .by(z) because it feels more  
>> like most of the rest of rspec. It'll also result in a more OO  
>> implementation.
>>
>>
>>
>> Consensus? Don't see one yet, but you have my recommendation.
>>
>>
>>
>> Cheers,
>>
>> David
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 19:36
>>
>> Message:
>> So, what is the consensus?
>>
>>
>>
>> specify "should take some discussion to get right" do
>>
>>  proc{User.create :name => 'Bob'}.should_change(User, :count)
>>
>>
>>
>>  proc{User.noop}.should_not_change(User, :count)
>>
>> end
>>
>>
>>
>> specify "should be fairly tricky" do
>>
>>   proc{User.create :name => 'Bob'}.should_change(User,
>>
>> :count).by(1)
>>
>> end
>>
>>
>>
>> The 'by' version strikes me as a little domain-specific. It
>>
>> only works with values that support subtraction.
>>
>>
>>
>>
>>
>> Implementation-wise, how do I make the should proxy call the
>>
>> proc/lambda? Do we already have something like
>>
>> binding_of_caller in here?
>>
>>
>>
>> I also thing I slightly prefer the :to => and :from =>
>>
>> version (an options Hash), though I will admit that that's
>>
>> partly because it is easier to implement. It avoids needing
>>
>> to subclass Should.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 19:28
>>
>> Message:
>> And, just so we don't forget:
>>
>>
>>
>> { ... }.should_not_change(@tim, :name)
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 19:25
>>
>> Message:
>> Good discussions!  I'm really excited about this feature.
>>
>>
>>
>> I'm grasping at straws here because I don't know how the Rspec  
>> internals are implemented, but what if should_change(@tim, :name)  
>> actually called the proc and simply stored the before and after  
>> values, then #from, #to, and #by just called expectations on those  
>> references?  Then, all forms would work regardless of order?
>>
>>
>>
>>   # @before.should_not == @after
>>
>>   { ... }.should_change(@tim, :name)
>>
>>
>>
>>   # @before.should_not == @after
>>
>>   # @after.should == "Larry"
>>
>>   { ... }.should_change(@tim, :name).to("Larry")
>>
>>
>>
>>   # @before.should_not == @after
>>
>>   # @before.should == "Tim"
>>
>>   # @after.should == "Larry"
>>
>>   { ... }.should_change(@tim, :name).from("Tim").to("Larry")
>>
>>   { ... }.should_change(@tim, :name).to("Larry").from("Tim")
>>
>>
>>
>>   # @before.should_not == @after
>>
>>   # @before.should == @after + -1
>>
>>   { ... }.should_change(User, :count).by(-1)
>>
>>
>>
>> Would that work?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 19:10
>>
>> Message:
>> Wilson - this was your baby originally. You want to resubmit a patch?
>>
>>
>>
>> Also, I'm on the fence re: syntax per my last comment. We *could*  
>> do this:
>>
>>
>>
>> should_change(@tim, :name).to("Larry")
>>
>> should_change(@tim, :name).from('Tim').to('Larry')
>>
>> should_change(@tim, :age).by(1)
>>
>>
>>
>> If we call the proc on 'to' or 'by'. The nice thing about the  
>> other syntax is that it doesn't matter if you screw up the order:
>>
>>
>>
>> should_change(@time, :name, :to => 'Larry', :from => Tim);
>>
>> should_change(@time, :name, :from => 'Tim', :to => 'Larry');
>>
>>
>>
>> Those would behave the same way, whereas:
>>
>>
>>
>> should_change(@tim, :name).from('Tim').to('Larry')
>>
>> should_change(@tim, :name).to('Larry').from('Tim')
>>
>>
>>
>> would not. It also supports simply:
>>
>>
>>
>> should_change(@time, :name)
>>
>>
>>
>> without any specifics. Less declarative, true, but it becomes the  
>> developers choice.
>>
>>
>>
>> Let us know if you want to submit a patch, and if you do if you  
>> want some direction. Otherwise we'll add this sometime soon.
>>
>>
>>
>> Cheers,
>>
>> David
>>
>>
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 19:02
>>
>> Message:
>> I like the less verbose one, though that would be up to the  
>> developer.
>>
>>
>>
>> This aligns well w/ other expectations like:
>>
>>
>>
>> lambda {...}.should_raise
>>
>> lambda {...}.should_return (RFE 6187)
>>
>>
>>
>> I agree that we should add it now. And I think we should support  
>> the following forms:
>>
>>
>>
>> should_change(@tim, :name).to("Larry")
>>
>> should_change(@tim, :name).from('Tim').to('Larry')
>>
>> should_change(@tim, :age).by(1)
>>
>>
>>
>> Ah - but the middle one won't work because we won't know when to  
>> call the proc.
>>
>>
>>
>> How about this?
>>
>>
>>
>> should_change(@time, :name, :to => 'Larry');
>>
>> should_change(@time, :name, :from => 'Tim', :to => 'Larry');
>>
>> should_change(@time, :age, :by => 1);
>>
>>
>>
>> The reason I want from/to is to specify the value going in and  
>> coming out. I think that's more explicit.
>>
>>
>>
>> Thoughts?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Aslak Helles?y (aslak_hellesoy)
>> Date: 2006-11-15 18:48
>>
>> Message:
>> Right, it's not @tim changing his name, it's *something*. How about:
>>
>>
>>
>> lambda do
>>
>>   ...
>>
>> end.should_change(@tim, :name).to("Larry")
>>
>>
>>
>> or slightly more readable (but more verbose):
>>
>>
>>
>> updating_tim = lambda do
>>
>>   # do the post or whatever
>>
>> end
>>
>> updating_tim.should_change(@tim, :name).to("Larry")
>>
>>
>>
>> That's more aligned with the mocking syntax. I do think this is  
>> very much aligned with BDD. It provides a way to specify some  
>> general outcome (or observable behaviour) of an arbitrary chunk of  
>> code. I like it.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 17:36
>>
>> Message:
>> This syntax is definitely better, though it's still a bit awkward  
>> in the sense that it reads like we're saying that @tim should be  
>> responsible for changing his name, as opposed to "after this  
>> action, @tim's name should have been changed" (by whom, we don't  
>> know or care).
>>
>>
>>
>> It's the best we've got so far.
>>
>>
>>
>> Aslak - do you have any thoughts as to the BDD-ness of this? It  
>> seems to me that it's got a different structure from what we've  
>> been recommending (i.e. context goes in setup - here context is  
>> implied within specify), but it also seems very behaviour oriented  
>> (BOP?????).
>>
>>
>>
>> Thoughts?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Aslak Helles?y (aslak_hellesoy)
>> Date: 2006-11-15 17:10
>>
>> Message:
>> I like that syntax!
>>
>>
>>
>> However, the last example might not work so well with Rails.  
>> Consider this:
>>
>>
>>
>> @tim.should_change(:name).to("Larry") do
>>
>>   post 'update', {:id => @tim.id, :person => {:name => 'Larry'}}
>>
>> end
>>
>>
>>
>> It will probably update tim in the database, but it won't change  
>> the @tim reference. You'll have to reload tim. Perhaps this would  
>> work:
>>
>>
>>
>> @tim.should_change(:name).to("Larry") do
>>
>>   post 'update', {:id => @tim.id, :person => {:name => 'Larry'}}
>>
>>   @tim.reload
>>
>> end
>>
>>
>>
>> Yeah, that should work I think.
>>
>>
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 16:56
>>
>> Message:
>> What about this syntax:
>>
>>
>>
>>   # non-deterministic
>>
>>   User.should_change(:count) { ... }
>>
>>
>>
>>   # increment/decrement
>>
>>   User.should_change(:count).by(1) { ... }
>>
>>   User.should_change(:count).by(-1) { ... }
>>
>>
>>
>>   # verify that name changed and the new value should == "Larry"
>>
>>   @tim.should_change(:name).to("Larry") { ... }
>>
>>   # verify before and after
>>
>>   @tim.should_change(:name).from("Tim").to("Larry") { ... }
>>
>>
>>
>> I haven't really thought about if the implementation of these  
>> would work. I like that it doesn't use past tense and makes it  
>> feel more like setting up the mock expectations instead of  
>> verifying after-the-fact, which conceptually is more what this is  
>> doing.
>>
>>
>>
>> Thoughts?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 11:53
>>
>> Message:
>> I'd stay away from sugar. Sugar is only there to intercept  
>> messages that start with "should_", hence the implementation.
>>
>>
>>
>> I do see the dilemma re: getting a handle on the User rather than  
>> the value it returns for :count. Agreed that  
>> User.should_return_different_results_for(:count).after {
>>
>> block here } is clunky, but I think that's the right direction.  
>> Not sure of a better way to do it though.
>>
>>
>>
>> As for the plugin requiring a change for every release, that's  
>> only temporary. We do plan to address plugins and extensions  
>> because we want people to feel free to publish extensions.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 11:10
>>
>> Message:
>> Oh, and Brandon's example:
>>
>>
>>
>> User.count.should_be_changed_by(1) { User.create(...) }
>>
>>
>>
>> ..is, as far as I can tell, impossible to implement without
>>
>> modifying the Ruby interpreter.
>>
>> If User.count starts out at 5, 'should' is being called on
>>
>> an instance of Fixnum. There's no way to tell what value to
>>
>> compare it to, after the block returns.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-15 11:08
>>
>> Message:
>> That's precisely why I implemented it this way. I would much
>>
>> prefer to be able to write:
>>
>> User.count.should_be_changed..etc.etc..
>>
>> ..but without a way to get a handle on the "method that was
>>
>> called to return the object that should was called on", I
>>
>> couldn't think of another way.
>>
>>
>>
>> Also, re: sugar.rb
>>
>> Here's a line of code from trunk:
>>
>> return original_method_missing.bind(self).call(sym, *args,
>>
>> &block) unless sym.to_s =~ /^should_/
>>
>> As you can see, sugar assumes that the message being
>>
>> intercepted begins with 'should'. Because this patch
>>
>> requires a different behavior, I will still need to
>>
>> monkey-patch sugar.rb.
>>
>>
>>
>> I'm happy to do this as a plugin, it just looks like I would
>>
>> need to release a new version almost every time the trunk
>>
>> changed.
>>
>>
>>
>> The other way to implement this would be something like:
>>
>> User.should_return_different_results_for(:count).after {
>>
>> block here}
>>
>>
>>
>> ..but that seems clunky.  I like the way this version reads,
>>
>> myself.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 09:26
>>
>> Message:
>> Ok, looking into this a little, I see why it is implemented how it  
>> is.  The idea is that you want to check that the result of a  
>> method call is changed by a block, not necessarily the referenced  
>> object itself.  It happens to work if the referenced object is  
>> being modified, but what about when it's just being re-assigned?
>>
>>
>>
>> For example, this would work because :count is being incremented:
>>
>>
>>
>>   User.count.should_be_changed_by(1) { User.create(...) }
>>
>>
>>
>> But, this would fail:
>>
>>
>>
>>   @user.name.should_be_changed_to("Jim") { @user.name = "Jim" }
>>
>>
>>
>> should_be_changed_to is looking that the referenced variable  
>> changes, but in the block, I'm just reassigning it, so the  
>> original reference to name is not modified.
>>
>>
>>
>> Thoughts?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-15 09:06
>>
>> Message:
>> So, the main issue is just the implementation, and not the feature  
>> itself?  I agree with you about it being too non-deterministic.
>>
>>
>>
>> I like the examples that you gave, it feels more like original the  
>> assert_difference.
>>
>>
>>
>> I'd also like to see "should_be_changed_to(newval)"
>>
>>
>>
>> Maybe I'll get myself familiar with the rspec internals and  
>> actually contribute instead of standing on the sidelines. ;)
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 06:30
>>
>> Message:
>> Brandon - re: inclusion as a core feature:
>>
>>
>>
>> I think it is too non-deterministic as/is. In the example, one  
>> could override (accidentally, of course) User.create to delete  
>> something and the spec would pass.
>>
>>
>>
>> We *could* make it more deterministic:
>>
>>
>>
>> User.count.should_be_changed_by('+ 1') do {...}
>>
>>
>>
>> or
>>
>>
>>
>> User.count.should_be_changed_from(3).to(4) do {...}
>>
>>
>>
>> One thing that concerns me is that we lose the separation between  
>> context and outcome that is essential to BDD.
>>
>>
>>
>> Although it is very behavioral in nature. I'm on the fence.
>>
>>
>>
>> Thoughts?
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: David Chelimsky (dchelimsky)
>> Date: 2006-11-15 06:22
>>
>> Message:
>> Wilson - re: your question about plugins and sugar:
>>
>>
>>
>> sugar.rb now only intercepts the :should_ part of  
>> a :should_do_something message. What you want to extend is the  
>> Should class itself (in this case with a 'changed_by' method that  
>> takes a block), and the Not class if you want a negative counterpart.
>>
>>
>>
>> Be forewarned that we're not committing to the current structure  
>> at this point, so you'd be incurring some risk that a future  
>> version of RSpec might break your plugin because you're patching  
>> something internal. We will, however, at some point soon (within a  
>> month or two I think), publish (and thereby commit to) an entry  
>> point to extending expectations.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Brandon Keepers (brandon)
>> Date: 2006-11-14 20:00
>>
>> Message:
>> What is the reasoning for not including this as a core feature?   
>> This is a very helpful expectation.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Aslak Helles?y (aslak_hellesoy)
>> Date: 2006-11-06 13:11
>>
>> Message:
>> I don't think anyone has thought much about how a plugin mechanism  
>> will actually be implemented. We'll try as hard as we can to make  
>> something forward compatible and easy to use.
>>
>>
>>
>> Your patch here would be a good case to verify a plugin mechanism  
>> against.
>>
>>
>>
>> I actually implemented this very feature a couple of months ago  
>> (rel 0.5.14). We decided to remove the day after. The thread about  
>> it starts here:
>>
>>
>>
>> http://rubyforge.org/pipermail/rspec-devel/2006-July/000324.html
>>
>>
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Wilson Bilkovich (wilson)
>> Date: 2006-11-06 12:51
>>
>> Message:
>> Hrm. Are plugins going to be able to modify sugar.rb in a
>>
>> forward-compatible way?
>>
>> I'm not sure how to implement this without stepping in that
>>
>> early in the chain.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> Comment By: Aslak Helles?y (aslak_hellesoy)
>> Date: 2006-11-06 12:45
>>
>> Message:
>> I think this is the kind of stuff we'd encourage people to bundle  
>> as an RSpec plugin. We don't have a plugin mechanism yet, but  
>> we'll hopefully have one soon.
>>
>> --------------------------------------------------------------------- 
>> -
>>
>> You can respond by visiting:
>> http://rubyforge.org/tracker/? 
>> func=detail&atid=3151&aid=6417&group_id=797
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: This is a digitally signed message part
Url : http://rubyforge.org/pipermail/rspec-devel/attachments/20061116/a8bc4306/attachment-0001.bin 


More information about the rspec-devel mailing list