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

noreply at rubyforge.org noreply at rubyforge.org
Thu Nov 16 23:23:07 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: expectation module
Group: None
Status: Closed
Resolution: None
Priority: 3
Submitted By: Wilson Bilkovich (wilson)
Assigned to: David Chelimsky (dchelimsky)
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-16 23:23

Message:
Yeah. I like the polymorphic version better, for sure. Nice
refactoring.

The symbol thing came to me while I was getting coffee at
Starbucks. I don't need it badly enough to fight for it.

One tiny, tiny nitpick.. I used the:

"blah blah #@instancevar"  syntax, and you used "blah blah
#{@instancevar}".  The two are now mixed in change.rb. 
Should probably pick one and stick with it in the whole file.

Good times. :)

----------------------------------------------------------------------

Comment By: David Chelimsky (dchelimsky)
Date: 2006-11-16 21:49

Message:
Thank you so much for this patch! I applied it to trunk (rev 1115) with the following changes/exceptions:

Added a couple of specs to get back to 100% coverage.
Added expected messages to the specs.
Refactored a few things for clarity (for me).
Added a NotChange class.
Did not include :symbol.should_change(receiver, :message).

re: NotChange - that actually inspired me to add a NotHave class as well. Down w/ procedural conditionals! Up with polymorphic handling of conditionals!

re: :symbol.should_change - I'm not really comfortable with that syntactically because it's difficult to know what the symbol is related to. If you feel strongly about the symbol syntax, please raise another RFE and we'll chat about it (I think the thread on this one is long enough already!)

Cheers,
David

ps - working on docs now - will commit shortly

----------------------------------------------------------------------

Comment By: Wilson Bilkovich (wilson)
Date: 2006-11-16 16:48

Message:
Actually, that works. Maybe it needs a spec to prove it. Heh.

lambda{}.should_not_change(@blah, :foo).to(x) doesn't make
sense, because the initialize method of 'should not change'
will already have failed the expectation by then.

However, if you really wanted to be wordy, you actually
could write:  lambda{}.should_not_change(@blah, :foo).from(x)

Unless I screwed something up, that will work.  If the
initial value wasn't x, it will fail. It will pass if the
initial value was x, and the final value is also x.

----------------------------------------------------------------------

Comment By: Brandon Keepers (brandon)
Date: 2006-11-16 16:35

Message:
Good work on the patch.  Very clean.

One thought with using the Change class for negation. should_not_change now returns the Change class, which may lead people to believe they can do this:

  context "should_not_change.to" do
    specify "should pass when attribute is changed to something besides value" do
      lambda do
        lambda { @tim.name = "Steve" }.should_not_change(@tim, :name).to("Larry")
      end.should_pass
    end
  end

My guess is that it is nearly impossible to implement that since you can't determine where to call the block.  So this should probably be the case:

  context "should_not_change" do
    specify "should return nil" do
      lambda { }.should_not_change(@tim, :name).should_be_nil
    end
  end


----------------------------------------------------------------------

Comment By: Wilson Bilkovich (wilson)
Date: 2006-11-16 16:07

Message:
By the way.. despite the fact that I like the code in the
new patch.. I still feel that the original syntax reads
better. MUCH better, because I think lambdas are ugly.

I am worried that we are trading off usability and
readability here due to implementation issues.

----------------------------------------------------------------------

Comment By: Wilson Bilkovich (wilson)
Date: 2006-11-16 15:55

Message:
OK. How about this? (patch attached)

I also added a sassy set of behavior when you invoke
should_change on a symbol or string, instead of a proc.

----------------------------------------------------------------------

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