Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_format_assertmsg: use safeformat instead of saferepr #6225

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 18, 2019

Custom assertion messages should not get shortened via saferepr.

Via blueyed#125.
Fixes #5199.

Custom assertion messages should not get shortened via `saferepr`.

Via #125.
Fixes pytest-dev#5199.
@blueyed blueyed force-pushed the _format_assertmsg-safeformat-upstream branch from bb9f2d3 to dbb26a3 Compare November 18, 2019 21:26
@nicoddemus
Copy link
Member

Hi @blueyed,

Thanks for the patch!

The underlying reasoning is sound, but the test doesn't seem to be harden enough.

Here's the output of test_AssertionError_message on features:

___________________________________________ test_bytes ____________________________________________

    def test_bytes():
>       assert 0, b'b' * 80
E       AssertionError: b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
E       assert 0

test_AssertionError_message.py:6: AssertionError

And here's the output from your branch:

___________________________________________ test_bytes ____________________________________________

    def test_bytes():
>       assert 0, b'b' * 80
E       AssertionError: (b'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'
E          b'bbbb')
E       assert 0

test_AssertionError_message.py:6: AssertionError

Both are not truncating the message, and in fact the message from features looks better to me. Thoughts?

@blueyed
Copy link
Contributor Author

blueyed commented Nov 19, 2019

@nicoddemus
saferepr truncates at 240 characters.

I've thought it would be enough to show that safeformat gets used, which here wraps at 80 characters.

I've initially done this to fix:

>>> saferepr(list(range(0, 10)))
'[0, 1, 2, 3, 4, 5, ...]'

vs

>>> safeformat(list(range(0, 10)))
'[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]'

(Note that this patch would also look better with wider columns, not using 80, of course.
That's something to do as a follow-up though.)

I've also thought to use just str in the first place, but then thought that formatting is good in general.

Also note that safeformat does not wrap strings.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 19, 2019

Should maybe use _pformat_dispatch from cc503c1?

@nicoddemus
Copy link
Member

I've thought it would be enough to show that safeformat gets used, which here wraps at 80 characters.

I see, thanks for the explanation. Do you know why it is showing the byte around () though?

Should maybe use _pformat_dispatch from cc503c1?

Hmm sounds reasonable.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 19, 2019

I've thought it would be enough to show that safeformat gets used, which here wraps at 80 characters.

I see, thanks for the explanation. Do you know why it is showing the byte around () though?

This is because it gets split into multiple lines.

Also happens for strings, but requires spaces there it seems:

>>> pprint.pprint(('b'*10 + ' ') * 8)
('bbbbbbbbbb bbbbbbbbbb bbbbbbbbbb bbbbbbbbbb bbbbbbbbbb bbbbbbbbbb bbbbbbbbbb '
 'bbbbbbbbbb ')

Should maybe use _pformat_dispatch from cc503c1?

Hmm sounds reasonable.

Not so sure myself anymore. Anything fitting 80 chars (to be the terminal width in the long run) should be ok to not wrap probably.

So I'd rather leave it as is, maybe adding the extra test for not truncating the tuple/list?

@blueyed blueyed closed this Nov 27, 2019
@blueyed blueyed deleted the _format_assertmsg-safeformat-upstream branch December 2, 2019 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants