Merge lp:~gz/bzr/unexpected_successes_are_bad_654474 into lp:bzr

[Martin Packman]

Make test unexpected successes count as failures

Recently a few people have run into a mysterious "FAILED ()" selftest result with no errors reported. This is from using testtools/subunit versions with bug 654474 fixed, and a couple of long-standing unexpected successes in the bzrlib test suite now count as the run failing, but don't give any output.

I did a quick poll about bumping the minimum version of testtools (again), but this branch gets the basics of the handling right without needing that step yet. So, for now they count as and are reported similar to straight failures.

One annoyance is I should really test VerboseTestResult.report_unexpected_success as well, but need to rearrange test_selftest to be a bit more sane and avoid lots of code duplication.

[Andrew Bennetts (spiv)]

Docstring pedantry:

8 + def addUnexpectedSuccess(self, test, details=None):
9 + """Tell result the test unexpectedly passed, counting as a failure

That sentence should end in a period.

I've chatted IRL about the test_smart_add changes, I think I may have a nicer alternative that; I'll put up a branch in a moment.

[Andrew Bennetts (spiv)]

My tweak is at lp:~spiv/bzr/unexpected_successes_are_bad_654474

[Andrew Bennetts (spiv)]

The rest of this looks good to me. If you're happy with my tweak then I'm happy for this to land.

review: Approve

[Martin Packman (gz)]

Tweak seems reasonable to me, I've pulled it into this branch.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-05-16 23:19:29 +0000
+++ bzrlib/tests/__init__.py	2011-05-17 17:06:50 +0000
@@ -449,6 +449,19 @@
         self.known_failure_count += 1
         self.report_known_failure(test, err)
 
+    def addUnexpectedSuccess(self, test, details=None):
+        """Tell result the test unexpectedly passed, counting as a failure
+
+        When the minimum version of testtools required becomes 0.9.8 this
+        can be updated to use the new handling there.
+        """
+        super(ExtendedTestResult, self).addFailure(test, details=details)
+        self.failure_count += 1
+        self.report_unexpected_success(test,
+            "".join(details["reason"].iter_text()))
+        if self.stop_early:
+            self.stop()
+
     def addNotSupported(self, test, feature):
         """The test will not be run because of a missing feature.
         """
@@ -613,6 +626,13 @@
     def report_known_failure(self, test, err):
         pass
 
+    def report_unexpected_success(self, test, reason):
+        self.stream.write('FAIL: %s\n    %s: %s\n' % (
+            self._test_description(test),
+            "Unexpected success. Should have failed",
+            reason,
+            ))
+
     def report_skip(self, test, reason):
         pass
 
@@ -670,6 +690,12 @@
                 % (self._testTimeString(test),
                    self._error_summary(err)))
 
+    def report_unexpected_success(self, test, reason):
+        self.stream.write(' FAIL %s\n%s: %s\n'
+                % (self._testTimeString(test),
+                   "Unexpected success. Should have failed",
+                   reason))
+
     def report_success(self, test):
         self.stream.write('   OK %s\n' % self._testTimeString(test))
         for bench_called, stats in getattr(test, '_benchcalls', []):
 
=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
--- bzrlib/tests/per_workingtree/test_smart_add.py	2011-05-04 21:10:36 +0000
+++ bzrlib/tests/per_workingtree/test_smart_add.py	2011-05-17 17:06:50 +0000
@@ -296,15 +296,25 @@
         self.wt = self.make_branch_and_tree('.')
         self.overrideAttr(osutils, 'normalized_filename')
 
+    def test_requires_normalized_unicode_filenames_fails_on_unnormalized(self):
+        """Adding unnormalized unicode filenames fail if and only if the
+        workingtree format has the requires_normalized_unicode_filenames flag
+        set.
+        """
+        osutils.normalized_filename = osutils._accessible_normalized_filename
+        if self.workingtree_format.requires_normalized_unicode_filenames:
+            self.assertRaises(
+                errors.NoSuchFile, self.wt.smart_add, [u'a\u030a'])
+        else:
+            self.wt.smart_add([u'a\u030a'])
+
     def test_accessible_explicit(self):
         osutils.normalized_filename = osutils._accessible_normalized_filename
         if self.workingtree_format.requires_normalized_unicode_filenames:
-            self.expectFailure(
-                'Working tree format smart_add requires normalized unicode filenames',
-                self.assertRaises, errors.NoSuchFile,
-                self.wt.smart_add, [u'a\u030a'])
-        else:
-            self.wt.smart_add([u'a\u030a'])
+            raise tests.TestNotApplicable(
+                'Working tree format smart_add requires normalized unicode '
+                'filenames')
+        self.wt.smart_add([u'a\u030a'])
         self.wt.lock_read()
         self.addCleanup(self.wt.unlock)
         self.assertEqual([('', 'directory'), (u'\xe5', 'file')],
@@ -314,12 +324,10 @@
     def test_accessible_implicit(self):
         osutils.normalized_filename = osutils._accessible_normalized_filename
         if self.workingtree_format.requires_normalized_unicode_filenames:
-            self.expectFailure(
-                'Working tree format smart_add requires normalized unicode filenames',
-                self.assertRaises, errors.NoSuchFile,
-                self.wt.smart_add, [])
-        else:
-            self.wt.smart_add([])
+            raise tests.TestNotApplicable(
+                'Working tree format smart_add requires normalized unicode '
+                'filenames')
+        self.wt.smart_add([])
         self.wt.lock_read()
         self.addCleanup(self.wt.unlock)
         self.assertEqual([('', 'directory'), (u'\xe5', 'file')],
 
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_selftest.py	2011-05-17 17:06:50 +0000
@@ -1083,6 +1083,24 @@
             '\n'
             'OK \\(known_failures=1\\)\n')
 
+    def test_unexpected_success_bad(self):
+        class Test(tests.TestCase):
+            def test_truth(self):
+                self.expectFailure("No absolute truth", self.assertTrue, True)
+        runner = tests.TextTestRunner(stream=StringIO())
+        result = self.run_test_runner(runner, Test("test_truth"))
+        self.assertContainsRe(runner.stream.getvalue(),
+            "=+\n"
+            "FAIL: \\S+\.test_truth\n"
+            "-+\n"
+            "(?:.*\n)*"
+            "No absolute truth\n"
+            "(?:.*\n)*"
+            "-+\n"
+            "Ran 1 test in .*\n"
+            "\n"
+            "FAILED \\(failures=1\\)\n\\Z")
+
     def test_result_decorator(self):
         # decorate results
         calls = []
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-17 10:30:37 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-17 17:06:50 +0000
@@ -138,6 +138,9 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* A test that was expected to fail but passes instead now counts as a failure
+  catching up with new testtools and subunit handling. (Martin [GZ], #654474)
+
 * Make it easier for plugins to reuse the per_workingtree scenarios by
   restoring the wt_scenarios helper that was accidentally deleted.
   (Vincent Ladeuil, #783472)