Factor the execution of a test method into its own function to ensure proper cleanup

[amccarth]

Summary

This solves problems in tearDown of individual test cases, which were especially noticeable on Windows.

When a test throws an exception, the exception retains references to the functions in the stack, which in term keeps their local variables alive. When those locals are SB proxy objects, cleanup doesn't proceed as it should because they keep LLDB objects alive that should have been orphaned.

By moving the running of the test method to a separate function, we ensure that the exception goes out of scope before we attempt tearDown, and the gc.collect() makes sure the orphaned objects are properly destroyed before the framework attempts to delete the target.

Without this fix, a test case that throws might cause the shared module list to retain an open handle to the inferior, even after the target is deleted. This affects the state of subsequent test cases in the same test. This was especially noticeable on Windows in tests like TestTargetAPI, because the open handle prevented make from deleting the inferior after the failed test case and the next test case from re-building the inferior. Thus nearly all tests cases would fail because of a problem in just one.

[tfiala]

LGTM, Adrian. Thanks!

[zturner]

test/lldbtest.py

2548–2550 ↗

(On Diff #37523)

This problem is more general than just modules. Maybe it could say "so that we can be sure that all test-specific resources have been freed before we attempt to delete the targets."

[amccarth]

Nice catch on the comment.

lldb/trunk/source/Target/Target.cpp

if (m_valid)
         LoadScriptingResourceForModule(module_sp, this);
         ModulesDidLoad (my_module_list);
     }
 }
 
 void
 Target::ModuleRemoved (const ModuleList& module_list, const ModuleSP &module_sp)
 {
-    // A module is being added to this target for the first time
+    // A module is being removed from this target.
     if (m_valid)
     {
         ModuleList my_module_list;
         my_module_list.Append(module_sp);
         ModulesDidUnload (my_module_list, false);
     }
 }
 

lldb/trunk/test/lldbtest.py

 ----------------------------------------------------------------------
 Ran 72 tests in 135.468s
 
 OK
 $ 
 """
 
 import abc
+import gc
 import glob
 import os, sys, traceback
 import os.path
 import re
 import signal
 from subprocess import *
 import StringIO
 import time
def get_process_working_directory(self):
         else:
             # local tests change directory into each test subdirectory
             return os.getcwd() 
     
     def tearDown(self):
         #import traceback
         #traceback.print_stack()
 
+        # Ensure all the references to SB objects have gone away so that we can
+        # be sure that all test-specific resources have been freed before we
+        # attempt to delete the targets.
+        gc.collect()
+
         # Delete the target(s) from the debugger as a general cleanup step.
         # This includes terminating the process for each target, if any.
         # We'd like to reuse the debugger for our next test without incurring
         # the initialization overhead.
         targets = []
         for target in self.dbg:
             if target:
                 targets.append(target)

lldb/trunk/test/unittest2/case.py

def run(self, result=None):
             success = False
             try:
                 self.setUp()
             except SkipTest, e:
                 self._addSkip(result, str(e))
             except Exception:
                 result.addError(self, sys.exc_info())
             else:
-                try:
-                    testMethod()
-                except self.failureException:
-                    result.addFailure(self, sys.exc_info())
-                except _ExpectedFailure, e:
-                    addExpectedFailure = getattr(result, 'addExpectedFailure', None)
-                    if addExpectedFailure is not None:
-                        addExpectedFailure(self, e.exc_info, e.bugnumber)
-                    else: 
-                        warnings.warn("Use of a TestResult without an addExpectedFailure method is deprecated", 
-                                      DeprecationWarning)
-                        result.addSuccess(self)
-                except _UnexpectedSuccess, x:
-                    addUnexpectedSuccess = getattr(result, 'addUnexpectedSuccess', None)
-                    if addUnexpectedSuccess is not None:
-                        addUnexpectedSuccess(self, x.bugnumber)
-                    else:
-                        warnings.warn("Use of a TestResult without an addUnexpectedSuccess method is deprecated", 
-                                      DeprecationWarning)
-                        result.addFailure(self, sys.exc_info())
-                except SkipTest, e:
-                    self._addSkip(result, str(e))
-                except Exception:
-                    result.addError(self, sys.exc_info())
-                else:
-                    success = True
-
-                try:
-                    self.tearDown()
-                except Exception:
-                    result.addCleanupError(self, sys.exc_info())
-                    success = False
-
-                self.dumpSessionInfo()
-
-            cleanUpSuccess = self.doCleanups()
-            success = success and cleanUpSuccess
-            if success:
-                result.addSuccess(self)
-        finally:
-            result.stopTest(self)
-            if orig_result is None:
-                stopTestRun = getattr(result, 'stopTestRun', None)
-                if stopTestRun is not None:
-                    stopTestRun()
+                success = self.runMethod(testMethod, result)
+
+                try:
+                    self.tearDown()
+                except Exception:
+                    result.addCleanupError(self, sys.exc_info())
+                    success = False
+
+                self.dumpSessionInfo()
+
+            cleanUpSuccess = self.doCleanups()
+            success = success and cleanUpSuccess
+            if success:
+                result.addSuccess(self)
+        finally:
+            result.stopTest(self)
+            if orig_result is None:
+                stopTestRun = getattr(result, 'stopTestRun', None)
+                if stopTestRun is not None:
+                    stopTestRun()
+
+    def runMethod(self, testMethod, result):
+        """Runs the test method and catches any exception that might be thrown.
+
+        This is factored out of TestCase.run() to ensure that any exception
+        thrown during the test goes out of scope before tearDown.  Otherwise, an
+        exception could hold references to Python objects that are bound to
+        SB objects and prevent them from being deleted in time.
+        """
+            else:
+                warnings.warn("Use of a TestResult without an addExpectedFailure method is deprecated",
+                warnings.warn("Use of a TestResult without an addUnexpectedSuccess method is deprecated",
+            return True
+        return False
 
     def doCleanups(self):
         """Execute all cleanup functions. Normally called for you after
         tearDown."""
         result = self._resultForDoCleanups
         ok = True
         while self._cleanups:
             function, args, kwargs = self._cleanups.pop(-1)