assigning to model properties for mocking

[Calvin Spealman]

I'm trying to build a test suite in my project and I'm making heavy
use of mocks to write them cleanly. Testing has reached a point where
I need to mock the fkey property of a model instance, but I'd really
like to use this as a mock. Of course, I can't assign the mock here
because it isn't an instance of the correct (or any) Model subclass.

Would a patch be accepted (which I have ready) to remove the TypeError
that comes from trying to assign to the property on the class, so that
I can patch it (via the mock library patch function) with a Mock
instance, for my test?

[Malcolm Tredinnick]

On Tue, 2008-10-21 at 18:55 -0700, Calvin Spealman wrote:
> I'm trying to build a test suite in my project and I'm making heavy
> use of mocks to write them cleanly. Testing has reached a point where
> I need to mock the fkey property of a model instance, but I'd really
> like to use this as a mock. Of course, I can't assign the mock here
> because it isn't an instance of the correct (or any) Model subclass.
>
> Would a patch be accepted (which I have ready) to remove the TypeError

You mean the ValueError, right? Raised from various __set__ methods in
django/db/models/fields/related.py?

> that comes from trying to assign to the property on the class, so that
> I can patch it (via the mock library patch function) with a Mock
> instance, for my test?

So this proposal falls into a couple of common traps that make it hard
to assess. I think more information is needed. You've kind of leapt from
zero to "proposal" without covering a number of intermediate steps.

Firstly, that TypeError is there for a reason (see #6886): to prevent
people from assigning the incorrect type of object to that foreign key
attribute. It's deliberate code. Simply removing it puts us back where
we were, so you need to address why removing the safety net is worth it
just for this particular side-alley (and I use that term
deliberately ... testing that doesn't use normal code-paths always runs
the risk of having bugs in the testing approach). Changes that only
benefit testing at the cost of real-world code suggest that the testing
approach might have problems and an alternative direction is needed.

Secondly, you haven't really described why this change is necessary and
can't be solved in another way. The presence of that error indicates
that you aren't meant to assign incorrect types there. So you're really
trying to tease apart the interface guarantees at a point where they
aren't intended to be modifiable as part of the public API. Why can't
you put your mocks one level down: make a subclass of the proper type
that mocks out the methods on the real related classes? There may be
some technically insurmountable problem that we need to address, but you
haven't explained what it is.

I was trying to work out over lunch why I felt this wasn't a nice
solution and I think it probably has a bit of a code smell to it:
putting a mock object in the place you're suggesting. It comes back to
trying to inject something into a non-public part of the interface. Can
you give a short code sample showing what you're trying to do? You can
assume that Django's relation fields work (Django's own tests already
test that so that you don't have to). And since part of the very nature
of being a "model A with a relation to model B" is that one of A's
attributes points to a model B, trying to split the atom finer than the
A-B couplet there is making your tests depend on something that could
change without warning.

I'm completely in favour of allowing different testing methodologies and
approaches, since Django's own testing approach is, as I've mentioned
elsewhere, a little schizophrenic in its goals and this can sometimes
lead to difficulties. I'm also generally a pretty big fan of isolation
of interactions using mock objects where possible. But only in so far as
it integrates smoothly -- and, particularly, non-detrimentally -- with
normal, non-testing code and not taking things to extremes (dependencies
exist, sometimes, in ways that shouldn't be broken in two). Adding stuff
to regular execution paths that exist *only* to support testing is
something I'm generally pretty down on. Testing should flow naturally
from emulating normal usage, not require special paths.

In summary, I think there are at least two things I'd like to see here:
(1) why can't this be done another way, in particular by making a mock
class that is a subclass of model so that it passes the instance check
(a technical reason; "that's not how it's done" isn't a reason) and (2)
why is this such a strong requirement as to justify removing the
existing safety net that was put in place only a few months ago.

Those two are really related, but it really comes down to explaining
what the real problem is that you're trying to solve. The one sentence
summary you gave in your post is a little short on details of the
approach.

Regards,
Malcolm

[Calvin Spealman]

Firstly, you misunderstand the fundamentals of my proposal. I did not ever say that the check should be disabled and I know that is obviously a very silly idea. But I do notice I wasn't clear enough in my actual intent. I need to patch the descriptor, so that I can disable this in my tests *only*, no where else. Unfortunately, this kind of patching is currently not allowed by Django because jango.db.models.fields.related.ReverseSingleRelatedObjectDescriptor and its friend explicitly raise AttributeError on calls to __get__ which are not given an instance. This is anti-intuitive and does not match Python expectations. For example,

    class Foo(object):

       @property

       def x(self):

              return 1

    print Foo.x

---> <property object at 0x00000000>

Yet the model classes do not allow the same access to its descriptors, which breaks patching. When patching, the original attribute of an object must be saved before its replaced with a mock, so that it can be restored after the test. This is where the current behavior breaks my ability to cleanly test my model's methods.

So that is my simple proposal: removing that AttributeError.

Interestingly, while not allowing code to access the descriptor on the class to get it, there is no such restriction on setting it. So I can replace it with a mock, but cannot restore it afterwards.

--
Read my blog! I depend on your acceptance of my opinion! I am interesting!
http://techblog.ironfroggy.com/
Follow me if you're into that sort of thing: http://www.twitter.com/ironfroggy

[Karen Tracey]

On Thu, Oct 23, 2008 at 5:49 AM, Calvin Spealman <ironf...@gmail.com> wrote:

...

But I do notice I wasn't clear enough in my actual intent. I need to patch the descriptor, so that I can disable this in my tests *only*, no where else. Unfortunately, this kind of patching is currently not allowed by Django because jango.db.models.fields.related.ReverseSingleRelatedObjectDescriptor and its friend explicitly raise AttributeError on calls to __get__ which are not given an instance. This is anti-intuitive and does not match Python expectations. For example,

    class Foo(object):

       @property

       def x(self):

              return 1

    print Foo.x

---> <property object at 0x00000000>

Yet the model classes do not allow the same access to its descriptors, which breaks patching. When patching, the original attribute of an object must be saved before its replaced with a mock, so that it can be restored after the test. This is where the current behavior breaks my ability to cleanly test my model's methods.

So that is my simple proposal: removing that AttributeError.

Interestingly, while not allowing code to access the descriptor on the class to get it, there is no such restriction on setting it. So I can replace it with a mock, but cannot restore it afterwards.

Perhaps posting the patch you mention in your first email somewhere like dpaste.com so people can see what exactly you'd like to do would help.  As it is, I'm still not understanding.  I believe the specific method you are referring to is this one:

http://code.djangoproject.com/browser/django/tags/releases/1.0/django/db/models/fields/related.py#L223

But I don't see how you can just remove the AttributeError that is raised if instance is None, since the subsequent code needs to use the value of instance.

I'm also not following how __set__ (immediately following __get__ in the code) does not have the same restriction as __get__, since it has the exact same 'if instance is None: raise AttributeError' check at the beginning of the method? 

Karen

[Luke Plant]

On Thursday 23 October 2008 13:47:09 Karen Tracey wrote:

> Perhaps posting the patch you mention in your first email somewhere
> like dpaste.com so people can see what exactly you'd like to do
> would help. As it is, I'm still not understanding. I believe the
> specific method you are referring to is this one:
>
> http://code.djangoproject.com/browser/django/tags/releases/1.0/djan
>go/db/models/fields/related.py#L223
>
> But I don't see how you can just remove the AttributeError that is
> raised if instance is None, since the subsequent code needs to use
> the value of instance.

Calvin should have said that instead of raising an AttributeError, you
just 'return self' in the descriptor __get__ methods. It will then
work as he wants it to. (Not sure how it works with __set__).

Another advantage of this method is that the normal exploratory
programming you can do at an interactive prompt works nicely:

e.g. in ipython:

In [13]:from myapp.models import MyModel
In [14]:MyModel.some_field.__class__
Out[14]:<class 'django.db.models.fields.related.ReverseSingleRelatedObjectDescriptor'>

With current Django src, you get an AttributeError with the above.

I'm definitely +1 this change, I've come across this before when doing
this type of thing, and found it annoying, and definitely unpythonic,
as I've not come across any other python code that behaves this way.

Luke

--
"Doubt: In the battle between you and the world, bet on the world."
(despair.com)

Luke Plant || http://lukeplant.me.uk/

[Malcolm Tredinnick]

On Thu, 2008-10-23 at 05:49 -0400, Calvin Spealman wrote:
> On Wed, Oct 22, 2008 at 4:14 AM, Malcolm Tredinnick
> <mal...@pointy-stick.com> wrote:
>
>
> On Tue, 2008-10-21 at 18:55 -0700, Calvin Spealman wrote:
> > I'm trying to build a test suite in my project and I'm
> making heavy
> > use of mocks to write them cleanly. Testing has reached a
> point where
> > I need to mock the fkey property of a model instance, but
> I'd really
> > like to use this as a mock. Of course, I can't assign the
> mock here
> > because it isn't an instance of the correct (or any) Model
> subclass.
> >
> > Would a patch be accepted (which I have ready) to remove the
> TypeError
>

[...]

> Firstly, you misunderstand the fundamentals of my proposal. I did not
> ever say that the check should be disabled

"A patch to remove the TypeError" sounds an awful lot like proposing it
be disabled.

> and I know that is obviously a very silly idea. But I do notice I
> wasn't clear enough in my actual intent. I need to patch the
> descriptor, so that I can disable this in my tests *only*, no where
> else. Unfortunately, this kind of patching is currently not allowed by
> Django because
> jango.db.models.fields.related.ReverseSingleRelatedObjectDescriptor
> and its friend explicitly raise AttributeError on calls to __get__
> which are not given an instance.

Aah... okay. This is an entirely different thing. Jacob has already made
a reasonable case in some ticket about doing this (for __get__ only),
since it would help introspection, as Luke mentions. Sounds like that's
going to happen.

Search for the ticket on help() not working for things with ForeignKeys
(which is caused by exactly this introspective behaviour being blocked).

Regards,
Malcolm

[Calvin Spealman]

Thanks for all the input and response everyone.