[rspec-devel] [ rspec-Patches-6417 ] [PATCH] Support @object.some_method_should_be_changed_by {block}

Brandon Keepers bkeepers at gmail.com
Thu Nov 16 01:52:31 EST 2006


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

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

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



More information about the rspec-devel mailing list