Merge lp:~jml/testtools/unexpected-success-2 into lp:~testtools-committers/testtools/trunk

[Jonathan Lange]

This branch finished off the rest of bug 654474. It changes the contract of TestResult such that wasSuccessful returns False if there has ever been an unexpected success.

In addition, it updates the contract tests to test for the behaviour of wasSuccessful in many more cases, and adds contract tests for Python 2.6 and 2.7 which we run against our fake implementations.

[lp:~jml/testtools/unexpected-success-2]

145. By Jonathan Lange on 2010-11-28

Update NEWS

[Robert Collins (lifeless)]

Seems to me that wasSuccessful() should be reset by startTestRun - what do you think?

Rather than a mixin, this looks like a job for testscenarios. Its fine without but clearly not as nice.

review: Approve

[Jonathan Lange (jml)]

Agreed on both points. Will only update startTestRun for testtools
TestResult, since I'm not controlling Python.

Would be better with testscenarios, but not in this branch. Also, we need
to have a chat about dependencies.

Thanks,
jml

[Martin Packman (gz)]

On Python 3 (with lp:~gz/testtools/raises_regressions_675327 merged) I get three failures. All in TestMultiTestresultContract (nit, should that be TestMultiTest*R*esultContract?) the methods test_addError_is_failure, test_addFailure_is_failure, and test_addUnexpectedSuccess_was_successful all return True from `result.wasSuccessful()`.

There are actually four failures, but one is silent...

As noted by Martin Pool in bug 654474 comment 5 changing the outcome without also providing feedback is bad, so I think TextTestResult.stopTestRun also needs changing. Something along the lines of but prettier than:

=== modified file 'testtools/testresult/real.py'
--- testtools/testresult/real.py 2010-10-24 09:21:01 +0000
+++ testtools/testresult/real.py 2010-11-28 19:18:08 +0000
@@ -258,6 +261,8 @@
         stop = self._now()
         self._show_list('ERROR', self.errors)
         self._show_list('FAIL', self.failures)
+ for test in self.unexpectedSuccesses:
+ self.stream.write("%sUNEXPECTEDSUCCESS: %s\n%s" % (self.sep1, test.id(), self.sep2))
         self.stream.write("Ran %d test%s in %.3fs\n\n" %
             (self.testsRun, plural,
              self._delta_to_float(stop - self.__start)))
@@ -267,7 +272,7 @@
             self.stream.write("FAILED (")
             details = []
             details.append("failures=%d" % (
- len(self.failures) + len(self.errors)))
+ len(self.failures) + len(self.errors) + len(self.unexpectedSuccesses)))
             self.stream.write(", ".join(details))
             self.stream.write(")\n")
         super(TextTestResult, self).stopTestRun()

The silent failure that should then be revealed on Python 3 also needs fixing:

=== modified file 'testtools/tests/test_compat.py'
--- testtools/tests/test_compat.py 2010-11-11 09:46:18 +0000
+++ testtools/tests/test_compat.py 2010-11-28 19:18:08 +0000
@@ -19,6 +19,7 @@
     )
 from testtools.matchers import (
     MatchesException,
+ Not,
     Raises,
     )

@@ -246,7 +247,7 @@
         if newio:
             self.expectFailure("Python 3 StringIO expects text not bytes",
                 self.assertThat, lambda: soutwrapper.write(self.uni),
- Raises(MatchesException(TypeError)))
+ Not(Raises(MatchesException(TypeError))))
         soutwrapper.write(self.uni)
         self.assertEqual("pa???n", sout.getvalue())

Final nit:

+ def test_addUnexpectedSuccess_was_successful(self):
+ # addUnexpectedSuccess does not the test run in Python 2.7.

Verb?

review: Needs Fixing

[Jonathan Lange (jml)]

mgz, thanks very much for the review. I've fixed the nits, TextTestResult, the MultiTestResult failure (map(f, xs) returns an iterator in Python 3) and the hidden failure in test_compat. I also added tests for the TextTestResult changes.

lifeless, I've made the startTestRun change.

I'm going to merge it now, but I'd welcome any comments you have on the incremental diff.

[lp:~jml/testtools/unexpected-success-2]

146. By Jonathan Lange on 2010-11-28

minor nits

147. By Jonathan Lange on 2010-11-29

Make MultiTestResult at all usable in Python 3

148. By Jonathan Lange on 2010-11-29

startTestRun resets unexpected successes and other errors

149. By Jonathan Lange on 2010-11-29

NEWS update

150. By Jonathan Lange on 2010-11-29

Actually report the error in TextTestResult (thanks mgz)

151. By Jonathan Lange on 2010-11-29

Fix a inaccurate unexpected success, hidden by our broken Python 3 support

[Robert Collins (lifeless)]

+ len(self.failures) + len(self.errors)))
+ len(self.failures) + len(self.errors)
+ + len(self.unexpectedSuccesses)))
sum(map(len, (self, self.failures, self.errors,
    self.unexpectedSuccesses)))

Would be cleaner, I think?

=== modified file 'NEWS'
--- NEWS	2010-11-29 00:11:19 +0000
+++ NEWS	2010-11-29 00:28:14 +0000
@@ -8,7 +8,12 @@
 -------
 
 * addUnexpectedSuccess is translated to addFailure for test results that don't
-  know about addUnexpectedSuccess.  (Jonathan Lange, #654474)
+  know about addUnexpectedSuccess.  Further, it fails the entire result for
+  all testtools TestResults (i.e. wasSuccessful() returns False after
+  addUnexpectedSuccess has been called).  (Jonathan Lange, #654474)
+
+* startTestRun will reset any errors on the result.  That is, wasSuccessful()
+  will always return True immediately after startTestRun() is called.
 
 * Responsibility for running test cleanups has been moved to ``RunTest``.
   This change does not affect public APIs and can be safely ignored by test

=== modified file 'testtools/testresult/doubles.py'
--- testtools/testresult/doubles.py	2009-12-31 03:15:19 +0000
+++ testtools/testresult/doubles.py	2010-11-29 00:28:14 +0000
@@ -1,4 +1,4 @@
-# Copyright (c) 2009 Jonathan M. Lange. See LICENSE for details.
+# Copyright (c) 2009-2010 Jonathan M. Lange. See LICENSE for details.
 
 """Doubles of test result objects, useful for testing unittest code."""
 
@@ -15,15 +15,18 @@
     def __init__(self):
         self._events = []
         self.shouldStop = False
+        self._was_successful = True
 
 
 class Python26TestResult(LoggingBase):
     """A precisely python 2.6 like test result, that logs."""
 
     def addError(self, test, err):
+        self._was_successful = False
         self._events.append(('addError', test, err))
 
     def addFailure(self, test, err):
+        self._was_successful = False
         self._events.append(('addFailure', test, err))
 
     def addSuccess(self, test):
@@ -38,6 +41,9 @@
     def stopTest(self, test):
         self._events.append(('stopTest', test))
 
+    def wasSuccessful(self):
+        return self._was_successful
+
 
 class Python27TestResult(Python26TestResult):
     """A precisely python 2.7 like test result, that logs."""
@@ -62,9 +68,11 @@
     """A test result like the proposed extended unittest result API."""
 
     def addError(self, test, err=None, details=None):
+        self._was_successful = False
         self._events.append(('addError', test, err or details))
 
     def addFailure(self, test, err=None, details=None):
+        self._was_successful = False
         self._events.append(('addFailure', test, err or details))
 
     def addExpectedFailure(self, test, err=None, details=None):
@@ -80,6 +88,7 @@
             self._events.append(('addSuccess', test))
 
     def addUnexpectedSuccess(self, test, details=None):
+        self._was_successful = False
         if details is not None:
             self._events.append(('addUnexpectedSuccess', test, details))
         else:
@@ -88,8 +97,15 @@
     def progress(self, offset, whence):
         self._events.append(('progress', offset, whence))
 
+    def startTestRun(self):
+        super(ExtendedTestResult, self).startTestRun()
+        self._was_successful = True
+
     def tags(self, new_tags, gone_tags):
         self._events.append(('tags', new_tags, gone_tags))
 
     def time(self, time):
         self._events.append(('time', time))
+
+    def wasSuccessful(self):
+        return self._was_successful

=== modified file 'testtools/testresult/real.py'
--- testtools/testresult/real.py	2010-10-24 09:21:01 +0000
+++ testtools/testresult/real.py	2010-11-29 00:28:14 +0000
@@ -108,6 +108,18 @@
         """Called when a test was expected to fail, but succeed."""
         self.unexpectedSuccesses.append(test)
 
+    def wasSuccessful(self):
+        """Has this result been successful so far?
+
+        If there have been any errors, failures or unexpected successes,
+        return False.  Otherwise, return True.
+
+        Note: This differs from standard unittest in that we consider
+        unexpected successes to be equivalent to failures, rather than
+        successes.
+        """
+        return not (self.errors or self.failures or self.unexpectedSuccesses)
+
     if str_is_unicode:
         # Python 3 and IronPython strings are unicode, use parent class method
         _exc_info_to_unicode = unittest.TestResult._exc_info_to_string
@@ -148,6 +160,9 @@
 
         New in python 2.7
         """
+        self.unexpectedSuccesses = []
+        self.errors = []
+        self.failures = []
 
     def stopTestRun(self):
         """Called after a test run completes
@@ -182,7 +197,7 @@
 
     def __init__(self, *results):
         TestResult.__init__(self)
-        self._results = map(ExtendedToOriginalDecorator, results)
+        self._results = list(map(ExtendedToOriginalDecorator, results))
 
     def _dispatch(self, message, *args, **kwargs):
         return tuple(
@@ -223,6 +238,13 @@
     def done(self):
         return self._dispatch('done')
 
+    def wasSuccessful(self):
+        """Was this result successful?
+
+        Only returns True if every constituent result was successful.
+        """
+        return all(self._dispatch('wasSuccessful'))
+
 
 class TextTestResult(TestResult):
     """A TestResult which outputs activity to a text stream."""
@@ -258,6 +280,10 @@
         stop = self._now()
         self._show_list('ERROR', self.errors)
         self._show_list('FAIL', self.failures)
+        for test in self.unexpectedSuccesses:
+            self.stream.write(
+                "%sUNEXPECTED SUCCESS: %s\n%s" % (
+                    self.sep1, test.id(), self.sep2))
         self.stream.write("Ran %d test%s in %.3fs\n\n" %
             (self.testsRun, plural,
              self._delta_to_float(stop - self.__start)))
@@ -267,7 +293,8 @@
             self.stream.write("FAILED (")
             details = []
             details.append("failures=%d" % (
-                len(self.failures) + len(self.errors)))
+                len(self.failures) + len(self.errors)
+                + len(self.unexpectedSuccesses)))
             self.stream.write(", ".join(details))
             self.stream.write(")\n")
         super(TextTestResult, self).stopTestRun()
@@ -363,6 +390,9 @@
         self._test_start = self._now()
         super(ThreadsafeForwardingResult, self).startTest(test)
 
+    def wasSuccessful(self):
+        return self.result.wasSuccessful()
+
 
 class ExtendedToOriginalDecorator(object):
     """Permit new TestResult API code to degrade gracefully with old results.
@@ -376,11 +406,13 @@
 
     def __init__(self, decorated):
         self.decorated = decorated
+        self._was_successful = True
 
     def __getattr__(self, name):
         return getattr(self.decorated, name)
 
     def addError(self, test, err=None, details=None):
+        self._was_successful = False
         self._check_args(err, details)
         if details is not None:
             try:
@@ -405,6 +437,7 @@
         return addExpectedFailure(test, err)
 
     def addFailure(self, test, err=None, details=None):
+        self._was_successful = False
         self._check_args(err, details)
         if details is not None:
             try:
@@ -431,6 +464,7 @@
         return addSkip(test, reason)
 
     def addUnexpectedSuccess(self, test, details=None):
+        self._was_successful = False
         outcome = getattr(self.decorated, 'addUnexpectedSuccess', None)
         if outcome is None:
             try:
@@ -487,6 +521,7 @@
         return self.decorated.startTest(test)
 
     def startTestRun(self):
+        self._was_successful = True
         try:
             return self.decorated.startTestRun()
         except AttributeError:
@@ -517,7 +552,7 @@
         return method(a_datetime)
 
     def wasSuccessful(self):
-        return self.decorated.wasSuccessful()
+        return self._was_successful
 
 
 class _StringException(Exception):

=== modified file 'testtools/tests/test_compat.py'
--- testtools/tests/test_compat.py	2010-11-11 09:46:18 +0000
+++ testtools/tests/test_compat.py	2010-11-29 00:28:14 +0000
@@ -19,6 +19,7 @@
     )
 from testtools.matchers import (
     MatchesException,
+    Not,
     Raises,
     )
 
@@ -196,34 +197,34 @@
         super(TestUnicodeOutputStream, self).setUp()
         if sys.platform == "cli":
             self.skip("IronPython shouldn't wrap streams to do encoding")
-    
+
     def test_no_encoding_becomes_ascii(self):
         """A stream with no encoding attribute gets ascii/replace strings"""
         sout = _FakeOutputStream()
         unicode_output_stream(sout).write(self.uni)
         self.assertEqual([_b("pa???n")], sout.writelog)
-    
+
     def test_encoding_as_none_becomes_ascii(self):
         """A stream with encoding value of None gets ascii/replace strings"""
         sout = _FakeOutputStream()
         sout.encoding = None
         unicode_output_stream(sout).write(self.uni)
         self.assertEqual([_b("pa???n")], sout.writelog)
-    
+
     def test_bogus_encoding_becomes_ascii(self):
         """A stream with a bogus encoding gets ascii/replace strings"""
         sout = _FakeOutputStream()
         sout.encoding = "bogus"
         unicode_output_stream(sout).write(self.uni)
         self.assertEqual([_b("pa???n")], sout.writelog)
-    
+
     def test_partial_encoding_replace(self):
         """A string which can be partly encoded correctly should be"""
         sout = _FakeOutputStream()
         sout.encoding = "iso-8859-7"
         unicode_output_stream(sout).write(self.uni)
         self.assertEqual([_b("pa?\xe8?n")], sout.writelog)
-    
+
     def test_unicode_encodings_not_wrapped(self):
         """A unicode encoding is left unwrapped as needs no error handler"""
         sout = _FakeOutputStream()
@@ -232,7 +233,7 @@
         sout = _FakeOutputStream()
         sout.encoding = "utf-16-be"
         self.assertIs(unicode_output_stream(sout), sout)
-    
+
     def test_stringio(self):
         """A StringIO object should maybe get an ascii native str type"""
         try:
@@ -246,7 +247,7 @@
         if newio:
             self.expectFailure("Python 3 StringIO expects text not bytes",
                 self.assertThat, lambda: soutwrapper.write(self.uni),
-                Raises(MatchesException(TypeError)))
+                Not(Raises(MatchesException(TypeError))))
         soutwrapper.write(self.uni)
         self.assertEqual("pa???n", sout.getvalue())
 

=== modified file 'testtools/tests/test_testresult.py'
--- testtools/tests/test_testresult.py	2010-11-28 12:15:49 +0000
+++ testtools/tests/test_testresult.py	2010-11-29 00:28:14 +0000
@@ -49,8 +49,39 @@
 StringIO = try_imports(['StringIO.StringIO', 'io.StringIO'])
 
 
-class TestTestResultContract(TestCase):
-    """Tests for the contract of TestResults."""
+class Python26Contract(object):
+
+    def test_fresh_result_is_successful(self):
+        # A result is considered successful before any tests are run.
+        result = self.makeResult()
+        self.assertTrue(result.wasSuccessful())
+
+    def test_addError_is_failure(self):
+        # addError fails the test run.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addError(self, an_exc_info)
+        result.stopTest(self)
+        self.assertFalse(result.wasSuccessful())
+
+    def test_addFailure_is_failure(self):
+        # addFailure fails the test run.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addFailure(self, an_exc_info)
+        result.stopTest(self)
+        self.assertFalse(result.wasSuccessful())
+
+    def test_addSuccess_is_success(self):
+        # addSuccess does not fail the test run.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addSuccess(self)
+        result.stopTest(self)
+        self.assertTrue(result.wasSuccessful())
+
+
+class Python27Contract(Python26Contract):
 
     def test_addExpectedFailure(self):
         # Calling addExpectedFailure(test, exc_info) completes ok.
@@ -58,6 +89,52 @@
         result.startTest(self)
         result.addExpectedFailure(self, an_exc_info)
 
+    def test_addExpectedFailure_is_success(self):
+        # addExpectedFailure does not fail the test run.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addExpectedFailure(self, an_exc_info)
+        result.stopTest(self)
+        self.assertTrue(result.wasSuccessful())
+
+    def test_addSkipped(self):
+        # Calling addSkip(test, reason) completes ok.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addSkip(self, _u("Skipped for some reason"))
+
+    def test_addSkip_is_success(self):
+        # addSkip does not fail the test run.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addSkip(self, _u("Skipped for some reason"))
+        result.stopTest(self)
+        self.assertTrue(result.wasSuccessful())
+
+    def test_addUnexpectedSuccess(self):
+        # Calling addUnexpectedSuccess(test) completes ok.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addUnexpectedSuccess(self)
+
+    def test_addUnexpectedSuccess_was_successful(self):
+        # addUnexpectedSuccess does not fail the test run in Python 2.7.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addUnexpectedSuccess(self)
+        result.stopTest(self)
+        self.assertTrue(result.wasSuccessful())
+
+    def test_startStopTestRun(self):
+        # Calling startTestRun completes ok.
+        result = self.makeResult()
+        result.startTestRun()
+        result.stopTestRun()
+
+
+class TestResultContract(Python27Contract):
+    """Tests for the contract of TestResults."""
+
     def test_addExpectedFailure_details(self):
         # Calling addExpectedFailure(test, details=xxx) completes ok.
         result = self.makeResult()
@@ -76,24 +153,12 @@
         result.startTest(self)
         result.addFailure(self, details={})
 
-    def test_addSkipped(self):
-        # Calling addSkip(test, reason) completes ok.
-        result = self.makeResult()
-        result.startTest(self)
-        result.addSkip(self, _u("Skipped for some reason"))
-
     def test_addSkipped_details(self):
         # Calling addSkip(test, reason) completes ok.
         result = self.makeResult()
         result.startTest(self)
         result.addSkip(self, details={})
 
-    def test_addUnexpectedSuccess(self):
-        # Calling addUnexpectedSuccess(test) completes ok.
-        result = self.makeResult()
-        result.startTest(self)
-        result.addUnexpectedSuccess(self)
-
     def test_addUnexpectedSuccess_details(self):
         # Calling addUnexpectedSuccess(test) completes ok.
         result = self.makeResult()
@@ -106,32 +171,57 @@
         result.startTest(self)
         result.addSuccess(self, details={})
 
-    def test_startStopTestRun(self):
-        # Calling startTestRun completes ok.
-        result = self.makeResult()
-        result.startTestRun()
-        result.stopTestRun()
-
-
-class TestTestResultContract(TestTestResultContract):
+    def test_addUnexpectedSuccess_was_successful(self):
+        # addUnexpectedSuccess fails test run in testtools.
+        result = self.makeResult()
+        result.startTest(self)
+        result.addUnexpectedSuccess(self)
+        result.stopTest(self)
+        self.assertFalse(result.wasSuccessful())
+
+    def test_startTestRun_resets_unexpected_success(self):
+        result = self.makeResult()
+        result.startTest(self)
+        result.addUnexpectedSuccess(self)
+        result.stopTest(self)
+        result.startTestRun()
+        self.assertTrue(result.wasSuccessful())
+
+    def test_startTestRun_resets_failure(self):
+        result = self.makeResult()
+        result.startTest(self)
+        result.addFailure(self, an_exc_info)
+        result.stopTest(self)
+        result.startTestRun()
+        self.assertTrue(result.wasSuccessful())
+
+    def test_startTestRun_resets_errors(self):
+        result = self.makeResult()
+        result.startTest(self)
+        result.addError(self, an_exc_info)
+        result.stopTest(self)
+        result.startTestRun()
+        self.assertTrue(result.wasSuccessful())
+
+class TestTestResultContract(TestCase, TestResultContract):
 
     def makeResult(self):
         return TestResult()
 
 
-class TestMultiTestresultContract(TestTestResultContract):
+class TestMultiTestResultContract(TestCase, TestResultContract):
 
     def makeResult(self):
         return MultiTestResult(TestResult(), TestResult())
 
 
-class TestTextTestResultContract(TestTestResultContract):
+class TestTextTestResultContract(TestCase, TestResultContract):
 
     def makeResult(self):
         return TextTestResult(StringIO())
 
 
-class TestThreadSafeForwardingResultContract(TestTestResultContract):
+class TestThreadSafeForwardingResultContract(TestCase, TestResultContract):
 
     def makeResult(self):
         result_semaphore = threading.Semaphore(1)
@@ -139,6 +229,36 @@
         return ThreadsafeForwardingResult(target, result_semaphore)
 
 
+class TestExtendedTestResultContract(TestCase, TestResultContract):
+
+    def makeResult(self):
+        return ExtendedTestResult()
+
+
+class TestPython26TestResultContract(TestCase, Python26Contract):
+
+    def makeResult(self):
+        return Python26TestResult()
+
+
+class TestAdaptedPython26TestResultContract(TestCase, TestResultContract):
+
+    def makeResult(self):
+        return ExtendedToOriginalDecorator(Python26TestResult())
+
+
+class TestPython27TestResultContract(TestCase, Python27Contract):
+
+    def makeResult(self):
+        return Python27TestResult()
+
+
+class TestAdaptedPython27TestResultContract(TestCase, TestResultContract):
+
+    def makeResult(self):
+        return ExtendedToOriginalDecorator(Python27TestResult())
+
+
 class TestTestResult(TestCase):
     """Tests for `TestResult`."""
 
@@ -308,6 +428,12 @@
                 self.fail("yo!")
         return Test("failed")
 
+    def make_unexpectedly_successful_test(self):
+        class Test(TestCase):
+            def succeeded(self):
+                self.expectFailure("yo!", lambda: None)
+        return Test("succeeded")
+
     def make_test(self):
         class Test(TestCase):
             def test(self):
@@ -393,9 +519,18 @@
         self.assertThat(self.getvalue(),
             DocTestMatches("...\n\nFAILED (failures=1)\n", doctest.ELLIPSIS))
 
+    def test_stopTestRun_not_successful_unexpected_success(self):
+        test = self.make_unexpectedly_successful_test()
+        self.result.startTestRun()
+        test.run(self.result)
+        self.result.stopTestRun()
+        self.assertThat(self.getvalue(),
+            DocTestMatches("...\n\nFAILED (failures=1)\n", doctest.ELLIPSIS))
+
     def test_stopTestRun_shows_details(self):
         self.result.startTestRun()
         self.make_erroring_test().run(self.result)
+        self.make_unexpectedly_successful_test().run(self.result)
         self.make_failing_test().run(self.result)
         self.reset_output()
         self.result.stopTestRun()
@@ -428,7 +563,10 @@
     self.fail("yo!")
 AssertionError: yo!
 ------------
-...""", doctest.ELLIPSIS))
+======================================================================
+UNEXPECTED SUCCESS: testtools.tests.test_testresult.Test.succeeded
+----------------------------------------------------------------------
+...""", doctest.ELLIPSIS | doctest.REPORT_NDIFF))
 
 
 class TestThreadSafeForwardingResult(TestWithFakeExceptions):