Merge lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting into lp:zope.testing

[Māris Fogels]

Hello,

This branch fixes a problem in zope.testing's subunit support. Instead of raising an Exception when the traceback contains UTF8-encoded characters we try to handle the text in a robust way. See bug 591309 for the details.

We assume the traceback is in utf-8, as that is the encoding most people would use for a doctest or Python module, but just in case, I included a test for handling other codecs.

This entire patch would be unnecessary if the traceback were guaranteed to be a unicode object. Unfortunately, I do not know the entry points from which a bytestring can be passed into the formatter, so this defensive code feels safer than risking a testrunner to crash. The unicode-type test makes the code forward-compatible.

I exposed the SubunitOutputFormatter's 'stream' attribute to facilitate testing.

Maris

[lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting]

378. By Māris Fogels on 2010-06-08

Removed an unused variable

[Sidnei da Silva (sidnei)]

Looks good to me. +1!

review: Approve

[Tres Seaver (tseaver)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Māris Fogels wrote:
> Māris Fogels has proposed merging lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting into lp:zope.testing.
>
> Requested reviews:
> ZTK steering group (ztk-steering-group)
> Related bugs:
> #591309 Crash when subunit reports test failures containing UTF8-encoded data
> https://bugs.launchpad.net/bugs/591309
>
>
> Hello,
>
> This branch fixes a problem in zope.testing's subunit support.
> Instead of raising an Exception when the traceback contains
> UTF8-encoded characters we try to handle the text in a robust way.
> See bug 591309 for the details.
>
> We assume the traceback is in utf-8, as that is the encoding most
> people would use for a doctest or Python module, but just in case, I
> included a test for handling other codecs.
>
> This entire patch would be unnecessary if the traceback were
> guaranteed to be a unicode object. Unfortunately, I do not know the
> entry points from which a bytestring can be passed into the
> formatter, so this defensive code feels safer than risking a
> testrunner to crash. The unicode-type test makes the code
> forward-compatible.
>
> I exposed the SubunitOutputFormatter's 'stream' attribute to
> facilitate testing.

I'm -1 on doing any further work on the subunit stuff on the trunk until
we clean up the testing story: at the moment, the features are
completely untested in 95% of the developement environments where folks
work on zope.testing / zope.testrunner, because subunit cannot be
installed in them.

I have a branch merge pending to add distutils support to subunit for
the Python libraries and scripts, which would make releasing them to
PyPI as an sdist feasible. Once that occurs, we can make the
'subunit-python' egg a testing dependency for zope.testing /
zope.testrunner, and ensure that the features don't bitrot.

Tres.
- --
===================================================================
Tres Seaver [phone] <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkwPrYQACgkQ+gerLs4ltQ7RUACfc/7DzxUKL8m0eVOPgY70vRdf
AzgAn3vm4VNvVyTlaEvqI/LWdEAl9GT0
=9Lzj
-----END PGP SIGNATURE-----

[Māris Fogels (mars)]

> I'm -1 on doing any further work on the subunit stuff on the trunk until
> we clean up the testing story: at the moment, the features are
> completely untested in 95% of the developement environments where folks
> work on zope.testing / zope.testrunner, because subunit cannot be
> installed in them.
>
> I have a branch merge pending to add distutils support to subunit for
> the Python libraries and scripts, which would make releasing them to
> PyPI as an sdist feasible. Once that occurs, we can make the
> 'subunit-python' egg a testing dependency for zope.testing /
> zope.testrunner, and ensure that the features don't bitrot.
>
>
> Tres.

Tres, I agree, but does "any further work" include bugfixes to the subunit code? I have spent weeks hunting these bugs down. It would be a pity to lose them. (I may have more fixes coming, too.)

Maris

[Robert Collins (lifeless)]

I don't think it makes sense to hold back things that devs with a working environment can validate simply because other devs *might* cause regressions. Yes, we should get the zope.testing buildbot testing subunit, but the lack of that isn't a reason to stop fixing code that someone can run a test on.

[Māris Fogels (mars)]

I'm working on a fix to make the subunit support conditional.

[Māris Fogels (mars)]

I used virtualenv to compare the results of running the suite with and without subunit installed. The suite skips the test_subunit module when subunit is missing, and includes it when subunit is present. This is the proper behaviour.

The branch should be good to land as-is. If desired, I can include a comment in the docstring of the test_subunit module that states that module is run conditionally.

[Jonathan Lange (jml)]

Tres, now that subunit has a release on PyPI (see http://pypi.python.org/pypi/python-subunit), can this patch please be landed?

[Benji York (benji)]

The copyright line in the new module (src/zope/testing/testrunner/test_subunit.py) should have "2010" as the date. Other than that this change looks good.

review: Approve

[lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting]

379. By Māris Fogels on 2010-07-19

Changed the copyright to 2010.

[Māris Fogels (mars)]

On Mon, Jul 19, 2010 at 9:27 AM, Benji York <email address hidden> wrote:
> Review: Approve
> The copyright line in the new module (src/zope/testing/testrunner/test_subunit.py) should have "2010" as the date.  Other than that this change looks good.

Fixed.

[Benji York (benji)]

I merged this in in r114856.

I added at test dependency for python-subunit on the principal that testing more is better than less as long as the test dependencies aren't too evil, and python-subunit doesn't seem evil.

=== modified file 'src/zope/testing/testrunner/formatter.py'
--- src/zope/testing/testrunner/formatter.py	2010-06-03 16:55:38 +0000
+++ src/zope/testing/testrunner/formatter.py	2010-07-19 20:51:10 +0000
@@ -711,14 +711,18 @@
     TAG_THREADS = 'zope:threads'
     TAG_REFCOUNTS = 'zope:refcounts'
 
-    def __init__(self, options):
+    def __init__(self, options, stream=None):
         if subunit is None:
             raise Exception("Requires subunit 0.0.5 or better")
         if content is None:
             raise Exception("Requires testtools 0.9.2 or better")
         self.options = options
-        self._stream = sys.stdout
+
+        if stream is None:
+            stream = sys.stdout
+        self._stream = stream
         self._subunit = subunit.TestProtocolClient(self._stream)
+
         # Used to track the last layer that was set up or torn down. Either
         # None or (layer_name, last_touched_time).
         self._last_layer = None
@@ -912,9 +916,18 @@
         # create an object equivalent to an instance of 'TracebackContent'.
         formatter = OutputFormatter(None)
         traceback = formatter.format_traceback(exc_info)
+
+        # We have no idea if the traceback is a unicode object or a bytestring
+        # with non-ASCII characters.  We had best be careful when handling it.
+        if isinstance(traceback, unicode):
+            unicode_tb = traceback
+        else:
+            # Assume the traceback was utf-8 encoded, but still be careful.
+            unicode_tb = traceback.decode('utf8', 'replace')
+
         return {
             'traceback': content.Content(
-                self.TRACEBACK_CONTENT_TYPE, lambda: [traceback.encode('utf8')])}
+                self.TRACEBACK_CONTENT_TYPE, lambda: [unicode_tb.encode('utf8')])}
 
     def test_error(self, test, seconds, exc_info):
         """Report that an error occurred while running a test.

=== added file 'src/zope/testing/testrunner/test_subunit.py'
--- src/zope/testing/testrunner/test_subunit.py	1970-01-01 00:00:00 +0000
+++ src/zope/testing/testrunner/test_subunit.py	2010-07-19 20:51:10 +0000
@@ -0,0 +1,59 @@
+##############################################################################
+#
+# Copyright (c) 2004-2010 Zope Foundation and Contributors.
+# All Rights Reserved.
+#
+# This software is subject to the provisions of the Zope Public License,
+# Version 2.1 (ZPL).  A copy of the ZPL should accompany this distribution.
+# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
+# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
+# FOR A PARTICULAR PURPOSE.
+#
+##############################################################################
+"""Unit tests for the testrunner's subunit integration.
+"""
+
+
+import sys
+import unittest
+import formatter
+import subunit
+from StringIO import StringIO
+
+
+class TestSubunitTracebackPrinting(unittest.TestCase):
+
+    def makeByteStringFailure(self, text, encoding):
+        try:
+            # Note that this deliberately throws a string of bytes instead
+            # of a unicode object.  This simulates errors thrown by
+            # utf8-encoded doctests.
+            bytestr = text.encode(encoding)
+            self.fail(bytestr)
+        except self.failureException:
+            return sys.exc_info()
+
+    def setUp(self):
+        class FormatterOptions:
+            verbose=False
+        options = FormatterOptions()
+
+        self.output = StringIO()
+        self.subunit_formatter = formatter.SubunitOutputFormatter(
+            options, stream=self.output)
+
+    def test_print_failure_containing_utf8_bytestrings(self):
+        exc_info = self.makeByteStringFailure(unichr(6514), 'utf8')
+        self.subunit_formatter.test_failure(self, 0, exc_info)
+        assert "AssertionError: \xe1\xa5\xb2" in self.output.getvalue()
+
+    def test_print_error_containing_utf8_bytestrings(self):
+        exc_info = self.makeByteStringFailure(unichr(6514), 'utf8')
+        self.subunit_formatter.test_error(self, 0, exc_info)
+        assert "AssertionError: \xe1\xa5\xb2" in self.output.getvalue()
+
+    def test_print_failure_containing_latin1_bytestrings(self):
+        exc_info = self.makeByteStringFailure(unichr(241), 'latin1')
+        self.subunit_formatter.test_failure(self, 0, exc_info)
+        assert "AssertionError: \xef\xbf\xbd0" in self.output.getvalue()
\ No newline at end of file

=== modified file 'src/zope/testing/testrunner/tests.py'
--- src/zope/testing/testrunner/tests.py	2010-06-03 16:55:38 +0000
+++ src/zope/testing/testrunner/tests.py	2010-07-19 20:51:10 +0000
@@ -296,4 +296,8 @@
                     optionflags=doctest.ELLIPSIS + doctest.NORMALIZE_WHITESPACE,
                     checker=checker))
 
+        suites.append(
+            unittest.defaultTestLoader.loadTestsFromName(
+                'zope.testing.testrunner.test_subunit'))
+
     return unittest.TestSuite(suites)