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

[Zach Dennis]

Pat,

That was a wonderfully thought out reply and at first read your
reasoning makes a lot of sense. Thank you for taking the time to write
it. I am going to let it sink in over the next few days,

Zach


On Wed, Apr 2, 2008 at 8:11 PM, Pat Maddox <pergesu at gmail.com> wrote:
>
> On Tue, Apr 1, 2008 at 4:20 PM, Zach Dennis <zach.dennis at gmail.com> wrote:
>  >
>  > On Tue, Apr 1, 2008 at 6:38 PM, Pat Maddox <pergesu at gmail.com> wrote:
>  >  > On Tue, Apr 1, 2008 at 3:09 PM, David Chelimsky <dchelimsky at gmail.com> wrote:
>  >  >  >  >  Example:
>  >  >  >  >
>  >  >  >  >  describe SomeModel do
>  >  >  >  >   it_has_many :widgets, :destroy => :null, :class_name => "BaseWidget"
>  >  >  >  >   it_has_one :fuzzbucket
>  >  >  >  >   it_belongs_to :another_model
>  >  >  >  >  end
>  >  >  >
>  >  >  >  I see more and more structures appearing like this. I have very mixed
>  >  >  >  feelings about them. This is about structure, not behaviour. Even if
>  >  >  >  the underlying code is actually approaching this in a more behavioural
>  >  >  >  way, it's still expressing structure at the high level.
>  >  >
>  >  >  I don't have mixed feelings about this.  I think this type of spec is
>  >  >  terrible.  It completely duplicates the implementation.  It's not even
>  >  >  testing anything.
>  >  >
>  >  >  This is not a value judgment against you though, Zach.  I think when
>  >  >  people do stuff like this they genuinely have good intentions.  It's
>  >  >  just that it seems to be quite difficult to test highly declarative
>  >  >  stuff like AR associations.
>  >  >
>  >  >  Now that I've given my rather harsh opinion, I have to get back to
>  >  >  work :)  I'll try follow up later with something more helpful like
>  >  >  thoughts on how to write better specs.
>  >  >
>  >
>  >  I don't like the fact that it tests the structure of the association
>  >  (as opposed to testing the behavior), but I do like that it tests the
>  >  conceptual relationship between models. I find value in this. Even
>  >  though it is declarative it is very clear and meaningful to the next
>  >  guy looking at the code, and if someone changes something incidentally
>  >  they are quickly pointed to the fact that they broke a conceptual
>  >  relationship between two models.
>  >
>  >  Please do respond with more thoughts, as this is a topic I'd like to
>  >  get hammered out as it will provide value to every developer on this
>  >  list,
>
>  I've put some more thought into this and have a bit of time to reply.
>  So here goes.
>
>  The first thing I'm going to do is demonstrate why I feel this is a
>  bad spec.  Please understand that this is not a criticism of anyone in
>  particular.  I'm merely using this as an example of specs that don't
>  add any value.
>
>  Take a look at the spec again:
>
>
>  describe SomeModel do
>   it_has_many :widgets, :destroy => :null, :class_name => "BaseWidget"
>   it_has_one :fuzzbucket
>   it_belongs_to :another_model
>  end
>
>  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.
>
>  The concrete benefits of object-level specification are, in my mind, that it
>   - helps you design your code well
>   - leaves behind regression tests
>   - provides executable examples of how to use code
>
>  Often when we write specs we have to balance those goals...for
>  example, one major criticism of using mocks is that tests that use
>  mocks don't act as effective regression tests.  That's a valid
>  criticism in certain contexts, but you'll find that most people who
>  make such criticisms are being myopic - they either don't understand
>  or don't share the other goals, and so write the technique off all
>  together.
>
>  Besides the fact that the given association helpers duplicate the
>  implementation to an i-t-underscore, what else is wrong with them?  I
>  would argue that they provide 0 value in all three categories.
>
>  They don't help drive the design.  You either need widgets or you
>  don't.  If you decide you do, you add a declaration to the
>  implementation.  Done.  There was never any question about how to
>  design it.  Rails made that decision for you.
>
>  They have no value as regression tests.  How likely is it that any of
>  that code will break?  Not likely at all.  It's not like anyone's ever
>  going to go in there and change the behavior, because there is no
>  behavior, other than that which is abstracted away by Rails (thus
>  already thoroughly tested).  If you make any change to the
>  implementation then the specs will fail...so they're brittle without
>  providing any value.
>
>  The lack of documentation value should be obvious.  The specs don't
>  show you how to use the objects together.  You have to know how Rails
>  associations work.  And you get absolutely no information from the
>  spec that you don't get from the implementation itself.
>
>  Hopefully that clarifies why I don't feel that specs like these are
>  valuable.  I also hope that this helps others develop heuristics for
>  when to write/delete/ignore tests.
>
>  With that out of the way, how would I specify this SomeModel class?
>  Well, if the class is as given and there's no business logic, then I
>  would only write a couple specs for SomeModel directly.  These would
>  be specs for the widgets association.
>
>  describe SomeModel, "widgets" do
>   before(:each) do
>     @model = SomeModel.create!
>     BaseWidget.create! :some_model_id => @model.id
>   end
>
>   it "should find BaseWidgets" do
>     @model.should have(1).widget
>   end
>
>   it "should nullify keys when deleting the widgets" do
>     lambda { @model.widgets.destroy_all }.should_not change(BaseWidget, :count)
>     @model.should have(0).widgets
>   end
>  end
>
>  The reason I would do this is because there's a greater chance that
>  some of this stuff could break.  None of the other associations will.
>
>  Looking back at this, I'm not sure that I would write the second
>  example.  The reason I might not write it is that I don't think I have
>  enough information.  The foreign keys get nulled out when I call
>  #destroy_all, but that's something that I know from the has_many
>  declaration, and that I know works.  My real question at this point is
>  WHY I want the keys nulled out instead of just deleting the records.
>  Do I have some requirement in the system that these associations
>  should be broken, but the child records should stick around?  If so, I
>  should have another spec that demonstrates that behavior.
>
>  These associations *should* be tested, but they should be tested
>  indirectly from _somewhere else_.  They're not important enough to
>  deserve tests at such a low granularity.  They should be tested via an
>  acceptance test where a view iterates through them, or some test which
>  calls a DB aggregate method on the proxy.  They don't have any
>  interesting behavior on their own, and we are concerned primarily with
>  behavior.
>
>  Does that help?
>
>  Pat
>



-- 
Zach Dennis
http://www.continuousthinking.com