Opened 3 years ago

Closed 3 years ago

Last modified 3 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: jsdalton Owned by: nobody
Component: Testing framework Version: master
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 jsdalton 3 years ago.

Download all attachments as: .zip

Change History (8)

Changed 3 years ago by jsdalton

comment:1 Changed 3 years ago by jsdalton

  • Cc jim.dalton@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

comment:2 Changed 3 years ago by carljm

  • Triage Stage changed from Unreviewed to Accepted

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 follow-up: Changed 3 years ago by jsdalton

  • 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

comment:4 in reply to: ↑ 3 Changed 3 years ago by carljm

  • 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 Changed 3 years ago by jsdalton

  • 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 Changed 3 years ago by carljm

  • Severity changed from Normal to Release blocker

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

comment:7 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from new to closed

In [16942]:

(The changeset message doesn't reference this ticket)

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