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

noreply at rubyforge.org noreply at rubyforge.org
Wed Nov 15 18:48:26 EST 2006


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: 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