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

Proposed by Māris Fogels on 2010-06-08
Status: Merged
Merge reported by: Benji York
Merged at revision: not available
Proposed branch: lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting
Merge into: lp:zope.testing
Diff against target: 122 lines (+79/-3)
3 files modified
src/zope/testing/testrunner/formatter.py (+16/-3)
src/zope/testing/testrunner/test_subunit.py (+59/-0)
src/zope/testing/testrunner/tests.py (+4/-0)
To merge this branch: bzr merge lp:~mars/zope.testing/fix-subunit-utf8-traceback-reporting
Reviewer Review Type Date Requested Status
Benji York (community) Approve on 2010-07-19
Sidnei da Silva 2010-06-08 Approve on 2010-06-08
Review via email: mp+27086@code.launchpad.net

Description of the change

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

To post a comment you must log in.
378. By Māris Fogels on 2010-06-08

Removed an unused variable

Sidnei da Silva (sidnei) wrote :

Looks good to me. +1!

review: Approve
Tres Seaver (tseaver) wrote :

-----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 +1 540-429-0999 <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) wrote :

> 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) wrote :

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) wrote :

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

Māris Fogels (mars) wrote :

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) wrote :

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) wrote :

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
379. By Māris Fogels on 2010-07-19

Changed the copyright to 2010.

Māris Fogels (mars) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/zope/testing/testrunner/formatter.py'
2--- src/zope/testing/testrunner/formatter.py 2010-06-03 16:55:38 +0000
3+++ src/zope/testing/testrunner/formatter.py 2010-07-19 20:51:10 +0000
4@@ -711,14 +711,18 @@
5 TAG_THREADS = 'zope:threads'
6 TAG_REFCOUNTS = 'zope:refcounts'
7
8- def __init__(self, options):
9+ def __init__(self, options, stream=None):
10 if subunit is None:
11 raise Exception("Requires subunit 0.0.5 or better")
12 if content is None:
13 raise Exception("Requires testtools 0.9.2 or better")
14 self.options = options
15- self._stream = sys.stdout
16+
17+ if stream is None:
18+ stream = sys.stdout
19+ self._stream = stream
20 self._subunit = subunit.TestProtocolClient(self._stream)
21+
22 # Used to track the last layer that was set up or torn down. Either
23 # None or (layer_name, last_touched_time).
24 self._last_layer = None
25@@ -912,9 +916,18 @@
26 # create an object equivalent to an instance of 'TracebackContent'.
27 formatter = OutputFormatter(None)
28 traceback = formatter.format_traceback(exc_info)
29+
30+ # We have no idea if the traceback is a unicode object or a bytestring
31+ # with non-ASCII characters. We had best be careful when handling it.
32+ if isinstance(traceback, unicode):
33+ unicode_tb = traceback
34+ else:
35+ # Assume the traceback was utf-8 encoded, but still be careful.
36+ unicode_tb = traceback.decode('utf8', 'replace')
37+
38 return {
39 'traceback': content.Content(
40- self.TRACEBACK_CONTENT_TYPE, lambda: [traceback.encode('utf8')])}
41+ self.TRACEBACK_CONTENT_TYPE, lambda: [unicode_tb.encode('utf8')])}
42
43 def test_error(self, test, seconds, exc_info):
44 """Report that an error occurred while running a test.
45
46=== added file 'src/zope/testing/testrunner/test_subunit.py'
47--- src/zope/testing/testrunner/test_subunit.py 1970-01-01 00:00:00 +0000
48+++ src/zope/testing/testrunner/test_subunit.py 2010-07-19 20:51:10 +0000
49@@ -0,0 +1,59 @@
50+##############################################################################
51+#
52+# Copyright (c) 2004-2010 Zope Foundation and Contributors.
53+# All Rights Reserved.
54+#
55+# This software is subject to the provisions of the Zope Public License,
56+# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution.
57+# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
58+# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
59+# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
60+# FOR A PARTICULAR PURPOSE.
61+#
62+##############################################################################
63+"""Unit tests for the testrunner's subunit integration.
64+"""
65+
66+
67+import sys
68+import unittest
69+import formatter
70+import subunit
71+from StringIO import StringIO
72+
73+
74+class TestSubunitTracebackPrinting(unittest.TestCase):
75+
76+ def makeByteStringFailure(self, text, encoding):
77+ try:
78+ # Note that this deliberately throws a string of bytes instead
79+ # of a unicode object. This simulates errors thrown by
80+ # utf8-encoded doctests.
81+ bytestr = text.encode(encoding)
82+ self.fail(bytestr)
83+ except self.failureException:
84+ return sys.exc_info()
85+
86+ def setUp(self):
87+ class FormatterOptions:
88+ verbose=False
89+ options = FormatterOptions()
90+
91+ self.output = StringIO()
92+ self.subunit_formatter = formatter.SubunitOutputFormatter(
93+ options, stream=self.output)
94+
95+ def test_print_failure_containing_utf8_bytestrings(self):
96+ exc_info = self.makeByteStringFailure(unichr(6514), 'utf8')
97+ self.subunit_formatter.test_failure(self, 0, exc_info)
98+ assert "AssertionError: \xe1\xa5\xb2" in self.output.getvalue()
99+
100+ def test_print_error_containing_utf8_bytestrings(self):
101+ exc_info = self.makeByteStringFailure(unichr(6514), 'utf8')
102+ self.subunit_formatter.test_error(self, 0, exc_info)
103+ assert "AssertionError: \xe1\xa5\xb2" in self.output.getvalue()
104+
105+ def test_print_failure_containing_latin1_bytestrings(self):
106+ exc_info = self.makeByteStringFailure(unichr(241), 'latin1')
107+ self.subunit_formatter.test_failure(self, 0, exc_info)
108+ assert "AssertionError: \xef\xbf\xbd0" in self.output.getvalue()
109\ No newline at end of file
110
111=== modified file 'src/zope/testing/testrunner/tests.py'
112--- src/zope/testing/testrunner/tests.py 2010-06-03 16:55:38 +0000
113+++ src/zope/testing/testrunner/tests.py 2010-07-19 20:51:10 +0000
114@@ -296,4 +296,8 @@
115 optionflags=doctest.ELLIPSIS + doctest.NORMALIZE_WHITESPACE,
116 checker=checker))
117
118+ suites.append(
119+ unittest.defaultTestLoader.loadTestsFromName(
120+ 'zope.testing.testrunner.test_subunit'))
121+
122 return unittest.TestSuite(suites)

Subscribers

People subscribed via source and target branches