Use addCleanup() to register TestContextDecorator cleanups.

[François Freitag]

unittest.TestCase offers an ​addCleanup() function that is guaranteed to run even if unittest.TestCase.setUp() fails.

In #29024, the cleanup was implemented by ​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:

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?

[François Freitag]

Clarified ordering issue with comments in the example code.

[Mariusz Felisiak]

Thanks for this ticket, however I cannot reproduce a test failure.

[François Freitag]

Patch modifyingt the test suite to reproduce the failure.

[François Freitag]

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
4. ./runtests.py test_utils.tests --parallel=1

[Mariusz Felisiak]

Is TestContextDecorator considered private enough to be changed with a mention in the release note?

It should be fine.

[François Freitag]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 57dadfac:

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.