Code review comment for lp:~jml/testtools/unprintable-assertThat-804127

Revision history for this message
Jonathan Lange (jml) wrote :

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.

« Back to merge proposal