[Django] #31944: Use addCleanup() to register TestContextDecorator cleanups
Django
------------------------------------------------+------------------------
Reporter: François Freitag | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
`unittest.TestCase` offers an
[https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
addCleanup()] function that is guaranteed to run even if
`unittest.TestCase.setUp()` fails.
In #29024, the cleanup was implemented by
[https://github.com/django/django/blob/0b0658111cba538b91072b9a133fd5545f3f46d1/django/test/utils.py#L357-L361
catching Exception, calling TestContextDecorator.disable() and re-
raising]. Using `addCleanup()` allows a small simplification. More
importantly, it prevents an ordering issue with the cleanups that can
cause tests to bleed when subclasses of Django `TestCase` relying of
`addCleanup()` have their cleanups occurring after the
`TestContextDecorator.disable()`. To illustrate:
{{{#!python
class ChangeCSRFCookieName(TestContextDecorator):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.stack = contextlib.ExitStack()
def enable(self):
cm =
override_settings(CSRF_COOKIE_NAME="cross_site_request_forgery_token")
self.stack.enter_context(cm)
def disable(self):
self.stack.close()
class ChangeSettingsTestCase(SimpleTestCase):
def setUp(self):
super().setUp()
settings_cm = self.settings(CACHES={})
settings_cm.enable()
self.addCleanup(settings_cm.disable)
@ChangeCSRFCookieName()
class ChangeCSRFCookieNameTest(ChangeSettingsTestCase):
def test_initial(self):
pass
class LeakTest(SimpleTestCase):
def test_csrf_cookie_name_has_leaked(self):
# Should use the default value.
self.assertEqual("csrftoken", settings.CSRF_COOKIE_NAME)
# Fails with:
#======================================================================
#FAIL: test_csrf_cookie_name_has_leaked (test_utils.tests.LeakTest)
#----------------------------------------------------------------------
#Traceback (most recent call last):
# File "/home/freitafr/dev/django-side/tests/test_utils/tests.py", line
1514, in test_csrf_cookie_name_has_leaked
# self.assertEqual("csrftoken", settings.CSRF_COOKIE_NAME)
#AssertionError: 'csrftoken' != 'cross_site_request_forgery_token'
#- csrftoken
#+ cross_site_request_forgery_token
}}}
The cleanups from `addCleanup()` are scheduled to happen in reverse order
to the order they are added (LIFO). That should make sure each cleanup is
executed from the innermost to the outermost.
I am happy to submit a patch for the fix, but I am concerned the change
may be backward incompatible. Perhaps people relied on this ordering, and
“fixing” the ordering will trigger test bleeding in their test suite. Is
`TestContextDecorator` considered private enough to be changed with a
mention in the release note?
--
Ticket URL: <https://code.djangoproject.com/ticket/31944>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: François Freitag | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
New description:
`unittest.TestCase` offers an
[https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup
addCleanup()] function that is guaranteed to run even if
`unittest.TestCase.setUp()` fails.
In #29024, the cleanup was implemented by
[https://github.com/django/django/blob/0b0658111cba538b91072b9a133fd5545f3f46d1/django/test/utils.py#L357-L361
catching Exception, calling TestContextDecorator.disable() and re-
raising]. Using `addCleanup()` allows a small simplification. More
importantly, it prevents an ordering issue with the cleanups that can
cause tests to bleed when subclasses of Django `TestCase` relying of
`addCleanup()` have their cleanups occurring after the
`TestContextDecorator.disable()`. To illustrate:
{{{#!python
class ChangeCSRFCookieName(TestContextDecorator):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.stack = contextlib.ExitStack()
def enable(self):
# 1. Before setUp, TestContextDecorator runs enable().
cm =
override_settings(CSRF_COOKIE_NAME="cross_site_request_forgery_token")
self.stack.enter_context(cm)
def disable(self):
# 3. After tearDown, TestContextDecorator runs disable(), which
restores settings captured in 1.
# This should be step 4.
self.stack.close()
class ChangeSettingsTestCase(SimpleTestCase):
def setUp(self):
super().setUp()
# 2. Capture settings after they’ve been changed in 1.
settings_cm = self.settings(CACHES={})
settings_cm.enable()
self.addCleanup(settings_cm.disable) # 4. Restore settings
captured in 2. Should be executed at step 3.
--
Comment (by François Freitag):
Clarified ordering issue with comments in the example code.
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:1>
Django
Reporter: François Freitag | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => needsinfo
Comment:
Thanks for this ticket, however I cannot reproduce a test failure.
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:2>
Django
Reporter: François Freitag | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* Attachment "31944_repr.patch" added.
Patch modifyingt the test suite to reproduce the failure.
Django
Reporter: François Freitag | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* status: closed => new
* resolution: needsinfo =>
Comment:
Thanks for attempting to reproduce. To show the leak, tests must be run
sequentially. Otherwise, the second test case (LeakTest) runs on another
process where settings were not modified, thus does not detect the leak.
Steps to reproduce in my environment:
1. `git checkout 20d38fd75903b58335abd8e7cc576a1f1219a4fa`
2. Apply attached patch
3. `cd tests`
2. `./runtests.py test_utils.tests --parallel=1`
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: François Freitag | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Unreviewed => Accepted
Comment:
> Is TestContextDecorator considered private enough to be changed with a
mention in the release note?
It should be fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
Type: | Freitag
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by François Freitag):
* owner: nobody => François Freitag
* status: new => assigned
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/13391
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
Type: | Freitag
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
Type: | Freitag
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"57dadfac3cfae050c49b80c5ac56008acdef5196" 57dadfac]:
{{{
#!CommitTicketReference repository=""
revision="57dadfac3cfae050c49b80c5ac56008acdef5196"
Fixed #31944 -- Used addCleanup() to register TestContextDecorator
cleanups.
Cleanups from addCleanup() are scheduled to happen in reverse order to
the order they are added (LIFO). Ensures each cleanup is executed from
the innermost to the outermost.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31944#comment:7>