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
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2010-10-18 17:06:37 +0000
+++ bzrlib/tests/__init__.py 2010-11-19 23:07:43 +0000
@@ -55,8 +55,8 @@
55import testtools55import testtools
56# nb: check this before importing anything else from within it56# nb: check this before importing anything else from within it
57_testtools_version = getattr(testtools, '__version__', ())57_testtools_version = getattr(testtools, '__version__', ())
58if _testtools_version < (0, 9, 2):58if _testtools_version < (0, 9, 5):
59 raise ImportError("need at least testtools 0.9.2: %s is %r"59 raise ImportError("need at least testtools 0.9.5: %s is %r"
60 % (testtools.__file__, _testtools_version))60 % (testtools.__file__, _testtools_version))
61from testtools import content61from testtools import content
6262
@@ -651,7 +651,10 @@
651 encode = codec[0]651 encode = codec[0]
652 else:652 else:
653 encode = codec.encode653 encode = codec.encode
654 stream = osutils.UnicodeOrBytesToBytesWriter(encode, stream)654 # GZ 2010-09-08: Really we don't want to be writing arbitrary bytes,
655 # so should swap to the plain codecs.StreamWriter
656 stream = osutils.UnicodeOrBytesToBytesWriter(encode, stream,
657 "backslashreplace")
655 stream.encoding = new_encoding658 stream.encoding = new_encoding
656 self.stream = stream659 self.stream = stream
657 self.descriptions = descriptions660 self.descriptions = descriptions
658661
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py 2010-10-15 11:20:45 +0000
+++ bzrlib/tests/test_selftest.py 2010-11-19 23:07:43 +0000
@@ -1282,6 +1282,23 @@
1282 result = self.run_test_runner(runner, test)1282 result = self.run_test_runner(runner, test)
1283 self.assertLength(1, calls)1283 self.assertLength(1, calls)
12841284
1285 def test_unicode_test_output_on_ascii_stream(self):
1286 """Showing results should always succeed even on an ascii console"""
1287 class FailureWithUnicode(tests.TestCase):
1288 def test_log_unicode(self):
1289 self.log(u"\u2606")
1290 self.fail("Now print that log!")
1291 out = StringIO()
1292 self.overrideAttr(osutils, "get_terminal_encoding",
1293 lambda trace=False: "ascii")
1294 result = self.run_test_runner(tests.TextTestRunner(stream=out),
1295 FailureWithUnicode("test_log_unicode"))
1296 self.assertContainsRe(out.getvalue(),
1297 "Text attachment: log\n"
1298 "-+\n"
1299 "\d+\.\d+ \\\\u2606\n"
1300 "-+\n")
1301
12851302
1286class SampleTestCase(tests.TestCase):1303class SampleTestCase(tests.TestCase):
12871304
12881305
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2010-11-18 19:38:28 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2010-11-19 23:07:43 +0000
@@ -103,6 +103,14 @@
103 Instead, use '...' as a wildcard if you don't care about the output.103 Instead, use '...' as a wildcard if you don't care about the output.
104 (Martin Pool, #637830)104 (Martin Pool, #637830)
105105
106* Bump minimum testtools version required to run ``bzr selftest`` from 0.9.2
107 to 0.9.5 which will allow tests that need the fixed unicode handling to be
108 written. (Martin [gz])
109
110* Printing selftest results to a non-UTF-8 console will now escape characters
111 that can't be encoded rather than aborting the test run with an exception.
112 (Martin [gz], #633216)
113
106114
107bzr 2.3b3115bzr 2.3b3
108#########116#########