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.

Sign up for GitHub

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

_diff_text: use repr with escape characters #6702

Closed
wants to merge 13 commits into from
Closed

_diff_text: use repr with escape characters #6702

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c97c50e
_diff_text: use repr with escape characters
blueyed Feb 10, 2020
0e153a0
fixup! _diff_text: use repr with escape characters
blueyed Feb 10, 2020
9318b23
raw strings, zero-width, note
blueyed Feb 14, 2020
079e0f9
test_reprcompare_zerowidth_and_non_printable
blueyed Feb 15, 2020
989bdd7
simplify itertools.chain.from_iterable
blueyed Feb 15, 2020
5274504
remove zero-width
blueyed Feb 15, 2020
2e81042
handle different line-endings
blueyed Feb 15, 2020
2ba59a1
replace itertools.chain.from_iterable
blueyed Feb 15, 2020
e567e04
test_diff_different_line_endings: more coverage
blueyed Feb 15, 2020
3e6c734
revisit, handle more on only one side
blueyed Feb 19, 2020
3a7c810
fix "Avoid deeply nested control flow statements"
blueyed Feb 19, 2020
48c9061
do not repr extra newlines
blueyed Feb 19, 2020
5f92be8
fix max_split
blueyed Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/_pytest/assertion/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ def _diff_text(left: str, right: str, verbose: int = 0) -> List[str]:
characters which are identical to keep the diff minimal.
"""
from difflib import ndiff
from wcwidth import wcswidth

explanation = [] # type: List[str]

Expand Down Expand Up @@ -225,10 +226,18 @@ def _diff_text(left: str, right: str, verbose: int = 0) -> List[str]:
left = repr(str(left))
right = repr(str(right))
explanation += ["Strings contain only whitespace, escaping them using repr()"]
explanation += [
line.strip("\n")
for line in ndiff(left.splitlines(keepends), right.splitlines(keepends))
]

left_lines = left.splitlines(keepends)
right_lines = right.splitlines(keepends)

if any(wcswidth(x) == -1 for x in left_lines + right_lines):
bluetech marked this conversation as resolved.
Show resolved Hide resolved
left_lines = [repr(x) for x in left_lines]
right_lines = [repr(x) for x in right_lines]
explanation += [
"Strings contain non-printable/escape characters, escaping them using repr()"
bluetech marked this conversation as resolved.
Show resolved Hide resolved
bluetech marked this conversation as resolved.
Show resolved Hide resolved
]

explanation += [line.strip("\n") for line in ndiff(left_lines, right_lines)]
return explanation


Expand Down
47 changes: 36 additions & 11 deletions testing/test_assertion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,14 @@ def test_multiline_text_diff(self):
left = "foo\nspam\nbar"
right = "foo\neggs\nbar"
diff = callequal(left, right)
assert "- spam" in diff
assert "+ eggs" in diff
assert diff == [
"'foo\\nspam\\nbar' == 'foo\\neggs\\nbar'",
blueyed marked this conversation as resolved.
Show resolved Hide resolved
"Strings contain non-printable/escape characters, escaping them using repr()",
" 'foo\\n'",
"- 'spam\\n'",
"+ 'eggs\\n'",
" 'bar'",
]
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this change (and others) - do you consider a newline a non-printable/zero-width character? Why? It seems quite confusing to me to see a multi-line output but also \n, i.e. with newlines represented twice.

Copy link
Contributor Author

@blueyed blueyed Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@The-Compiler

Why?

Otherwise it will not be visible. Check https://github.com/pytest-dev/pytest/pull/6702/files/9318b236ff35fc6a0734ee91bff2011afb93971a#diff-14ad89cac128f1dd3aaa927c656035b3R1342.

Note: this is for a line (text/str) diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be made smarter though maybe to not show them when not at the end of a line?

Copy link
Member

Choose a reason for hiding this comment

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

For illustration, on master:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 1 bar
E         ? ^
E         + 2 bar
E         ? ^

On this branch:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E         NOTE: Strings contain non-printable/zero-width characters. Escaping them using repr().
E           'foo\n'
E         - '1 bar'
E         ?  ^
E         + '2 bar'
E         ?  ^

Is this what you would prefer @The-Compiler ?

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E         NOTE: Strings contain non-printable/zero-width characters. Escaping them using repr().
E         - 'foo\n1 bar'
E         ?       ^
E         + 'foo\n2 bar'
E         ?       ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For illustration, on master:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 1 bar
E         ? ^
E         + 2 bar
E         ? ^

On master it has a different order (which I find still wrong - it should be left-to-right, top-to-bottom, with only +/- swapped, but off topic here - maybe therefore edited manually?):

    def test_eq_similar_text(self):
        x = "foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 2 bar
E         ? ^
E         + 1 bar
E         ? ^

How about?

    def test_eq_similar_text(self):
        x = "foo\n1 bar\n"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar\n' == 'foo\n2 bar'
E         NOTE: Strings contain different line-endings. Escaping them using repr().
E         - 'foo\n1 bar\n'
E         ?       ^    --
E         + 'foo\n2 bar'
E         ?       ^

However, with longer strings it is useful to split them on newlines, of course.

>       assert x == "foo\n1 bar"
E       AssertionError: assert 'foo\n1 bar\n' == 'foo\n1 bar'
E         NOTE: Strings contain different line-endings. Escaping them using repr().
E           foo
E         - 1 bar\n
E         ?      --
E         + 1 bar
E         -

FWIW it always looked a bit strange to me seeing:

- foo
+ foo
     ^

(it could also be a space etc)

E         - 'foo\n1 bar'
E         ?       ^
E         + 'foo\n2 bar'
E         ?       ^

This could also be triggered via some minimal length (related: blueyed#218, where I split it onto separate lines with a certain length).

Copy link
Member

@The-Compiler The-Compiler Feb 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I guess explicit is indeed better than implicit in this case. I agree having the ^ marker pointing to "nothing" is odd, and I remember people being confused about that.


def test_bytes_diff_normal(self):
"""Check special handling for bytes diff (#5260)"""
Expand Down Expand Up @@ -989,7 +995,7 @@ def test_full_output_truncated(self, monkeypatch, testdir):

line_count = 7
line_len = 100
expected_truncated_lines = 2
expected_truncated_lines = 3
testdir.makepyfile(
r"""
def test_many_lines():
Expand All @@ -1007,19 +1013,26 @@ def test_many_lines():
# without -vv, truncate the message showing a few diff lines only
result.stdout.fnmatch_lines(
[
"*- 1*",
"*- 3*",
"*- 5*",
"> assert a == b",
"E AssertionError: assert '000000000000...6666666666666' == '000000000000...6666666666666'",
"E Skipping 91 identical leading characters in diff, use -v to show",
"E Strings contain non-printable/escape characters, escaping them using repr()",
"E '000000000\\n'",
"E - '1*\\n'",
"E '2*\\n'",
"E - '3*\\n'",
"E '4*\\...",
"E ",
"*truncated (%d lines hidden)*use*-vv*" % expected_truncated_lines,
]
)

result = testdir.runpytest("-vv")
result.stdout.fnmatch_lines(["* 6*"])
result.stdout.fnmatch_lines(["* '6*"])

monkeypatch.setenv("CI", "1")
result = testdir.runpytest()
result.stdout.fnmatch_lines(["* 6*"])
result.stdout.fnmatch_lines(["* '6*"])


def test_python25_compile_issue257(testdir):
Expand Down Expand Up @@ -1068,6 +1081,17 @@ def test_reprcompare_whitespaces():
]


def test_reprcompare_escape_sequences_strings():
assert callequal("\x1b[31mred", "\x1b[31mgreen") == [
"'\\x1b[31mred' == '\\x1b[31mgreen'",
"Strings contain non-printable/escape characters, escaping them using repr()",
"- '\\x1b[31mred'",
"? ^",
"+ '\\x1b[31mgreen'",
"? + ^^",
]


def test_pytest_assertrepr_compare_integration(testdir):
testdir.makepyfile(
"""
Expand Down Expand Up @@ -1311,9 +1335,10 @@ def test_diff():
result.stdout.fnmatch_lines(
r"""
*assert 'asdf' == 'asdf\n'
* - asdf
* + asdf
* ? +
E Strings contain non-printable/escape characters, escaping them using repr()
* - 'asdf'
* + 'asdf\n'
* ? ++
"""
)

Expand Down