Merge lp:~jml/testtools/unprintable-assertThat-804127 into lp:~testtools-committers/testtools/trunk

[Jonathan Lange]

This branch addresses bug 804127, and I believe fixes it. The approach is to raise our own kind of exception, one that actually works with extended unicode information.

In addition, I've added a to_text() method to compat, that is a way of saying 'unicode' in Python 2 and 3.

Points of concern:
 * Using to_text() in test_verbose_unicode but str() in all the other tests feels wrong, but I'm not sure what's right.
 * I haven't changed or better defined the contract for describe(). Is this really a problem?
 * I was surprised that _exception_to_text could have been so broken for Python 3, but it turns out that it wasn't used at all for Python 3. This makes me wonder how we should be actually testing this bug.

[Martin Packman (gz)]

This looks like really the best option, thanks.

+try:
+ to_text = unicode
+except NameError:
+ to_text = str
...
- return unicode(evalue)
+ return to_text(evalue)

I think this is abstraction at the wrong level, Python 3 should just be able to do `str(e)` rather than `_exception_to_text(e)` where you use it, or if we really want the exception catching (doesn't seem needed in the test), add a Python 3 version which would basically be:

    def _exception_to_text(evalue):
        try:
            return str(evalue)
        except Exception:
            return ...some fallback...

+ def __init__(self, matchee, matcher, mismatch, verbose=False):

I'm not sure about having verbosity as an attribute of the exception instance, but guess that's a natural consequence given the way the option was added earlier. Otherwise, the traceback formatting code would be complicated by trying to call something other than unicode()/str() on the exception so a verbose argument could be passed. Which would get very ugly.

+ def __str__(self):
+ difference = self.mismatch.describe()

If we are strict about describe methods always returning unicode, and don't expect MismatchError instances to be handled outside the testtools module, this is okay. Otherwise, it would be better to have both a __unicode__ and __str__ method, with the __str__ method returning ascii on Python 2. I can put up some testcases demonstrating the issues involved.

+ # XXX: Using to_text rather than str because, on Python 2, str will
+ # raise UnicodeEncodeError.

As mentioned above, this may be worth avoiding by having __str__ fall back to ascii on Python 2.

+ if ``self.failureException`` has been set to a non-default value, then
+ mismatches will become test errors rather than test failures.

Not that I've ever seen anyone use this unittest feature, but could the testtools runners at least avoid this by doing `except (self.failureException, MismatchError):` or similar?

review: Approve

[Jonathan Lange (jml)]

On Fri, Aug 5, 2011 at 5:03 PM, Martin [gz] <email address hidden> wrote:
> Review: Approve
> This looks like really the best option, thanks.
>
> +try:
> +    to_text = unicode
> +except NameError:
> +    to_text = str
> ...
> -        return unicode(evalue)
> +        return to_text(evalue)
>
> I think this is abstraction at the wrong level, Python 3 should just be able to do `str(e)` rather than `_exception_to_text(e)` where you use it, or if we really want the exception catching (doesn't seem needed in the test), add a Python 3 version which would basically be:
>
>    def _exception_to_text(evalue):
>        try:
>            return str(evalue)
>        except Exception:
>            return ...some fallback...
>

Yeah, I see your point. I think having that wrapper is the best way,
since that allows us to write tests that verify behaviour. Currently
_exception_to_text is only used in Python 2 anyway.

I still think to_text is useful though :)

> +    def __init__(self, matchee, matcher, mismatch, verbose=False):
>
> I'm not sure about having verbosity as an attribute of the exception instance, but guess that's a natural consequence given the way the option was added earlier. Otherwise, the traceback formatting code would be complicated by trying to call something other than unicode()/str() on the exception so a verbose argument could be passed. Which would get very ugly.

Perhaps I could raise a standard failure in assertThat for
verbose=False. I agree it's a bit of a violation abstraction as is.

>
> +    def __str__(self):
> +        difference = self.mismatch.describe()
>
> If we are strict about describe methods always returning unicode, and don't expect MismatchError instances to be handled outside the testtools module, this is okay. Otherwise, it would be better to have both a __unicode__ and __str__ method, with the __str__ method returning ascii on Python 2. I can put up some testcases demonstrating the issues involved.

Those test cases would be useful, thanks. Even if they are just
copy-and-paste from the REPL.

> +  if ``self.failureException`` has been set to a non-default value, then
> +  mismatches will become test errors rather than test failures.
>
> Not that I've ever seen anyone use this unittest feature, but could the testtools runners at least avoid this by doing `except (self.failureException, MismatchError):` or similar?

Good idea!

Thanks for the great review!

jml

[Martin Packman (gz)]

I've got a little module with tests, the summary of interesting ones is:

    assertThat(_b("\a7"), Equals(_b("\b1")), verbose=True)
Results in a non-ascii byte string for the assertion on Python 2, worse when the bytes include control characters of various sorts. On Python 3 the bytes type ends up in an escaped form when stringified which is much better. All verbose matching with bytes has this issue.

    assertThat(_b("\a7"), MatchesRegex(_b("\b1")))
    assertThat(_b("\a7"), StartsWith(_b("\b1")))
The reverse problem here on Python 3, the matchers are printed as /b'\xb1'/ and 'b'\xb1'' which is not ideal. Python 2 again ends up with a non-ascii assertion.

    assertThat(_u("\a7"), MatchesRegex(_b("\b1")), verbose=True)
Results in an unprintable MismatchError on Python 2.

[Martin Packman (gz)]

See <lp:~gz/+junk/testtools_that> for examples of what failing tests look like.

[lp:~jml/testtools/unprintable-assertThat-804127]

225. By Jonathan Lange on 2011-08-09

Use the right level API to check the error output.

226. By Jonathan Lange on 2011-08-09

Docstring.

227. By Jonathan Lange on 2011-08-11

Take parameters.

228. By Jonathan Lange on 2011-08-11

Merge trunk

[Jonathan Lange (jml)]

IRC discussion about this branch::

<jml> mgz: this bytes/unicode thing is doing my head in
<mgz> ehhee
<mgz> jml: and if you've not read PEP 383 yet, which poolie linked the other day, know that it only gets messiuer
<mgz> -u
<mgz> I think testtools is basically there, on a hard problem, apart from two things
<jml> I haven't.
<mgz> 1) Matcher and Mismatch classes need to not use %s, specifically StartsWith and MatchesRegex
<mgz> 2) __str__ on Python 2 should mangle to ascii rather than return unicode and blow things up
<jml> Ahhh, you *say* that.
<mgz> testtools can be smart and try __unicode__ first, and other if the objects leak into other less careful libraries they won't be harmed
<jml> mgz: you mean something like http://paste.ubuntu.com/663608/?
<mgz> in fact, testtools already is smart.
<mgz> I'd have spelled it differently, but yup, looks about what I was thinking
<mgz> did it cause issues?
<mgz> the only issue remaining from there is to not let non-ascii bytestrings in, which is why StartsWith etc needs chaning
<jml> mgz: yeah
<mgz> *changing
<jml> mgz: even on Equals(), you get an unprintable assertion.
<mgz> that's the describe() return issue.
<mgz> I think it might be best to check the type in that __unicode__ function and do something similar to how bytes() get repr-ed in Python 3, rather than trying to fix every method...
<mgz> hm.
<jml> http://paste.ubuntu.com/663616/ for something to muck around with.
<mgz> yup, that's the kind of assertion that we want to work for people.
<mgz> okay, I'll think a bit and throw some code up
<mgz> ...not as in vomit, though I don't guarentee it won't look like that
<jml> http://paste.ubuntu.com/663619/
<jml> more data
<jml> mgz: heh heh
<jml> mgz: also, I didn't really know how to turn your demo tests into actual tests.
<jml> mgz: just pushed up my latest changes, fixes the _exception_to_text abstraction violation.
<jml> mgz: but, however you choose to project your code onto the internet, it would be much appreciated.
<mgz> yeah, I left the demo file as was because getting into another layer of assertions gets really confusing, it's clearer to just see the output as a testtools user would
<jml> fair enough
<mgz> tests for the things that then make those cases work as intended are writable
<mgz> (what type the describe() method returns on non-ascii mismatches etc)
<jml> cool. I tried naively turning them into tests, and they all passed, which isn't really what we want.
<jml> yeah
<jml> fwiw, https://bugs.launchpad.net/testtools/+bug/686807 might be relevant
* mgz looks
<mgz> yeah, that change could probably just be made, the __str__ methods already return things that should work as object creation code, and str() falls back to repr()

Am now waiting on mgz to provide some code suggestions.

[Martin Packman (gz)]

An update on where I am, as we've failed to connect on IRC a couple of times.

Fixing problem 1) above with %s without changing the current behaviour is a little complex as there are several different variations on the output desired, and also changes across Python implementations even in the trivial case due to different prefixes. So, I got a little stuck trying to write a does-everything fancy repr without breaking the current behaviour. Simply changing everything to %r would work* but lose the breaking up of multiline strings and new regexp backslash behaviour. I'm nearly there with a proper fix, it's just fiddly.

*Mostly. One kind of problem currently is like:

    from testtools import TestCase
    from testtools.matchers import Equals

    class Test(TestCase):

        def test_lost_control(self):
            self.assertThat("\x1b[31;1m", Equals("CSI 32 ; 1 m"), verbose=True)

    if __name__ == "__main__":
        import unittest
        unittest.main()

This sort is particularly fun as C0 codes are perfectly valid in returns from __repr__ functions, so even with strings fixed other objects can still give surprising test result output.

[Jonathan Lange (jml)]

On Fri, Aug 19, 2011 at 10:02 PM, Martin [gz] <email address hidden> wrote:
> An update on where I am, as we've failed to connect on IRC a couple of times.
>

Thanks for the update!

jml

[lp:~jml/testtools/unprintable-assertThat-804127]

229. By Martin Packman on 2011-08-21

Tests for repr function to replace broken stringification schemes in matchers

230. By Martin Packman on 2011-08-22

Correct a couple of the examples for text_repr tests

231. By Martin Packman on 2011-08-22

Messy first pass at implementing text_repr

232. By Martin Packman on 2011-08-22

Use (possibly extended) repr rather than "%s" for matchee in verbose form of MismatchError

233. By Martin Packman on 2011-08-22

Make MismatchError stringification return appropriate types on Python 2

234. By Martin Packman on 2011-08-22

Remove now unused to_text alias for unicode string type from compat

235. By Martin Packman on 2011-08-24

Test and fix _BinaryMismatch.describe long forms using text_repr

236. By Martin Packman on 2011-08-24

Hack over bump with bytes till things can be rewritten in a better way

237. By Martin Packman on 2011-08-24

Use text_repr for DoesNotStartWith and DoesNotEndWith describe methods

238. By Martin Packman on 2011-08-24

Make StartsWith and EndsWith stringify more like other matchers

239. By Martin Packman on 2011-08-24

Fix MatchesRegex mismatch description with a little transcoding dance

240. By Martin Packman on 2011-08-24

Avoid potential bytes and unicode mixing with DocTestMismatch on Python 2

241. By Martin Packman on 2011-09-13

Extra tests to ensure multiline quoting is correct on Python 3

242. By Martin Packman on 2011-09-13

Fix spelling error noted in review by jml

243. By Martin Packman on 2011-09-13

Add cautions about unprintable strings to Mismatch documentation

244. By Martin Packman on 2011-09-13

Add third state to multiline argument of text_repr and comment the internal logic

245. By Martin Packman on 2011-09-13

Test that not passing multiline to text_repr defaults based on input

246. By Jonathan Lange on 2011-09-14

Merge trunk.

=== modified file 'NEWS'
--- NEWS	2011-08-08 11:16:01 +0000
+++ NEWS	2011-08-11 18:28:12 +0000
@@ -14,6 +14,12 @@
   now deprecated.  Please stop using it.
   (Jonathan Lange, #813460)
 
+* ``assertThat`` raises ``MismatchError`` instead of
+  ``TestCase.failureException``.  ``MismatchError`` is a subclass of
+  ``AssertionError``, so in most cases this change will not matter. However,
+  if ``self.failureException`` has been set to a non-default value, then
+  mismatches will become test errors rather than test failures.
+
 * ``gather_details`` takes two dicts, rather than two detailed objects.
   (Jonathan Lange, #801027)
 
@@ -30,7 +36,10 @@
 * All public matchers are now in ``testtools.matchers.__all__``.
   (Jonathan Lange, #784859)
 
-* assertThat output is much less verbose, displaying only what the mismatch
+* ``assertThat`` can actually display mismatches and matchers that contain
+  extended unicode characters. (Jonathan Lange, Martin [gz], #804127)
+
+* ``assertThat`` output is much less verbose, displaying only what the mismatch
   tells us to display. Old-style verbose output can be had by passing
   ``verbose=True`` to assertThat. (Jonathan Lange, #675323, #593190)
 

=== modified file 'scripts/all-pythons'
--- scripts/all-pythons	2011-07-26 22:28:21 +0000
+++ scripts/all-pythons	2011-08-11 18:28:12 +0000
@@ -29,7 +29,9 @@
 ROOT = os.path.dirname(os.path.dirname(__file__))
 
 
-def run_for_python(version, result):
+def run_for_python(version, result, tests):
+    if not tests:
+        tests = ['testtools.tests.test_suite']
     # XXX: This could probably be broken up and put into subunit.
     python = 'python%s' % (version,)
     # XXX: Correct API, but subunit doesn't support it. :(
@@ -58,7 +60,8 @@
     cmd = [
         python,
         '-W', 'ignore:Module testtools was already imported',
-        subunit_path, 'testtools.tests.test_suite']
+        subunit_path]
+    cmd.extend(tests)
     process = subprocess.Popen(
         cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
     _make_stream_binary(process.stdout)
@@ -87,4 +90,4 @@
     sys.path.append(ROOT)
     result = TestProtocolClient(sys.stdout)
     for version in '2.4 2.5 2.6 2.7 3.0 3.1 3.2'.split():
-        run_for_python(version, result)
+        run_for_python(version, result, sys.argv[1:])

=== modified file 'testtools/compat.py'
--- testtools/compat.py	2011-07-26 23:08:51 +0000
+++ testtools/compat.py	2011-08-11 18:28:12 +0000
@@ -143,8 +143,13 @@
                 stream.newlines, stream.line_buffering)
         except AttributeError:
             pass
-    return writer(stream, "replace")    
-
+    return writer(stream, "replace")
+
+
+try:
+    to_text = unicode
+except NameError:
+    to_text = str
 
 # The default source encoding is actually "iso-8859-1" until Python 2.5 but
 # using non-ascii causes a deprecation warning in 2.4 and it's cleaner to

=== modified file 'testtools/matchers.py'
--- testtools/matchers.py	2011-08-08 11:14:01 +0000
+++ testtools/matchers.py	2011-08-11 18:28:12 +0000
@@ -129,6 +129,33 @@
             id(self), self.__dict__)
 
 
+class MismatchError(AssertionError):
+    """Raised when a mismatch occurs."""
+
+    # This class exists to work around
+    # <https://bugs.launchpad.net/testtools/+bug/804127>.  It provides a
+    # guaranteed way of getting a readable exception, no matter what crazy
+    # characters are in the matchee, matcher or mismatch.
+
+    def __init__(self, matchee, matcher, mismatch, verbose=False):
+        # Have to use old-style upcalling for Python 2.4 and 2.5
+        # compatibility.
+        AssertionError.__init__(self)
+        self.matchee = matchee
+        self.matcher = matcher
+        self.mismatch = mismatch
+        self.verbose = verbose
+
+    def __str__(self):
+        difference = self.mismatch.describe()
+        if self.verbose:
+            return (
+                'Match failed. Matchee: "%s"\nMatcher: %s\nDifference: %s\n'
+                % (self.matchee, self.matcher, difference))
+        else:
+            return difference
+
+
 class MismatchDecorator(object):
     """Decorate a ``Mismatch``.
 

=== modified file 'testtools/testcase.py'
--- testtools/testcase.py	2011-07-27 19:47:22 +0000
+++ testtools/testcase.py	2011-08-11 18:28:12 +0000
@@ -29,6 +29,7 @@
     Annotate,
     Equals,
     MatchesException,
+    MismatchError,
     Is,
     Not,
     )
@@ -391,7 +392,7 @@
 
         :param matchee: An object to match with matcher.
         :param matcher: An object meeting the testtools.Matcher protocol.
-        :raises self.failureException: When matcher does not match thing.
+        :raises MismatchError: When matcher does not match thing.
         """
         # XXX: Should this take an optional 'message' parameter? Would kind of
         # make sense. The hamcrest one does.
@@ -406,13 +407,7 @@
                 full_name = "%s-%d" % (name, suffix)
                 suffix += 1
             self.addDetail(full_name, content)
-        if verbose:
-            message = (
-                'Match failed. Matchee: "%s"\nMatcher: %s\nDifference: %s\n'
-                % (matchee, matcher, mismatch.describe()))
-        else:
-            message = mismatch.describe()
-        self.fail(message)
+        raise MismatchError(matchee, matcher, mismatch, verbose)
 
     def defaultTestResult(self):
         return TestResult()

=== modified file 'testtools/tests/test_matchers.py'
--- testtools/tests/test_matchers.py	2011-08-08 11:14:01 +0000
+++ testtools/tests/test_matchers.py	2011-08-11 18:28:12 +0000
@@ -12,6 +12,7 @@
     )
 from testtools.compat import (
     StringIO,
+    to_text,
     _u,
     )
 from testtools.matchers import (
@@ -37,6 +38,7 @@
     MatchesStructure,
     Mismatch,
     MismatchDecorator,
+    MismatchError,
     Not,
     NotEquals,
     Raises,
@@ -62,6 +64,62 @@
         self.assertEqual({}, mismatch.get_details())
 
 
+class TestMismatchError(TestCase):
+
+    def test_is_assertion_error(self):
+        # MismatchError is an AssertionError, so that most of the time, it
+        # looks like a test failure, rather than an error.
+        def raise_mismatch_error():
+            raise MismatchError(2, Equals(3), Equals(3).match(2))
+        self.assertRaises(AssertionError, raise_mismatch_error)
+
+    def test_default_description_is_mismatch(self):
+        mismatch = Equals(3).match(2)
+        e = MismatchError(2, Equals(3), mismatch)
+        self.assertEqual(mismatch.describe(), str(e))
+
+    def test_default_description_unicode(self):
+        matchee = _u('\xa7')
+        matcher = Equals(_u('a'))
+        mismatch = matcher.match(matchee)
+        e = MismatchError(matchee, matcher, mismatch)
+        self.assertEqual(mismatch.describe(), str(e))
+
+    def test_verbose_description(self):
+        matchee = 2
+        matcher = Equals(3)
+        mismatch = matcher.match(2)
+        e = MismatchError(matchee, matcher, mismatch, True)
+        expected = (
+            'Match failed. Matchee: "%s"\n'
+            'Matcher: %s\n'
+            'Difference: %s\n' % (
+                matchee,
+                matcher,
+                matcher.match(matchee).describe(),
+                ))
+        self.assertEqual(expected, str(e))
+
+    def test_verbose_unicode(self):
+        # When assertThat is given matchees or matchers that contain non-ASCII
+        # unicode strings, we can still provide a meaningful error.
+        matchee = _u('\xa7')
+        matcher = Equals(_u('a'))
+        mismatch = matcher.match(matchee)
+        expected = (
+            'Match failed. Matchee: "%s"\n'
+            'Matcher: %s\n'
+            'Difference: %s\n' % (
+                matchee,
+                matcher,
+                mismatch.describe(),
+                ))
+        e = MismatchError(matchee, matcher, mismatch, True)
+        # XXX: Using to_text rather than str because, on Python 2, str will
+        # raise UnicodeEncodeError.
+        self.assertEqual(expected, to_text(e))
+
+
 class TestMatchersInterface(object):
 
     def test_matches_match(self):

=== modified file 'testtools/tests/test_testcase.py'
--- testtools/tests/test_testcase.py	2011-07-26 23:48:48 +0000
+++ testtools/tests/test_testcase.py	2011-08-11 18:28:12 +0000
@@ -18,7 +18,10 @@
     skipUnless,
     testcase,
     )
-from testtools.compat import _b
+from testtools.compat import (
+    _b,
+    _u,
+    )
 from testtools.matchers import (
     Equals,
     MatchesException,
@@ -29,6 +32,7 @@
     Python27TestResult,
     ExtendedTestResult,
     )
+from testtools.testresult.real import TestResult
 from testtools.tests.helpers import (
     an_exc_info,
     LoggingResult,
@@ -482,6 +486,48 @@
         self.assertFails(
             expected, self.assertThat, matchee, matcher, verbose=True)
 
+    def get_error_string(self, e):
+        """Get the string showing how 'e' would be formatted in test output.
+
+        This is a little bit hacky, since it's designed to give consistent
+        output regardless of Python version.
+
+        In testtools, TestResult._exc_info_to_unicode is the point of dispatch
+        between various different implementations of methods that format
+        exceptions, so that's what we have to call. However, that method cares
+        about stack traces and formats the exception class. We don't care
+        about either of these, so we take its output and parse it a little.
+        """
+        error = TestResult()._exc_info_to_unicode((e.__class__, e, None), self)
+        # We aren't at all interested in the traceback.
+        if error.startswith('Traceback (most recent call last):\n'):
+            lines = error.splitlines(True)[1:]
+            for i, line in enumerate(lines):
+                if not line.startswith(' '):
+                    break
+            error = ''.join(lines[i:])
+        # We aren't interested in how the exception type is formatted.
+        exc_class, error = error.split(': ', 1)
+        return error
+
+    def test_assertThat_verbose_unicode(self):
+        # When assertThat is given matchees or matchers that contain non-ASCII
+        # unicode strings, we can still provide a meaningful error.
+        matchee = _u('\xa7')
+        matcher = Equals(_u('a'))
+        expected = (
+            'Match failed. Matchee: "%s"\n'
+            'Matcher: %s\n'
+            'Difference: %s\n\n' % (
+                matchee,
+                matcher,
+                matcher.match(matchee).describe(),
+                ))
+        e = self.assertRaises(
+            self.failureException, self.assertThat, matchee, matcher,
+            verbose=True)
+        self.assertEqual(expected, self.get_error_string(e))
+
     def test_assertEqual_nice_formatting(self):
         message = "These things ought not be equal."
         a = ['apple', 'banana', 'cherry']