Closed pytestbot closed 9 years ago
Original comment by Carl Meyer (BitBucket: carljm, GitHub: carljm):
Here is a sample repr (of a serializer object from django-rest-framework) which causes the issue:
UserSerializer(data={'timezone': 'America/Denver'}, instance=<User: Test User>):
id = ReadOnlyField()
url = HyperlinkedIdentityField(view_name='user-detail')
name = CharField(style={'base_template': 'textarea.html'})
email = EmailField(max_length=256, validators=[<UniqueValidator(queryset=User.objects.all())>])
phone = CharField(allow_blank=True, allow_null=True, required=False, style={'base_template': 'textarea.html'})
city = CharField(allow_blank=True, required=False, style={'base_template': 'textarea.html'})
state = ChoiceField(allow_blank=True, choices=[(u'AL', u'Alabama'), (u'AK', u'Alaska'), (u'AZ', u'Arizona'), (u'AR', u'Arkansas'), (u'CA', u'California'), (u'CO', u'Colorado'), (u'CT', u'Connecticut'), (u'DE', u'Delaware'), (u'DC', u'District of Columbia'), (u'FL', u'Florida'), (u'GA', u'Georgia'), (u'HI', u'Hawaii'), (u'ID', u'Idaho'), (u'IL', u'Illinois'), (u'IN', u'Indiana'), (u'IA', u'Iowa'), (u'KS', u'Kansas'), (u'KY', u'Kentucky'), (u'LA', u'Louisiana'), (u'ME', u'Maine'), (u'MD', u'Maryland'), (u'MA', u'Massachusetts'), (u'MI', u'Michigan'), (u'MN', u'Minnesota'), (u'MS', u'Mississippi'), (u'MO', u'Missouri'), (u'MT', u'Montana'), (u'NE', u'Nebraska'), (u'NV', u'Nevada'), (u'NH', u'New Hampshire'), (u'NJ', u'New Jersey'), (u'NM', u'New Mexico'), (u'NY', u'New York'), (u'NC', u'North Carolina'), (u'ND', u'North Dakota'), (u'OH', u'Ohio'), (u'OK', u'Oklahoma'), (u'OR', u'Oregon'), (u'PA', u'Pennsylvania'), (u'RI', u'Rhode Island'), (u'SC', u'South Carolina'), (u'SD', u'South Dakota'), (u'TN', u'Tennessee'), (u'TX', u'Texas'), (u'UT', u'Utah'), (u'VT', u'Vermont'), (u'VA', u'Virginia'), (u'WA', u'Washington'), (u'WV', u'West Virginia'), (u'WI', u'Wisconsin'), (u'WY', u'Wyoming')], required=False, style={'base_template': 'textarea.html'})
post_code = CharField(allow_blank=True, required=False, style={'base_template': 'textarea.html'})
role = ChoiceField(choices=[(u'coach', u'Coach'), (u'client', u'Client')], required=False, style={'base_template': 'textarea.html'})
display_name = ReadOnlyField()
image = ImageField(required=False)
images = ReadOnlyField()
video_capable = BooleanField(required=False)
preferred_venue = ChoiceField(allow_blank=True, choices=[(u'phone', u'Phone'), (u'video', u'Video')], required=False, style={'base_template': 'textarea.html'})
Resulting in this error when a test assertion on the result of a serializer method fails:
E AssertionError: unbalanced braces: "{'name': [u'This field is required.'], 'email': [u'This field is required.']}\n>assert False\n{False = <bound method UserSerializer.is_valid of UserSerializer(data={'timezone': 'Ame...'video', u'Video')], required=False, style={'base_template': 'textarea.html'})>\n{<bound method UserSerializer.is_valid of UserSerializer(data={'timezone': 'Ame...'video', u'Video')], required=False, style={'base_template': 'textarea.html'})> = UserSerializer(data={'timezone': 'America/Denver'}, instance=<User: Test User>...u'video', u'Video')], required=False, style={'base_template': 'textarea.html'}).is_valid\n}()\n}"
Original comment by Carl Meyer (BitBucket: carljm, GitHub: carljm):
Good call. If I remove the else
clause that throws the AssertionError
, things actually work just fine. It acts as if the final }
in the explanation
matches the one it started with (which is actually correct, even though the elision makes it appear unbalanced), and so the output is correct.
Shall I just submit a PR with those two lines removed, then?
Original comment by Carl Meyer (BitBucket: carljm, GitHub: carljm):
@flub Here's a simplest-case reproduction (I can work it into the form of a test in pytest's test suite if you want, this was just a simpler place to start):
class LongReprWithBraces(object):
def __repr__(self):
s = 'LongReprWithBraces({' + ('a' * 80) + '}' + ('a' * 120) + ')'
return s
def some_method(self):
return False
def test_long_repr():
obj = LongReprWithBraces()
assert obj.some_method()
When run, this test should fail with a nicely-formatted explanation, but instead it fails with AssertionError: unbalanced braces:
followed by the unformatted explanation. The reason is the elision performed by py.io.saferepr
on long reprs, which doesn't respect brace-matching.
I see two possible fixes. One would be to fix py.io.saferepr
to be intelligent about matching braces. The easier fix is what Holger suggested above - just remove the else
clause in _collapse_false
that raises the unbalanced braces assertion. Although the latter fix is less "smart", In any case I can think of it would have the right effect.
Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):
Thanks for that example, I should be able to do some more digging around with this.
The reason I' wary of either fix you propose is because the repr of an object should not be interfering with the formatting language used to format explanations internally at all. I also have a memory of a similar bug which involved the repr of an object creating json and also interfering with the internal formatting of explanations. If memory serves me right we then fixed this by escaping the reprs somehow. But that case probably didn't trigger the False-folding so hence why I suspect that _collapse_false()
function is actually to blame not interpreting the escapes correctly. Or maybe the call path up to it somehow passes in the wrong form or something. But it all boils down to my first sentence above I think: the repr of an object should not interfere with the internal formatting language.
Original comment by Carl Meyer (BitBucket: carljm, GitHub: carljm):
Yes, that makes sense. The internal formatting language could of course use longer / more unusual markers. This doesn't eliminate the risk but could make it vanishingly rare.
I don't think the risk can be fully eliminated without passing around a data structure instead of a string.
Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):
I've found the other issue: #453. Looking at that it seems my memory served wrong and that was solved by escaping newlines, not escaping the braces. But close enough. I've created a pull request with a proposed fix.
I do agree that long-term passing around a data structure is a better idea, there's too many special values lying around at the moment.
Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):
Fix collapse false to look at unescaped braces only
Sometimes the repr of an object can contain the "\n{" sequence which is used as a formatting language, so they are escaped to "\n{". But the collapse-false code needs to look for the real "\n{" token instead of simply "{" as otherwise it may get unbalanced braces from the object's repr (sometimes caused by the collapsing of long reprs by saferepr).
Fixes issue #731.
Originally reported by: Carl Meyer (BitBucket: carljm, GitHub: carljm)
pytest.assertion.util._collapse_false(explanation)
breaks if the input explanation text has unbalanced curly braces.This explanation text often contains object
repr()
s.pytest.assertion.util.assertrepr_compare
usespy.io.saferepr
to get the objectrepr()
s and include them in the explanation.py.io.saferepr
will elide overly long objectrepr()
s with...
. In doing so, it can sometimes elide an}
but not the corresponding{
, if arepr()
contains curly braces in it.When this happens, it causes
_collapse_false
to fail withAssertionError: unbalanced braces
.I think this could be fixed in
saferepr
by making it smart about curly braces. I can work on a patch for that, if it's deemed worth doing.