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

Pat Maddox pergesu at gmail.com
Wed Apr 2 20:11:52 EDT 2008


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


More information about the rspec-devel mailing list