Recursion error in TestCase class decorated by override_settings when overriding method on parent class and calling parent method

[Jim Dalton]

A bit hard to explain but a fairly straightforward bug. Basically, if you have a sub class of TestCase and then a sub class of that sub class that's decorated by @override_settings at the class level, and you override a method and then call the parent method, you get a recursion error.

Kind of easier to demonstrate in code:

from django.test import TestCase
from django.test.utils import override_settings

class FooTests(TestCase):
  def setUp(self):
    # do some stuff
    pass

@override_settings(FOO='bar')
class SubFooTests(TestCase):
  def setUp(self):
    # recursion error!
    super(SubFooTests, self).setUp()

I have a patch with a regression test that formally demonstrates this behavior but have not investigated it yet. It's possible it's related to #16224, but I don't really know.

[Carl Meyer]

This is because of the dynamic-subclass-creation magic that the decorator does. Basically, the problem is that there doesn't seem to be any way to write a Python class decorator that doesn't break weirdly in some cases (c.f. the recent threads about decorating class-based views on the -developers mailing list).

I think in this case the decorator probably ought to just modify the given class in-place (monkeypatch it) rather than creating a dynamic subclass. Because I think the subclassing/super case you've demonstrated is much more common usage than someone trying to do this:

class Foo(TestCase):
    ...

Bar = override_settings(SOMETHING="value")(Foo)

Which is the case where monkeypatching fails (because both Foo and Bar end up modified).

I guess it's possible someone might try that, but we could just tell them to do this instead:

class Foo(TestCase):
    ...

@override_settings(SOMETHING="value")
class Bar(Foo):
    pass

I am not aware of any perfect option here; I think in this case monkeypatching would fail less often than what we do currently.

[Jim Dalton]

Thanks Carl. I think you are correct, monkey patching in this case is cleaner and less prone to weirdness. I moved the the tests I had created to settings_tests and worked the implementation. settings_tests all pass.

I filed the patch as a pull request, hopefully I'm doing this right:

​https://github.com/django/django/pull/60

[Carl Meyer]

Replying to jsdalton:

Thanks Carl. I think you are correct, monkey patching in this case is cleaner and less prone to weirdness. I moved the the tests I had created to settings_tests and worked the implementation. settings_tests all pass.

I filed the patch as a pull request, hopefully I'm doing this right:

​https://github.com/django/django/pull/60

Thanks for the patch! The pull request part is right, but something about the patch isn't; when I apply the patch and run the full test suite I get hundreds of infinite recursion errors. I haven't tracked down the cause.

[Jim Dalton]

Ah, dang, thanks. Looks like there were some issues in multiple inheritance scenarios, particularly where @override_settings was being applied to more than one class in the inheritance chain (e.g. the test class staticfiles_tests.TestCollectionNonLocalStorage). Shame on me for not running the full test suite with my first patch.

Anyhow, the resolution is pretty straightforward: assigning the original pre-setup/post-teardown methods to local variables instead of class attributes, so they don't end up stepping on each others toes.

I've updated my topic branch with the new commits, which is now reflected in the pull request. Full test suite now passes.

[Carl Meyer]

This is a release blocker, as its a major issue with a new feature in 1.4.

[Carl Meyer]

In [16942]:

(The changeset message doesn't reference this ticket)