[rspec-devel] rspec_on_rails, speccing models, adding it helpers...

[Zach Moazeni]

Pat, I appreciate your time in forming your thoughts and putting  
together a well reasoned reply. Yet there are a few areas where I  
think tests like these are valuable.

(Please note, this is not the original poster, even if we share the  
same first name)

> Change 'describe' to 'class' and remove 'do' from the first line.
> Then remove the 'it_' from the next three lines.  At this point you
> have the exact implementation of the class.
>
> I don't know about you, but that bothers the hell out of me.

This doesn't actually bug me as much. My company sometimes writes  
software for some pretty dumb users, and I mean that with kindness.  
Imagine a form with the following fields:

- First name
- Last name
- Email

I don't know how many submissions I've seen where they've entered  
"Bob  " or "jacob at foo.com ". Notice the spaces at the end. So most  
often I write classes like:

class SomeObj < ActiveRecord::Base
   def first_name=(first_name)
     self[:first_name] = first_name.nil? ? nil : first_name.strip
   end

   ... (repeated for last_name and email)
end

And from writing this same code many many times, I've moved to.

class SomeObj < ActiveRecord::Base
   strips :first_name, :last_name, :email
end

Naturally my spec was

describe SomeObj do
   it "should set the first name to nil if nil" do
     obj = SomeObj.new(:first_name => nil)
     obj.first_name.should be_nil
   end

   it "should strip the first name if containing spaces " do
     obj = SomeObj.new(:first_name => "foo  ")
     obj.first_name.should == "foo"
   end

   ...(repeated for last name and email)
end

Obviously you could (and maybe should) test "obj[:first_name]" to be  
more specific.

So being a lazy developer, I changed this common occurring spec into

describe SomeObj do
   should_strip :first_name, :last_name, :email
end

This is an example where the implementation and the spec both appear  
clones, yet I feel they are valuable.

>  -helps you design your code well

Not going to argue this point, because I don't think the spec does  
squat for this goal.

> - provides executable examples of how to use code

Although the implementation of the helper "should_strip()" is a little  
hard to read from the code standpoint, it does create the individual  
tests. And the spec output is:

User
- should set the first name to nil if nil
- should strip the first name if containing spaces
- should set the last name to nil if nil
- should strip the last name if containing spaces
- should set the email to nil if nil
- should strip the email if containing spaces

So if the spec breaks I get feedback on what is broken. Which also  
feels pretty descriptive.

That doesn't explain _how_ it can break though, and I feel that ties  
into the next goal:

> leaves behind regression tests


It came to pass that another developer added this:

class SomeObj < ActiveRecord::Base
   strips :first_name, :last_name, :email

   def first_name=(first_name)
     self[:first_name] = first_name
     do_some_business_logic_on_setter_of_first_name(first_name)
   end
end

Which broke the behavior of the object with which the "should strip  
the first name if containing spaces" failed.

> If you make any change to the implementation then the specs will  
> fail...so they're brittle without providing any value.


I have a hard time grappling with this topic, as it's a very recurring  
theme in mock testing. But I guess I don't see

describe SomeModel do
   it_has_many :widgets, :destroy => :null, :class_name => "BaseWidget"
end

any more brittle than

class SomeModel do
   def self.foo(bar)
     AnotherClass.baz(bar)
   end
end

describe SomeModel do
   describe "#foo" do
     it "should delegate to AnotherClass#baz" do
        AnotherClass.stubs(:baz).with("something").returns("something  
different")
        SomeModel.foo("something").should == "something different"
     end
   end
end

Yet I find a lot of value in that test (given adequate reasons why  
it's designed that way).

I think this reasoning still applies to the model relationships. I  
agree there isn't a lot of bang for the buck, but I see there is  
value. I like the fact it stresses the relationship at a basic level.

So to bring the argument full circle:

> The concrete benefits of object-level specification are, in my mind,  
> that it
>  - helps you design your code well

Not disagreeing

>  - leaves behind regression tests

- The scenario above (i.e. adding a method that collides with rails- 
added behavior)
- Stressing the basic add / remove functionality as you develop more  
complex validation rules on that collection. (e.g. Can't have more  
than x, Can't have a member in the set of y)
- Stressing foreign key constraints from bubbled exceptions from the  
database (though I think our company is in the minority on that one)

>  - provides executable examples of how to use code


- Declarative error messages (from the spec titles). An example spec  
doc from our existing project using our version of model behaviors:

User relationships
- should have many descriptions
- should destroy descriptions when deleted
- should have many photos
- should destroy photos when deleted
- should have many projects

A couple final notes:
- I don't disagree that some of these scenarios could be tested at a  
higher level, or if something changes, higher level tests will catch  
them. It's just been my experience that when leaning on higher level  
tests too much, too many things slip through. Or the verbosity of  
individual tests becomes a bitch.
- Our version of "should_have_many" "should_belong_to", etc, touches  
the database, and doesn't just check manipulated methods on the model.  
I find that much more valuable.
- I think these helpers needs to remain with basic functionality  
(Defining the relationships and effects of deletion), any more needs  
to be in custom specs.
- In a perfect world, I would see Rails Core, and other plugin  
developers to write such declarative behavior helpers along with their  
declarative code, but I don't think that's at all practical.
- While I'm not totally sold on the "Double-entry bookkeeping"  
argument with regards to testing, I feel there is a bit of that  
creeping in. For good or worse.
- Not intending this reply to be a straw man, but can't articulate  
what's in my head without bringing another example into the mix. Sorry  
if it confuses more than helps.

-Zach