[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