Opened 6 years ago

Closed 6 years ago

#29024 closed Bug (fixed)

`TestContextDecorator` never exits if `setUp` fails in tests

Reported by: Anthony King Owned by: Kamil
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description (last modified by Anthony King)

when using TestContextDecorator as a class decorator, enable is called just before Test.setUp, and disable is called after Test.tearDown.

unfortunately, tearDown is not called in the event of a failure inside setUp. This can result in unexpected behaviour.

Even though this class is not documented, there are decorators that use it that are.

example:

class example_decorator(TestContextDecorator):
    some_var = False

    def enable(self):
        self.__class__.some_var = True

    def disable(self):
        self.__class__.some_var = False

class Example1TestCase(TestCase):
    def test_var_is_false(self):
        # some_var will be False
        self.assertFalse(example_decorator.some_var)


@example_decorator()
class Example2TestCase(TestCase):
    def setUp(self):
        raise Exception

    def test_var_is_true(self):
        # won't be hit due to exception in setUp
        self.assertTrue(example_decorator.some_var)


class Example3TestCase(TestCase):
    def test_var_is_false(self):
        # some_var will be True now due to no cleanup in Example2TestCase
        self.assertFalse(example_decorator.some_var)

output:

.EF
======================================================================
ERROR: test_var_is_true (example.tests.Example2TestCase)
----------------------------------------------------------------------
... Traceback

======================================================================
FAIL: test_var_is_false (example.tests.Example3TestCase)
----------------------------------------------------------------------
...
AssertionError: True is not false

Ran 3 tests in 0.007s

FAILED (failures=1, errors=1)

There are 2 potential fixes here:

  • decorate setUpClass and tearDownClass
  • use addCleanup inside the setUp decorator

addCleanup will always run, and will only ever be registered if the context manager was successfully enabled.

It would also be useful to have this class documented, however that's out of scope of this ticket.

Change History (10)

comment:1 by Simon Charette, 6 years ago

Triage Stage: UnreviewedAccepted
Version: 1.11master

Using addCleanup instead of hooking up on tearDown makes sense here.

comment:2 by Shahbaj Sayyad, 6 years ago

Owner: changed from nobody to Shahbaj Sayyad
Status: newassigned

comment:3 by Anthony King, 6 years ago

Description: modified (diff)

comment:4 by Anthony King, 6 years ago

Hi Shahbaj,

I saw your post in the mailing list.

So how we've done it internally is like this:

class TestContextDecorator(DjangoTestContextDecorator):
    def decorate_class(self, cls):
        # https://code.djangoproject.com/ticket/29024

        if issubclass(cls, TestCase):
            decorated_setUp = cls.setUp

            @wraps(decorated_setUp)
            def setUp(inner_self):
                context = self.enable()
                if self.attr_name:
                    setattr(inner_self, self.attr_name, context)
                inner_self.addCleanup(self.disable)
                decorated_setUp(inner_self)

            cls.setUp = setUp
            return cls
        raise TypeError('Can only decorate subclasses of unittest.TestCase')

comment:5 by Shahbaj Sayyad, 6 years ago

Has patch: set

PR (no test)

Last edited 6 years ago by Tim Graham (previous) (diff)

comment:6 by Tim Graham, 6 years ago

Needs tests: set

comment:7 by Shahbaj Sayyad, 6 years ago

Owner: Shahbaj Sayyad removed
Status: assignednew

comment:8 by Kamil, 6 years ago

Owner: set to Kamil
Status: newassigned

comment:9 by Jeff, 6 years ago

Patch needs improvement: set

Recommendations present on the new PR.

Last edited 6 years ago by Tim Graham (previous) (diff)

comment:10 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 3d4080f:

Fixed #29024 -- Made TestContextDecorator call disable() if setUp() raises an exception.

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