Merge lp:~frankban/testrepository/encoding-error into lp:~testrepository/testrepository/trunk

[Francesco Banconi]

== Changes ==

Updated CLITestResult._format_error to handle string encoding.

== Tests ==

$ make check
./testr run --parallel
running=${PYTHON:-python} -m subunit.run --list testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpx1yzlH testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpRdjcPU testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpgSmDzD testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpKFWTY4 testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpIjU0LA testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpVkcDvf testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmp8BjhT1 testrepository.tests.test_suite
running=${PYTHON:-python} -m subunit.run --load-list /tmp/tmpzUqwSe testrepository.tests.test_suite
Ran 264 tests in 1.591s (-0.220s)
PASSED (id=32)

[lp:~frankban/testrepository/encoding-error]

148. By Francesco Banconi on 2012-03-19

New revision

149. By Francesco Banconi on 2012-03-20

New revision.

[Jonathan Lange (jml)]

Hello Francesco,

Thanks for the patch!

I've asked mgz to sanity check the unicode side of this patch, since he understands the many subtle gotchas that can, umm, get you.

Code-wise, I'm not a huge fan of using the short-circuiting properties of 'or', so I'll tweak that before landing.

Also, for future reference, testrepository doesn't require you to use any particular template for submitting merge proposals. In general, we'd (well, I'd) prefer descriptive text that explains what you're changing, why and that highlights any interesting implementation details or areas for particularly close review.

jml

[Jonathan Lange (jml)]

review: Approve

[Martin Packman (gz)]

Two small suggestions for simplification:

* Rather than manually encoding in _format_error, you could wrap the stream instead. There's a helper in testtools.compat called unicode_output_stream that will do this for you:

     def __init__(self, ui, get_id, stream, previous_run=None):
         """Construct a CLITestResult writing to stream."""
         super(CLITestResult, self).__init__(ui, get_id, previous_run)
- self.stream = stream
+ self.stream = unicode_output_stream(stream)
         self.sep1 = u'=' * 70 + '\n'
         self.sep2 = u'-' * 70 + '\n'

* The test looks more complicated than it really deserves. As the StringIO doesn't have an encoding attribute, it just expects the bytes replaced with question marks in the output, all you need to assert that is:

+ error = (ValueError, ValueError("\xa7"), None)
+ result.addFailure(self, error)
+ self.assertThat(
+ stream.getvalue(),
+ DocTestMatches("...ValueError: ?", doctest.ELLIPSIS))

The only complication is how the bytes get decoded depends on the locale the test in run in, to avoid that you could use an exception class that defined __unicode__ and returns something consistent.

review: Approve

[lp:~frankban/testrepository/encoding-error]

150. By Francesco Banconi on 2012-03-29

Changes suggested in review.

[Francesco Banconi (frankban)]

Thank you Jonathan and Martin for your suggestions.
I didn't know about the unicode_output_stream function in testtools, nice hint.
I've added a revision following suggestions in Martin's review. However, I'm a bit confused about the locale issue.
As you said, we are using a cStringIO object as stream, and, so I'd expect the output to be just encoded using 'ascii' and 'replace', so maybe I am missing something.

[Jonathan Lange (jml)]

Presumably there exists a locale where \xa7 will be decoded correctly without needing to fall back to using the replacement character. In those locales, this test will fail.

Here's something that's a bit more robust:

=== modified file 'testrepository/tests/ui/test_cli.py'
--- testrepository/tests/ui/test_cli.py 2012-03-29 10:58:30 +0000
+++ testrepository/tests/ui/test_cli.py 2012-04-02 12:10:54 +0000
@@ -282,8 +282,11 @@ class TestCLITestResult(TestCase):
         # characters.
         stream = StringIO()
         result = self.make_result(stream)
- error = (ValueError, ValueError('\xa7'), None)
+ class MyError(ValueError):
+ def __unicode__(self):
+ return u'\u201c'
+ error = (MyError, MyError(), None)
         result.addFailure(self, error)
         self.assertThat(
             stream.getvalue(),
- DocTestMatches("...ValueError: ?", doctest.ELLIPSIS))
+ DocTestMatches("...MyError: ?", doctest.ELLIPSIS))

I've applied this to your branch, and am about to merge it into trunk.

Thanks so much for your contribution.

jml

review: Approve

=== modified file 'testrepository/tests/ui/test_cli.py'
--- testrepository/tests/ui/test_cli.py	2011-11-04 20:30:39 +0000
+++ testrepository/tests/ui/test_cli.py	2012-03-29 11:04:19 +0000
@@ -266,7 +266,7 @@
             DocTestMatches(result._format_error('ERROR', self, error_text)))
 
     def test_addFailure_outputs_failure(self):
-        # CLITestResult.addError outputs the given error immediately to the
+        # CLITestResult.addFailure outputs the given error immediately to the
         # stream.
         stream = StringIO()
         result = self.make_result(stream)
@@ -276,3 +276,14 @@
         self.assertThat(
             stream.getvalue(),
             DocTestMatches(result._format_error('FAIL', self, error_text)))
+
+    def test_addFailure_handles_string_encoding(self):
+        # CLITestResult.addFailure outputs the given error handling non-ascii
+        # characters.
+        stream = StringIO()
+        result = self.make_result(stream)
+        error = (ValueError, ValueError('\xa7'), None)
+        result.addFailure(self, error)
+        self.assertThat(
+            stream.getvalue(),
+            DocTestMatches("...ValueError: ?", doctest.ELLIPSIS))

=== modified file 'testrepository/ui/cli.py'
--- testrepository/ui/cli.py	2011-11-04 20:30:39 +0000
+++ testrepository/ui/cli.py	2012-03-29 11:04:19 +0000
@@ -1,11 +1,11 @@
 #
 # Copyright (c) 2009 Testrepository Contributors
-# 
+#
 # Licensed under either the Apache License, Version 2.0 or the BSD 3-clause
 # license at the users choice. A copy of both licenses are available in the
 # project source as Apache-2.0 and BSD. You may not use this file except in
 # compliance with one of these two licences.
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under these licenses is distributed on an "AS IS" BASIS, WITHOUT
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
@@ -19,6 +19,8 @@
 import signal
 import sys
 
+from testtools.compat import unicode_output_stream
+
 from testrepository import ui
 
 
@@ -28,7 +30,7 @@
     def __init__(self, ui, get_id, stream, previous_run=None):
         """Construct a CLITestResult writing to stream."""
         super(CLITestResult, self).__init__(ui, get_id, previous_run)
-        self.stream = stream
+        self.stream = unicode_output_stream(stream)
         self.sep1 = u'=' * 70 + '\n'
         self.sep2 = u'-' * 70 + '\n'