#31944: Use addCleanup() to register TestContextDecorator cleanups
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Testing framework | Version: master
Severity: Normal | Resolution:
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:
> `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?
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.
@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?
--
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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/073.611ba0d5b1c5280873b228875d7038ef%40djangoproject.com.
