Merge lp:~lifeless/testtools/bug-335816-one-outcome-per-test into lp:~testtools-committers/testtools/trunk

[Robert Collins]

Fix the bug where multiple exceptions in a single test corrupt test result objects output.

[Robert Collins (lifeless)]

jml says ok,

=== modified file 'NEWS'
--- NEWS	2010-01-19 01:40:58 +0000
+++ NEWS	2010-05-13 12:20:50 +0000
@@ -34,6 +34,11 @@
 * The backtrace test result output tests should now pass on windows and other
   systems where os.sep is not '/'.
 
+* When a cleanUp or tearDown exception occurs, it is now accumulated as a new
+  traceback in the test details, rather than as a separate call to addError / 
+  addException. This makes testtools work better with most TestResult objects
+  and fixes bug #335816.
+
 
 0.9.2
 ~~~~~

=== modified file 'testtools/runtest.py'
--- testtools/runtest.py	2009-12-31 03:15:19 +0000
+++ testtools/runtest.py	2010-05-13 12:20:50 +0000
@@ -36,6 +36,8 @@
         classes in the list).
     :ivar exception_caught: An object returned when _run_user catches an
         exception.
+    :ivar _exceptions: A list of caught exceptions, used to do the single
+        reporting of error/failure/skip etc.
     """
 
     def __init__(self, case, handlers=None):
@@ -48,6 +50,7 @@
         self.case = case
         self.handlers = handlers or []
         self.exception_caught = object()
+        self._exceptions = []
 
     def run(self, result=None):
         """Run self.case reporting activity to result.
@@ -86,7 +89,16 @@
         result.startTest(self.case)
         self.result = result
         try:
+            self._exceptions = []
             self._run_core()
+            if self._exceptions:
+                # One or more caught exceptions, now trigger the test's
+                # reporting method for just one.
+                e = self._exceptions.pop()
+                for exc_class, handler in self.handlers:
+                    if isinstance(e, exc_class):
+                        handler(self.case, self.result, e)
+                        break
         finally:
             result.stopTest(self.case)
         return result
@@ -96,7 +108,9 @@
         if self.exception_caught == self._run_user(self.case._run_setup,
             self.result):
             # Don't run the test method if we failed getting here.
-            self.case._runCleanups(self.result)
+            e = self.case._runCleanups(self.result)
+            if e is not None:
+                self._exceptions.append(e)
             return
         # Run everything from here on in. If any of the methods raise an
         # exception we'll have failed.
@@ -112,8 +126,9 @@
                     failed = True
             finally:
                 try:
-                    if not self._run_user(
-                        self.case._runCleanups, self.result):
+                    e = self._run_user(self.case._runCleanups, self.result)
+                    if e is not None:
+                        self._exceptions.append(e)
                         failed = True
                 finally:
                     if not failed:
@@ -134,9 +149,9 @@
             # escape: but they are deprecated anyway.
             exc_info = sys.exc_info()
             e = exc_info[1]
+            self.case.onException(exc_info)
             for exc_class, handler in self.handlers:
-                self.case.onException(exc_info)
                 if isinstance(e, exc_class):
-                    handler(self.case, self.result, e)
+                    self._exceptions.append(e)
                     return self.exception_caught
             raise e

=== modified file 'testtools/testcase.py'
--- testtools/testcase.py	2010-01-16 04:36:08 +0000
+++ testtools/testcase.py	2010-05-13 12:20:50 +0000
@@ -76,6 +76,7 @@
         unittest.TestCase.__init__(self, *args, **kwargs)
         self._cleanups = []
         self._unique_id_gen = itertools.count(1)
+        self._traceback_id_gen = itertools.count(0)
         self.__setup_called = False
         self.__teardown_called = False
         self.__details = {}
@@ -145,9 +146,10 @@
 
         See the docstring for addCleanup for more information.
 
-        Returns True if all cleanups ran without error, False otherwise.
+        :return: None if all cleanups ran without error, the most recently
+            raised exception from the cleanups otherwise.
         """
-        ok = True
+        last_exception = None
         while self._cleanups:
             function, arguments, keywordArguments = self._cleanups.pop()
             try:
@@ -155,9 +157,10 @@
             except KeyboardInterrupt:
                 raise
             except:
-                self._report_error(self, result, None)
-                ok = False
-        return ok
+                exc_info = sys.exc_info()
+                self._report_traceback(exc_info)
+                last_exception = exc_info[1]
+        return last_exception
 
     def addCleanup(self, function, *arguments, **keywordArguments):
         """Add a cleanup function to be called after tearDown.
@@ -288,8 +291,7 @@
             predicate(*args, **kwargs)
         except self.failureException:
             exc_info = sys.exc_info()
-            self.addDetail('traceback',
-                content.TracebackContent(exc_info, self))
+            self._report_traceback(exc_info)
             raise _ExpectedFailure(exc_info)
         else:
             raise _UnexpectedSuccess(reason)
@@ -323,12 +325,14 @@
 
         :seealso addOnException:
         """
+        if exc_info[0] not in [
+            TestSkipped, _UnexpectedSuccess, _ExpectedFailure]:
+            self._report_traceback(exc_info)
         for handler in self.__exception_handlers:
             handler(exc_info)
 
     @staticmethod
     def _report_error(self, result, err):
-        self._report_traceback()
         result.addError(self, details=self.getDetails())
 
     @staticmethod
@@ -337,7 +341,6 @@
 
     @staticmethod
     def _report_failure(self, result, err):
-        self._report_traceback()
         result.addFailure(self, details=self.getDetails())
 
     @staticmethod
@@ -349,9 +352,13 @@
         self._add_reason(reason)
         result.addSkip(self, details=self.getDetails())
 
-    def _report_traceback(self):
-        self.addDetail('traceback',
-            content.TracebackContent(sys.exc_info(), self))
+    def _report_traceback(self, exc_info):
+        tb_id = advance_iterator(self._traceback_id_gen)
+        if tb_id:
+            tb_label = 'traceback-%d' % tb_id
+        else:
+            tb_label = 'traceback'
+        self.addDetail(tb_label, content.TracebackContent(exc_info, self))
 
     @staticmethod
     def _report_unexpected_success(self, result, err):

=== modified file 'testtools/tests/test_runtest.py'
--- testtools/tests/test_runtest.py	2009-12-31 03:15:19 +0000
+++ testtools/tests/test_runtest.py	2010-05-13 12:20:50 +0000
@@ -77,14 +77,12 @@
         e = KeyError('Yo')
         def raises():
             raise e
-        def log_exc(self, result, err):
-            log.append((result, err))
-        run = RunTest(case, [(KeyError, log_exc)])
+        run = RunTest(case, [(KeyError, None)])
         run.result = ExtendedTestResult()
         status = run._run_user(raises)
         self.assertEqual(run.exception_caught, status)
         self.assertEqual([], run.result._events)
-        self.assertEqual(["got it", (run.result, e)], log)
+        self.assertEqual(["got it"], log)
 
     def test__run_user_can_catch_Exception(self):
         case = self.make_case()
@@ -92,14 +90,12 @@
         def raises():
             raise e
         log = []
-        def log_exc(self, result, err):
-            log.append((result, err))
-        run = RunTest(case, [(Exception, log_exc)])
+        run = RunTest(case, [(Exception, None)])
         run.result = ExtendedTestResult()
         status = run._run_user(raises)
         self.assertEqual(run.exception_caught, status)
         self.assertEqual([], run.result._events)
-        self.assertEqual([(run.result, e)], log)
+        self.assertEqual([], log)
 
     def test__run_user_uncaught_Exception_raised(self):
         case = self.make_case()

=== modified file 'testtools/tests/test_testtools.py'
--- testtools/tests/test_testtools.py	2010-01-16 04:20:26 +0000
+++ testtools/tests/test_testtools.py	2010-05-13 12:20:50 +0000
@@ -301,6 +301,9 @@
         def runTest(self):
             self._calls.append('runTest')
 
+        def brokenTest(self):
+            raise RuntimeError('Deliberate broken test')
+
         def tearDown(self):
             self._calls.append('tearDown')
             TestCase.tearDown(self)
@@ -400,13 +403,29 @@
         self.assertRaises(
             KeyboardInterrupt, self.test.run, self.logging_result)
 
-    def test_multipleErrorsReported(self):
-        # Errors from all failing cleanups are reported.
-        self.test.addCleanup(lambda: 1/0)
-        self.test.addCleanup(lambda: 1/0)
-        self.test.run(self.logging_result)
-        self.assertErrorLogEqual(
-            ['startTest', 'addError', 'addError', 'stopTest'])
+    def test_multipleCleanupErrorsReported(self):
+        # Errors from all failing cleanups are reported as separate backtraces.
+        self.test.addCleanup(lambda: 1/0)
+        self.test.addCleanup(lambda: 1/0)
+        self.logging_result = ExtendedTestResult()
+        self.test.run(self.logging_result)
+        self.assertEqual(['startTest', 'addError', 'stopTest'],
+            [event[0] for event in self.logging_result._events])
+        self.assertEqual(set(['traceback', 'traceback-1']),
+            set(self.logging_result._events[1][2].keys()))
+
+    def test_multipleErrorsCoreAndCleanupReported(self):
+        # Errors from all failing cleanups are reported, with stopTest,
+        # startTest inserted.
+        self.test = TestAddCleanup.LoggingTest('brokenTest')
+        self.test.addCleanup(lambda: 1/0)
+        self.test.addCleanup(lambda: 1/0)
+        self.logging_result = ExtendedTestResult()
+        self.test.run(self.logging_result)
+        self.assertEqual(['startTest', 'addError', 'stopTest'],
+            [event[0] for event in self.logging_result._events])
+        self.assertEqual(set(['traceback', 'traceback-1', 'traceback-2']),
+            set(self.logging_result._events[1][2].keys()))
 
 
 class TestWithDetails(TestCase):