Skip to content

Issue 6757 - Improved diff output for similar strings. #7099

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

Closed
wants to merge 18 commits into from
Closed
2 changes: 2 additions & 0 deletions changelog/6757.improvement.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
When displaying a comparison between two texts in an assertion failure, if the texts differ significantly (more than 30% of lines and more than 4 lines), they are now displayed one after the other rather than in an interleaved diff (+/-) view.
In such cases, a diff is often incomprehensible, and it is easier to see the difference in full.
100 changes: 68 additions & 32 deletions src/_pytest/assertion/util.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Utilities for assertion debugging"""
import collections.abc
import pprint
from difflib import SequenceMatcher
from typing import AbstractSet
from typing import Any
from typing import Callable
Expand Down Expand Up @@ -131,6 +132,8 @@ def isiterable(obj: Any) -> bool:
def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[str]]:
"""Return specialised explanations for some operators/operands"""
verbose = config.getoption("verbose")
screen_width = config.get_terminal_reporter().screen_width
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding get_terminal_reporter() to Config and a screen_width property, it will be better to use config.get_terminal_writer().fullwidth.


if verbose > 1:
left_repr = safeformat(left)
right_repr = safeformat(right)
Expand All @@ -148,10 +151,10 @@ def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[
explanation = None
try:
if op == "==":
explanation = _compare_eq_any(left, right, verbose)
explanation = _compare_eq_any(left, right, screen_width, verbose)
elif op == "not in":
if istext(left) and istext(right):
explanation = _notin_text(left, right, verbose)
explanation = _notin_text(left, right, screen_width, verbose)
except outcomes.Exit:
raise
except Exception:
Expand All @@ -168,29 +171,7 @@ def assertrepr_compare(config, op: str, left: Any, right: Any) -> Optional[List[
return [summary] + explanation


def _compare_eq_any(left: Any, right: Any, verbose: int = 0) -> List[str]:
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, moving code around makes it harder to review, because it breaks the diff view, so best to do it separately.

I guess this is a relevant comment for this PR 😄

explanation = [] # type: List[str]
if istext(left) and istext(right):
explanation = _diff_text(left, right, verbose)
else:
if issequence(left) and issequence(right):
explanation = _compare_eq_sequence(left, right, verbose)
elif isset(left) and isset(right):
explanation = _compare_eq_set(left, right, verbose)
elif isdict(left) and isdict(right):
explanation = _compare_eq_dict(left, right, verbose)
elif type(left) == type(right) and (isdatacls(left) or isattrs(left)):
type_fn = (isdatacls, isattrs)
explanation = _compare_eq_cls(left, right, verbose, type_fn)
elif verbose > 0:
explanation = _compare_eq_verbose(left, right)
if isiterable(left) and isiterable(right):
expl = _compare_eq_iterable(left, right, verbose)
explanation.extend(expl)
return explanation


def _diff_text(left: str, right: str, verbose: int = 0) -> List[str]:
def _diff_text(left: str, right: str, screen_width: int, verbose: int = 0) -> List[str]:
"""Return the explanation for the diff between text.

Unless --verbose is used this will skip leading and trailing
Expand Down Expand Up @@ -231,10 +212,62 @@ def _diff_text(left: str, right: str, verbose: int = 0) -> List[str]:
explanation += ["Strings contain only whitespace, escaping them using repr()"]
# "right" is the expected base against which we compare "left",
# see https://github.com/pytest-dev/pytest/issues/3333
explanation += [
line.strip("\n")
for line in ndiff(right.splitlines(keepends), left.splitlines(keepends))
]

stripped_left = "".join(left.splitlines())
stripped_right = "".join(right.splitlines())
s = SequenceMatcher(None, stripped_left, stripped_right)

nlines_left = left.count("\n")
nlines_right = right.count("\n")

if s.ratio() < 0.30 or max(nlines_left, nlines_right) < 5:
explanation += [
line.strip("\n")
for line in ndiff(right.splitlines(keepends), left.splitlines(keepends))
]
else:

def _text_header(header: str, screen_width: int, margin: int = 10) -> List[str]:
hlength = len(header)
lines = [
"=" * int((screen_width - hlength - margin) / 2)
+ header
+ "=" * int((screen_width - hlength - margin) / 2)
]
if screen_width % 2 != 0:
lines[-1] += "="

return lines

explanation += _text_header(" ACTUAL ", screen_width)
explanation += list(left.split("\n"))
explanation += _text_header(" EXPECTED ", screen_width)
explanation += list(right.split("\n"))
Comment on lines +229 to +245
Copy link
Member

Choose a reason for hiding this comment

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

IIUC str.center can take care of most of this. Also can you explain why the margin (10) is needed?

Suggested change
def _text_header(header: str, screen_width: int, margin: int = 10) -> List[str]:
hlength = len(header)
lines = [
"=" * int((screen_width - hlength - margin) / 2)
+ header
+ "=" * int((screen_width - hlength - margin) / 2)
]
if screen_width % 2 != 0:
lines[-1] += "="
return lines
explanation += _text_header(" ACTUAL ", screen_width)
explanation += list(left.split("\n"))
explanation += _text_header(" EXPECTED ", screen_width)
explanation += list(right.split("\n"))
explanation += [
" ACTUAL ".center(screen_width - 10, "="),
*left.split("\n"),
" EXPECTED ".center(screen_width - 10, "="),
*right.split("\n"),
]


return explanation


def _compare_eq_any(
left: Any, right: Any, screen_width: int, verbose: int = 0
) -> List[str]:
explanation = [] # type: List[str]
if istext(left) and istext(right):
explanation = _diff_text(left, right, screen_width, verbose)
else:
if issequence(left) and issequence(right):
explanation = _compare_eq_sequence(left, right, verbose)
elif isset(left) and isset(right):
explanation = _compare_eq_set(left, right, verbose)
elif isdict(left) and isdict(right):
explanation = _compare_eq_dict(left, right, verbose)
elif type(left) == type(right) and (isdatacls(left) or isattrs(left)):
type_fn = (isdatacls, isattrs)
explanation = _compare_eq_cls(left, right, screen_width, verbose, type_fn)
elif verbose > 0:
explanation = _compare_eq_verbose(left, right)
if isiterable(left) and isiterable(right):
expl = _compare_eq_iterable(left, right, verbose)
explanation.extend(expl)
return explanation


Expand Down Expand Up @@ -411,6 +444,7 @@ def _compare_eq_dict(
def _compare_eq_cls(
left: Any,
right: Any,
screen_width: int,
verbose: int,
type_fns: Tuple[Callable[[Any], bool], Callable[[Any], bool]],
) -> List[str]:
Expand Down Expand Up @@ -445,17 +479,19 @@ def _compare_eq_cls(
("%s: %r != %r") % (field, getattr(left, field), getattr(right, field)),
"",
"Drill down into differing attribute %s:" % field,
*_compare_eq_any(getattr(left, field), getattr(right, field), verbose),
*_compare_eq_any(
getattr(left, field), getattr(right, field), screen_width, verbose
),
]
return explanation


def _notin_text(term: str, text: str, verbose: int = 0) -> List[str]:
def _notin_text(term: str, text: str, screen_width: int, verbose: int = 0) -> List[str]:
index = text.find(term)
head = text[:index]
tail = text[index + len(term) :]
correct_text = head + tail
diff = _diff_text(text, correct_text, verbose)
diff = _diff_text(text, correct_text, screen_width, verbose)
newdiff = ["%s is contained here:" % saferepr(term, maxsize=42)]
for line in diff:
if line.startswith("Skipping"):
Expand Down
3 changes: 3 additions & 0 deletions src/_pytest/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,9 @@ def _ensure_unconfigure(self) -> None:
def get_terminal_writer(self):
return self.pluginmanager.get_plugin("terminalreporter")._tw

def get_terminal_reporter(self):
return self.pluginmanager.get_plugin("terminalreporter")

def pytest_cmdline_parse(
self, pluginmanager: PytestPluginManager, args: List[str]
) -> object:
Expand Down
6 changes: 6 additions & 0 deletions src/_pytest/terminal.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,12 @@ def showfspath(self, value: Optional[bool]) -> None:
def showlongtestinfo(self) -> bool:
return self.verbosity > 0

@property
def screen_width(self) -> int:
if self._screen_width is None:
return 80
return self._screen_width

def hasopt(self, char: str) -> bool:
char = {"xfailed": "x", "skipped": "s"}.get(char, char)
return char in self.reportchars
Expand Down
61 changes: 61 additions & 0 deletions testing/test_assertion.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ def getoption(self, name):
return verbose
raise KeyError("Not mocked out: %s" % name)

def get_terminal_reporter(self):
class Terminal:
@property
def screen_width(self):
return 80

return Terminal()

return Config()


Expand Down Expand Up @@ -360,6 +368,59 @@ def test_multiline_text_diff(self) -> None:
assert "- eggs" in diff
assert "+ spam" in diff

def test_multiline_long_text_diff(self):
left = r"""
________________________________________
/ You have Egyptian flu: you're going to \
\ be a mummy. /
----------------------------------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
"""

right = r"""
________________________________________
/ You have Egyptian flu: you're going to \
\ be a mummy. /
----------------------------------------
\ ^__^
\ (oo)\_______
(__)\ )\/\
||----w |
|| ||
"""
diff = callequal(left, right)
assert diff is not None
assert diff[1:] == [
"=============================== ACTUAL ===============================",
"",
" ________________________________________",
" / You have Egyptian flu: you're going to \\",
" \\ be a mummy. /",
" ----------------------------------------",
" \\ ^__^",
" \\ (oo)\\_______",
" (__)\\ )\\/\\",
" ||----w |",
" || ||",
" ",
"============================== EXPECTED ==============================",
"",
" ________________________________________",
" / You have Egyptian flu: you're going to \\",
" \\ be a mummy. /",
" ----------------------------------------",
" \\ ^__^",
" \\ (oo)\\_______",
" (__)\\ )\\/\\",
" ||----w |",
" || ||",
" ",
]

def test_bytes_diff_normal(self):
"""Check special handling for bytes diff (#5260)"""
diff = callequal(b"spam", b"eggs")
Expand Down