Bazaar

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

  1. escape_selftest_console_output_633216
  2. Merge into bzr.dev
Proposed by Martin Packman
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5551
Proposed branch: lp:~gz/bzr/escape_selftest_console_output_633216
Merge into: lp:bzr
Diff against target: 73 lines (+31/-3)
3 files modified
bzrlib/tests/__init__.py (+6/-3)
bzrlib/tests/test_selftest.py (+17/-0)
doc/en/release-notes/bzr-2.3.txt (+8/-0)
To merge this branch: bzr merge lp:~gz/bzr/escape_selftest_console_output_633216
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Information
Martin Pool Approve
Review via email: mp+34890@code.launchpad.net

Commit message

Backslash escape selftest output when printing to non-unicode consoles

Description of the change

Simple change to make selftest output escaped rather than breaking the run with a UnicodeEncodeError when on a console that can't encode a given unicode string. Hasn't been much of a problem so far as selftest has tended to print pre-encoded junk bytes, but testtools changes mean that unicode will now be better preserved, and some future fixes to selftest will also help.

The test overrides osutils.get_terminal_encoding but TextTestWriter could switching to look at the encoding attribute of the stream rather than calling that in future.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

nice, thanks.

news entry please.

review: Approve
Revision history for this message
Martin Packman (gz) wrote :

I was trying to write a testcase that didn't require testtools > 0.9.4 but looking at it again this will clearly break older versions. Should I skip the test on older versions, or give in and bump the selftest requirement?

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> Martin [gz] <email address hidden> writes:

    > I was trying to write a testcase that didn't require testtools >
    > 0.9.4 but looking at it again this will clearly break older
    > versions. Should I skip the test on older versions, or give in and
    > bump the selftest requirement?

bump

We may need to update pqm for landing it though.

But since we are fixing long standing bugs, I don't see how to avoid
that.

Revision history for this message
Martin Packman (gz) wrote :

Added NEWS entry as request, also put up lp:~gz/bzr/require_testtools_0.9.5_for_selftest which should be viewed as a prerequisite to this branch.

Revision history for this message
Andrew Bennetts (spiv) wrote :

There's a trivial typo, a missing double-quote at the end of a line:

+ "Text attachment: log\n"

The new test doesn't seem to pass for me (when I cherrypick this change into the 2.2 branch), but on the other hand the selftest run does appear to complete...

The approach seems reasonable, though.

Revision history for this message
Martin Packman (gz) wrote :

> There's a trivial typo, a missing double-quote at the end of a line:
>
> + "Text attachment: log\n"

Well spotted, I presume I must have added the line wrapping after running the test then not run it again before pushing.

> The new test doesn't seem to pass for me (when I cherrypick this change into
> the 2.2 branch), but on the other hand the selftest run does appear to
> complete...

Can you paste the exact failure? Also, what version of testtools do you have?

Revision history for this message
Vincent Ladeuil (vila) wrote :

@spiv: did you sort out why the test was failing for you ?
@gz: Are we still waiting for testtools to be upgraded on pqm for this mp too ?

review: Needs Information
Revision history for this message
Martin Packman (gz) wrote :

Yes, this does need a newer testtools on PQM as well. If that's going to be stuck for a long time yet, I could rewrite the test to skip if the version is too low.

Revision history for this message
Martin Pool (mbp) wrote :

I think we can upgrade testtools. Is there a release, ideally a packaged release, with the changes needed?

Revision history for this message
Martin Packman (gz) wrote :

See lp:~gz/bzr/require_testtools_0.9.5_for_selftest for details Martin, but either of the last two testtools releases would do, and Vincent has filed an rt we're waiting on.

Revision history for this message
Martin Packman (gz) wrote :

Have merged lp:~gz/bzr/require_testtools_0.9.5_for_selftest into this branch as the test requires it and news was going to conflict anyway. Once that lands, this should also be good to go.

Revision history for this message
Martin Packman (gz) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/__init__.py'
2--- bzrlib/tests/__init__.py 2010-10-18 17:06:37 +0000
3+++ bzrlib/tests/__init__.py 2010-11-19 23:07:43 +0000
4@@ -55,8 +55,8 @@
5 import testtools
6 # nb: check this before importing anything else from within it
7 _testtools_version = getattr(testtools, '__version__', ())
8-if _testtools_version < (0, 9, 2):
9- raise ImportError("need at least testtools 0.9.2: %s is %r"
10+if _testtools_version < (0, 9, 5):
11+ raise ImportError("need at least testtools 0.9.5: %s is %r"
12 % (testtools.__file__, _testtools_version))
13 from testtools import content
14
15@@ -651,7 +651,10 @@
16 encode = codec[0]
17 else:
18 encode = codec.encode
19- stream = osutils.UnicodeOrBytesToBytesWriter(encode, stream)
20+ # GZ 2010-09-08: Really we don't want to be writing arbitrary bytes,
21+ # so should swap to the plain codecs.StreamWriter
22+ stream = osutils.UnicodeOrBytesToBytesWriter(encode, stream,
23+ "backslashreplace")
24 stream.encoding = new_encoding
25 self.stream = stream
26 self.descriptions = descriptions
27
28=== modified file 'bzrlib/tests/test_selftest.py'
29--- bzrlib/tests/test_selftest.py 2010-10-15 11:20:45 +0000
30+++ bzrlib/tests/test_selftest.py 2010-11-19 23:07:43 +0000
31@@ -1282,6 +1282,23 @@
32 result = self.run_test_runner(runner, test)
33 self.assertLength(1, calls)
34
35+ def test_unicode_test_output_on_ascii_stream(self):
36+ """Showing results should always succeed even on an ascii console"""
37+ class FailureWithUnicode(tests.TestCase):
38+ def test_log_unicode(self):
39+ self.log(u"\u2606")
40+ self.fail("Now print that log!")
41+ out = StringIO()
42+ self.overrideAttr(osutils, "get_terminal_encoding",
43+ lambda trace=False: "ascii")
44+ result = self.run_test_runner(tests.TextTestRunner(stream=out),
45+ FailureWithUnicode("test_log_unicode"))
46+ self.assertContainsRe(out.getvalue(),
47+ "Text attachment: log\n"
48+ "-+\n"
49+ "\d+\.\d+ \\\\u2606\n"
50+ "-+\n")
51+
52
53 class SampleTestCase(tests.TestCase):
54
55
56=== modified file 'doc/en/release-notes/bzr-2.3.txt'
57--- doc/en/release-notes/bzr-2.3.txt 2010-11-18 19:38:28 +0000
58+++ doc/en/release-notes/bzr-2.3.txt 2010-11-19 23:07:43 +0000
59@@ -103,6 +103,14 @@
60 Instead, use '...' as a wildcard if you don't care about the output.
61 (Martin Pool, #637830)
62
63+* Bump minimum testtools version required to run ``bzr selftest`` from 0.9.2
64+ to 0.9.5 which will allow tests that need the fixed unicode handling to be
65+ written. (Martin [gz])
66+
67+* Printing selftest results to a non-UTF-8 console will now escape characters
68+ that can't be encoded rather than aborting the test run with an exception.
69+ (Martin [gz], #633216)
70+
71
72 bzr 2.3b3
73 #########