Skip to content

Escape whitespace only strings when diffing them on failed assertions #3444

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

Merged

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented May 3, 2018

Fix #3443

Here's how it looks:


    def test_stuff():
        a = u'\r\n'
>       assert a == u'\n'
E       AssertionError: assert '\r\n' == '\n'
E         Strings contain only whitespace, escaping them using repr()
E         - '\r\n'
E         ?  --
E         + '\n'

.tmp\test_rn.py:4: AssertionError

@coveralls
Copy link

coveralls commented May 3, 2018

Coverage Status

Coverage increased (+0.02%) to 92.821% when pulling 413b1aa on nicoddemus:escape-whitespace-diffs into 08aed1a on pytest-dev:master.

@nicoddemus nicoddemus force-pushed the escape-whitespace-diffs branch from 3605511 to dca77b2 Compare May 3, 2018 23:38
@@ -171,10 +171,22 @@ def _diff_text(left, right, verbose=False):
"""
from difflib import ndiff
explanation = []

def to_unicode_text(binary_text):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe we should try to make this function name a bit more speaking

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the best I could come up with by reading the git blame message for the line in question... do you have a suggestion?

If we don't get a better name I think that's not a big deal TBH given that it is a local function, but definitely if we can get an improved name the better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

escape_for_readable_diff perhaps - its a bit tricky to name this one indeed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, updated!

@nicoddemus
Copy link
Member Author

Anything else @RonnyPfannschmidt?

@RonnyPfannschmidt RonnyPfannschmidt merged commit 27651f4 into pytest-dev:master May 4, 2018
@RonnyPfannschmidt
Copy link
Member

well done 👍

@nicoddemus nicoddemus deleted the escape-whitespace-diffs branch May 4, 2018 16:44
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.

3 participants