Opened 4 years ago

Closed 4 years ago

#31944 closed Cleanup/optimization (fixed)

Use addCleanup() to register TestContextDecorator cleanups.

Reported by: François Freitag Owned by: François Freitag
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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?

Attachments (1)

31944_repr.patch (1.4 KB ) - added by François Freitag 4 years ago.
Patch modifyingt the test suite to reproduce the failure.

Download all attachments as: .zip

Change History (8)

comment:1 by François Freitag, 4 years ago

Description: modified (diff)

Clarified ordering issue with comments in the example code.

comment:2 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed

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

Version 0, edited 4 years ago by Mariusz Felisiak (next)

by François Freitag, 4 years ago

Attachment: 31944_repr.patch added

Patch modifyingt the test suite to reproduce the failure.

comment:3 by François Freitag, 4 years ago

Resolution: needsinfo
Status: closednew

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

comment:4 by Mariusz Felisiak, 4 years ago

Summary: Use addCleanup() to register TestContextDecorator cleanupsUse addCleanup() to register TestContextDecorator cleanups.
Triage Stage: UnreviewedAccepted

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

It should be fine.

comment:5 by François Freitag, 4 years ago

Has patch: set
Owner: changed from nobody to François Freitag
Status: newassigned

comment:6 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

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.

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