Fix cleanup functions not being invoked on test failures

Also delay calling tearDown() when --pdb is given, so users still have
access to the instance variables (which are usually cleaned up during tearDown())
when debugging.

Fix #6947

Author: nicoddemus

changelog/6947.bugfix.rst

@@ -0,0 +1 @@
+Fix regression where functions registered with ``TestCase.addCleanup`` were not being called on test failures.

src/_pytest/debugging.py

@@ -272,11 +272,15 @@ def pytest_internalerror(self, excrepr, excinfo):
 class PdbTrace:
     @hookimpl(hookwrapper=True)
     def pytest_pyfunc_call(self, pyfuncitem):
-        _test_pytest_function(pyfuncitem)
+        wrap_pytest_function_for_tracing(pyfuncitem)
         yield
 
 
-def _test_pytest_function(pyfuncitem):
+def wrap_pytest_function_for_tracing(pyfuncitem):
+    """Changes the python function object of the given Function item by a wrapper which actually
+    enters pdb before calling the python function itself, effectively leaving the user
+    in the pdb prompt in the first statement of the function.
+    """
     _pdb = pytestPDB._init_pdb("runcall")
     testfunction = pyfuncitem.obj
 
@@ -291,6 +295,13 @@ def wrapper(*args, **kwargs):
     pyfuncitem.obj = wrapper
 
 
+def maybe_wrap_pytest_function_for_tracing(pyfuncitem):
+    """Wrap the given pytestfunct item for tracing support if --trace was given in
+    the command line"""
+    if pyfuncitem.config.getvalue("trace"):
+        wrap_pytest_function_for_tracing(pyfuncitem)
+
+
 def _enter_pdb(node, excinfo, rep):
     # XXX we re-use the TerminalReporter's terminalwriter
     # because this seems to avoid some encoding related troubles

src/_pytest/unittest.py

@@ -1,5 +1,4 @@
 """ discovery and running of std-library "unittest" style tests. """
-import functools
 import sys
 import traceback
 
@@ -114,15 +113,18 @@ class TestCaseFunction(Function):
     _testcase = None
 
     def setup(self):
-        self._needs_explicit_tearDown = False
         self._testcase = self.parent.obj(self.name)
         self._obj = getattr(self._testcase, self.name)
         if hasattr(self, "_request"):
             self._request._fillfixtures()
 
+        # a bound method to be called during teardown() if set (see 'runtest()')
+        self._explicit_tearDown = None
+
     def teardown(self):
-        if self._needs_explicit_tearDown:
-            self._testcase.tearDown()
+        if self._explicit_tearDown:
+            self._explicit_tearDown()
+            self._explicit_tearDown = None
         self._testcase = None
         self._obj = None
 
@@ -205,40 +207,31 @@ def _expecting_failure(self, test_method) -> bool:
         return bool(expecting_failure_class or expecting_failure_method)
 
     def runtest(self):
-        # TODO: move testcase reporter into separate class, this shouldnt be on item
-        import unittest
-
-        testMethod = getattr(self._testcase, self._testcase._testMethodName)
-
-        class _GetOutOf_testPartExecutor(KeyboardInterrupt):
-            """Helper exception to get out of unittests's testPartExecutor (see TestCase.run)."""
+        from _pytest.debugging import maybe_wrap_pytest_function_for_tracing
 
-        @functools.wraps(testMethod)
-        def wrapped_testMethod(*args, **kwargs):
-            """Wrap the original method to call into pytest's machinery, so other pytest
-            features can have a chance to kick in (notably --pdb)"""
-            try:
-                self.ihook.pytest_pyfunc_call(pyfuncitem=self)
-            except unittest.SkipTest:
-                raise
-            except Exception as exc:
-                expecting_failure = self._expecting_failure(testMethod)
-                if expecting_failure:
-                    raise
-                self._needs_explicit_tearDown = True
-                raise _GetOutOf_testPartExecutor(exc)
+        maybe_wrap_pytest_function_for_tracing(self)
 
         # let the unittest framework handle async functions
         if is_async_function(self.obj):
             self._testcase(self)
         else:
-            setattr(self._testcase, self._testcase._testMethodName, wrapped_testMethod)
+            # when --pdb is given, we want to postpone calling tearDown() otherwise
+            # when entering the pdb prompt, tearDown() would have probably cleaned up
+            # instance variables, which makes it difficult to debug
+            # arguably we could always postpone tearDown(), but this changes the moment where the
+            # TestCase instance interacts with the results object, so better to only do it
+            # when absolutely needed
+            if self.config.getoption("usepdb"):
+                self._explicit_tearDown = self._testcase.tearDown
+                setattr(self._testcase, "tearDown", lambda *args: None)
+
+            # we need to update the actual bound method with self.obj, because
+            # wrap_pytest_function_for_tracing replaces self.obj by a wrapper
+            setattr(self._testcase, self.name, self.obj)
             try:
-                self._testcase(result=self)
-            except _GetOutOf_testPartExecutor as exc:
-                raise exc.args[0] from exc.args[0]
+                self._testcase.run(result=self)
             finally:
-                delattr(self._testcase, self._testcase._testMethodName)
+                delattr(self._testcase, self.name)
 
     def _prunetraceback(self, excinfo):
         Function._prunetraceback(self, excinfo)

testing/test_unittest.py

@@ -876,6 +876,37 @@ def test_notTornDown():
     reprec.assertoutcome(passed=1, failed=1)
 
 
+def test_cleanup_functions(testdir):
+    """Ensure functions added with addCleanup are always called after each test ends (#6947)"""
+    testdir.makepyfile(
+        """
+        import unittest
+
+        cleanups = []
+
+        class Test(unittest.TestCase):
+
+            def test_func_1(self):
+                self.addCleanup(cleanups.append, "test_func_1")
+
+            def test_func_2(self):
+                self.addCleanup(cleanups.append, "test_func_2")
+                assert 0
+
+            def test_func_3_check_cleanups(self):
+                assert cleanups == ["test_func_1", "test_func_2"]
+    """
+    )
+    result = testdir.runpytest("-v")
+    result.stdout.fnmatch_lines(
+        [
+            "*::test_func_1 PASSED *",
+            "*::test_func_2 FAILED *",
+            "*::test_func_3_check_cleanups PASSED *",
+        ]
+    )
+
+
 def test_issue333_result_clearing(testdir):
     testdir.makeconftest(
         """
@@ -1131,6 +1162,41 @@ def test(self):
     assert result.ret == 0
 
 
+def test_pdb_teardown_called(testdir, monkeypatch):
+    """Ensure tearDown() is always called when --pdb is given in the command-line.
+
+    We delay the normal tearDown() calls when --pdb is given, so this ensures we are calling
+    tearDown() eventually to avoid memory leaks when using --pdb.
+    """
+    teardowns = []
+    monkeypatch.setattr(
+        pytest, "test_pdb_teardown_called_teardowns", teardowns, raising=False
+    )
+
+    testdir.makepyfile(
+        """
+        import unittest
+        import pytest
+
+        class MyTestCase(unittest.TestCase):
+
+            def tearDown(self):
+                pytest.test_pdb_teardown_called_teardowns.append(self.id())
+
+            def test_1(self):
+                pass
+            def test_2(self):
+                pass
+    """
+    )
+    result = testdir.runpytest_inprocess("--pdb")
+    result.stdout.fnmatch_lines("* 2 passed in *")
+    assert teardowns == [
+        "test_pdb_teardown_called.MyTestCase.test_1",
+        "test_pdb_teardown_called.MyTestCase.test_2",
+    ]
+
+
 def test_async_support(testdir):
     pytest.importorskip("unittest.async_case")