[rspec-devel] Fwd: [ rspec-Bugs-6905 ] Mocking rails associations where the association is validated

Pat Maddox pergesu at gmail.com
Mon Nov 27 18:34:19 EST 2006


On 11/27/06, David Chelimsky <dchelimsky at gmail.com> wrote:
> As usual, I said nothing..... (accidentally hit send instead of
> discard). Anyhow - comments below....
>
> On 11/27/06, David Chelimsky <dchelimsky at gmail.com> wrote:
> > On 11/27/06, aslak hellesoy <aslak.hellesoy at gmail.com> wrote:
> > > On 11/27/06, Pat Maddox <pergesu at gmail.com> wrote:
> > > > On 11/27/06, David Chelimsky <dchelimsky at gmail.com> wrote:
> > > > > On 11/27/06, Pat Maddox <pergesu at gmail.com> wrote:
> > > > > > Another question :)
> > > > > >
> > > > > > When I first wrote the add_person method, my spec looked like this:
> > > > > >
> > > > > >   specify "should accept a new person" do
> > > > > >     @user.people.size.should_be 0
> > > > > >     @user.add_person Person.new
> > > > > >     @user.people.size.should_be 1
> > > > > >   end
> > > > > >
> > > > > > That works, of course, and lets me infer that I have the new user.
> > > > > > However I think it's ugly that I'm accessing people to verify
> > > > > > something about the user...
> > > > > >
> > > > > > So then I decided to change it to this:
> > > > > >
> > > > > >   specify "should accept a new person" do
> > > > > >     mock_person = mock("person")
> > > > > >     mock_people = mock("people")
> > > > > >     @user.should_receive(:people).and_return(mock_people)
> > > > > >     mock_people.should_receive(:<<).with(mock_person)
> > > > > >     @user.add_person mock_person
> > > > > >   end
> > > > > >
> > > > > > That feels like the more RSpecy way to do this, despite being longer.
> > > > > > In both cases they dig into people (either checking the size or
> > > > > > checking that it calls <<), but in the latter case it verifies
> > > > > > behavior, rather than inferring it from state.  Does that sound right?
> > > > >
> > > > > This one is tricky. There are multiple perspectives on how much
> > > > > mocking you should do in model specs. One view is that model specs are
> > > > > really small integration specs (model class + database). Another view
> > > > > is that you should do what you did above, mocking out the other
> > > > > players where possible.
> > > > >
> > > > > I'm leaning towards the first view - that model specs are like little
> > > > > integration specs. My thinking is that because the way AR is designed,
> > > > > the things you end up mocking are often internal to AR, which is a
> > > > > huge mocking no-no (mock YOUR code, not other people's).
> > > >
> > > > Weren't we mocking other people's code in the controller specs as well?
> > > > ARClass.should_receive(:find).with(1).and_return(some_mock_object)
> > > > ARClass.should_receive(:new).with(:name => "name").and_return(other_mock_object)
>
> I think one *could* argue that mocking :find is closer to mocking your
> own code (as it is a method that appears on *your* AR subclass, even
> though it is implemented above) than mocking the collection that AR
> makes available when you ask it for a collection property. As I said
> before, this is very gray area, thanks to being cornered into
> inheritance rather than composition. That sounds whiny, I know. What
> can I say? When something is SO close to being really great, you're
> more likely to notice its flaws than its lesser counterparts.

I guess that's where I'm confused...the collection is defined on
*your* AR subclass as well.  Would you mind clarifying the difference
you see between MyClass.find and my_instance.items?  The latter case
is even more custom because it's not automatically implemented higher
in the heirarchy, but rather is defined only after you call "has_many
:items"


> > > > I'm not really sure what the difference is between mocking that out
> > > > and mocking players.  At first I thought that it's because the find
> > > > and new calls appear in my code, but so does players <<.
> > > >
> > > >
> > > > > In this case you could use this to make it feel more RSpecy without
> > > > > mocking anything:
> > > > >
> > > > > specify "should accept a new person" do
> > > > >   lambda do
> > > > >     @user.add_person
> > > > >   end.should_change(@user.people.size).by(1)
> > > > > end
> > > > >
> > > > > I'm not a huge fan of going after @user.people.size here, but I think
> > > > > that @user.number_of_people might be overkill.
> > > >
> > > > After thinking about this some more, I think I like the other version
> > > > with lots of mocks.  To me, it boils down to what I think is a
> > > > revelation I had last night...using the specs do define behavior,
> > > > rather than infer it from state.
> > > >
> > > > The one problem I see with my example was what if the User object
> > > > doesn't define a players method?  Then the spec passes fine, but the
> > > > code blows up of course.  This would be easily solved if you ensure
> > > > that partial mocks can only mock methods that the object already has.
> > > >
> > > > for example, I've got a user object that has players in this case...when I do
> > > > user.should_receive(:players)
> > > > then I think the should_receive call out to call
> > > > user.respond_to?(players) before mocking the method.  That seems
> > > > reasonable to me for partial mocks - we're just substituting in some
> > > > code for behavior that should already exist, because the existing code
> > > > is too complex or expensive and doesn't need to be executed at the
> > > > moment.  At the very least, perhaps it could generate a warning such
> > > > as "WARNING - 'people' not defined on user object."  WDYT?
> > > >
> > >
> > > That would be possible to bake into RSpec, but it would violate one of
> > > the main principles of BDD. BDD is (among other things) a technique to
> > > *discover* the interfaces of neighbouring objects N while speccing a
> > > class C. And while you're speccing C, N might not even exist.
> > >
> > > The WARNING idea is interesting though.
>
> RFE [#5064] (Let mock() take a class argument) seems to be addressing
> the similar issue.
>
> If we do something like this, I'd like to see it be modal. Under
> normal spec runs it has no effect, but w/ a command line switch you
> can get a report of what's being mocked that doesn't exist yet. I also
> added a comment describing a vague vision (I don't really know how
> this would work) in which if you use a class argument you could get
> the specs to run in an integration mode in which real instances of the
> class get substituted for the mocks, thereby allowing the specs to
> serve as both isolation and integration specs. This is not an original
> idea - Dan introduced something similar in JBehave, and someone on one
> of these lists wrote about it as well some time ago. But it's worth
> investigating, I think.

I agree that it would be better to use a command line switch to report
warnings.  One problem I initially saw is when some methods are
handled by method_missing, such as find_by_login.  We wouldn't want
warnings polluting the spec output when the method call is in fact
valid, albeit not defined on the object itself.

I remember reading about the integration mode, and I think it would be
very good.  You can use the current way to drive your design well, and
then use integration mode to ensure that the entire stack works
properly.  This would serve a different purpose from the Rails
integration tests, which imo are to prove workflow through your
application.

Pat


More information about the rspec-devel mailing list