Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#17011 closed Bug (fixed)

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

Reported by: Jim Dalton Owned by: nobody
Component: Testing framework Version: dev
Severity: Release blocker Keywords:
Cc: jim.dalton@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

override_settings_max_recursion_error.diff (1.5 KB ) - added by Jim Dalton 12 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Jim Dalton, 12 years ago

Cc: jim.dalton@… added
Has patch: set
Patch needs improvement: set

comment:2 by Carl Meyer, 12 years ago

Triage Stage: UnreviewedAccepted

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.

comment:3 by Jim Dalton, 12 years ago

Patch needs improvement: unset

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

in reply to:  3 comment:4 by Carl Meyer, 12 years ago

Patch needs improvement: set

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.

comment:5 by Jim Dalton, 12 years ago

Patch needs improvement: unset

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.

comment:6 by Carl Meyer, 12 years ago

Severity: NormalRelease blocker

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

comment:7 by Carl Meyer, 12 years ago

Resolution: fixed
Status: newclosed

In [16942]:

(The changeset message doesn't reference this ticket)

Note: See TracTickets for help on using tickets.
Back to Top