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

Tim Haines tmhaines at gmail.com
Wed Apr 2 23:01:40 EDT 2008 

I totally agree.  This sort of comment/guidance/opinion should be findable
from (or included on) the rspec.info page on models..

Pat - thanks a bunch for taking the time!

Tim.

On 03/04/2008, David Chelimsky <dchelimsky at gmail.com> wrote:
>
> On Wed, Apr 2, 2008 at 10:40 PM, Zach Dennis <zach.dennis at gmail.com>
> wrote:
> > Pat,
> >
> >  That was a wonderfully thought out reply and at first read your
> >  reasoning makes a lot of sense.
>
>
> +1
>
>
> > Thank you for taking the time to write
> >  it.
>
>
> +1 more
>
> Cheers,
>
> David
>
>
> > 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
> >  _______________________________________________
> >
> >
> > rspec-devel mailing list
> >  rspec-devel at rubyforge.org
> >  http://rubyforge.org/mailman/listinfo/rspec-devel
> >
> _______________________________________________
> rspec-devel mailing list
> rspec-devel at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://rubyforge.org/pipermail/rspec-devel/attachments/20080403/4e695e25/attachment-0001.html

More information about the rspec-devel mailing list