From fa442df2981406b31065938c57b6ee8eaed2e724 Mon Sep 17 00:00:00 2001 From: Noah Date: Fri, 6 Mar 2020 19:27:21 -0500 Subject: [PATCH] fix: show hidden line characters and accessible colors (#126) * wip: rewrite assertion diff report * wip: add colours, more context? * wip: refactor staged line logic * wip: linting * wip: rewrite assertion diff report (#150) * wip * wip * wip * wip * wip * wip * wip * refactor: replace generator type with iterator * refactor: limit context using generators * refactor: reduce diff * refactor: add note about impossible cases * test: hidden characters * chore: update changelog * chore: update changelog * test: diff reporter * lint: shorten line * test: more snapshot test cases * fix: allow context style to work in all backgrounds Co-authored-by: Emmanuel Ogbizi-Ugbe Co-authored-by: Emmanuel Ogbizi --- CHANGELOG.md | 3 +- src/syrupy/__init__.py | 12 +- src/syrupy/constants.py | 4 + src/syrupy/data.py | 26 +++ src/syrupy/extensions/amber.py | 3 +- src/syrupy/extensions/base.py | 173 +++++++++++++++--- src/syrupy/report.py | 4 +- src/syrupy/terminal.py | 28 ++- src/syrupy/utils.py | 4 +- tests/__snapshots__/test_extension_base.ambr | 45 +++++ .../test_integration_default.ambr | 36 ++-- tests/test_extension_base.py | 31 ++++ tests/test_integration_default.py | 2 + 13 files changed, 312 insertions(+), 59 deletions(-) create mode 100644 tests/__snapshots__/test_extension_base.ambr create mode 100644 tests/test_extension_base.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ddff5ea..8648c795 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ From v1.0.0 onwards, this project adheres to [Semantic Versioning](https://semve ## Master (Unreleased) -- Up to date with releases +- Fix bug where missing carriage return not shown in diff reports (#126) +- Account for accessibility and readability in snapshot outputs (#126) ## [v0.3.3](https://github.com/tophat/syrupy/compare/v0.3.2...v0.3.3) diff --git a/src/syrupy/__init__.py b/src/syrupy/__init__.py index 3b0cb2d4..784f6aaa 100644 --- a/src/syrupy/__init__.py +++ b/src/syrupy/__init__.py @@ -15,9 +15,9 @@ from .location import TestLocation from .session import SnapshotSession from .terminal import ( - green, - red, + received_style, reset, + snapshot_style, ) from .utils import import_module_member @@ -64,10 +64,14 @@ def pytest_assertrepr_compare(op: str, left: Any, right: Any) -> Optional[List[s https://docs.pytest.org/en/latest/reference.html#_pytest.hookspec.pytest_assertrepr_compare """ if isinstance(left, SnapshotAssertion): - assert_msg = reset(f"{green(left.name)} {op} {red('received')}") + assert_msg = reset( + f"{snapshot_style(left.name)} {op} {received_style('received')}" + ) return [assert_msg] + left.get_assert_diff(right) elif isinstance(right, SnapshotAssertion): - assert_msg = reset(f"{red('received')} {op} {green(right.name)}") + assert_msg = reset( + f"{received_style('received')} {op} {snapshot_style(right.name)}" + ) return [assert_msg] + right.get_assert_diff(left) return None diff --git a/src/syrupy/constants.py b/src/syrupy/constants.py index c5e0c6e8..8668001b 100644 --- a/src/syrupy/constants.py +++ b/src/syrupy/constants.py @@ -3,3 +3,7 @@ SNAPSHOT_UNKNOWN_FOSSIL_KEY = "unknown snapshot fossil" EXIT_STATUS_FAIL_UNUSED = 1 + +SYMBOL_ELLIPSIS = "..." # U+2026 +SYMBOL_NEW_LINE = "␤" # U+2424 +SYMBOL_CARRIAGE = "␍" # U+240D diff --git a/src/syrupy/data.py b/src/syrupy/data.py index 76d45e0a..1ddcd8b4 100644 --- a/src/syrupy/data.py +++ b/src/syrupy/data.py @@ -3,6 +3,7 @@ TYPE_CHECKING, Dict, Iterator, + List, Optional, ) @@ -117,3 +118,28 @@ def __iter__(self) -> Iterator["SnapshotFossil"]: def __contains__(self, key: str) -> bool: return key in self._snapshot_fossils + + +@attr.s +class DiffedLine: + a: str = attr.ib(default=None) + b: str = attr.ib(default=None) + c: List[str] = attr.ib(factory=list) + diff_a: str = attr.ib(default="") + diff_b: str = attr.ib(default="") + + @property + def has_snapshot(self) -> bool: + return self.a is not None + + @property + def has_received(self) -> bool: + return self.b is not None + + @property + def is_complete(self) -> bool: + return self.has_snapshot and self.has_received + + @property + def is_context(self) -> bool: + return bool(self.c) diff --git a/src/syrupy/extensions/amber.py b/src/syrupy/extensions/amber.py index 995230e5..a105728a 100644 --- a/src/syrupy/extensions/amber.py +++ b/src/syrupy/extensions/amber.py @@ -8,6 +8,7 @@ Set, ) +from syrupy.constants import SYMBOL_ELLIPSIS from syrupy.data import ( Snapshot, SnapshotFossil, @@ -28,7 +29,7 @@ class DataSerializer: class MarkerDepthMax: def __repr__(self) -> str: - return "..." + return SYMBOL_ELLIPSIS @classmethod def write_file(cls, snapshot_fossil: "SnapshotFossil") -> None: diff --git a/src/syrupy/extensions/base.py b/src/syrupy/extensions/base.py index 648d1371..e3eda81d 100644 --- a/src/syrupy/extensions/base.py +++ b/src/syrupy/extensions/base.py @@ -10,16 +10,23 @@ from typing import ( TYPE_CHECKING, Callable, - Generator, + Dict, + Iterator, + List, Optional, Set, - Union, ) from typing_extensions import final -from syrupy.constants import SNAPSHOT_DIRNAME +from syrupy.constants import ( + SNAPSHOT_DIRNAME, + SYMBOL_CARRIAGE, + SYMBOL_ELLIPSIS, + SYMBOL_NEW_LINE, +) from syrupy.data import ( + DiffedLine, Snapshot, SnapshotEmptyFossil, SnapshotFossil, @@ -27,11 +34,12 @@ ) from syrupy.exceptions import SnapshotDoesNotExist from syrupy.terminal import ( - emphasize, - green, - mute, - red, + context_style, + received_diff_style, + received_style, reset, + snapshot_diff_style, + snapshot_style, ) from syrupy.utils import walk_snapshot_dir @@ -214,36 +222,141 @@ def __ensure_snapshot_dir(self, *, index: int) -> None: class SnapshotReporter(ABC): def diff_lines( self, serialized_data: "SerializedData", snapshot_data: "SerializedData" - ) -> Generator[str, None, None]: + ) -> Iterator[str]: for line in self.__diff_lines(str(snapshot_data), str(serialized_data)): yield reset(line) - def __diff_lines(self, a: str, b: str) -> Generator[str, None, None]: - line_styler = {"-": green, "+": red} - staged_line, skip = "", False - for line in ndiff(a.splitlines(), b.splitlines()): - if staged_line and line[:1] != "?": - yield line_styler[staged_line[:1]](staged_line) - staged_line, skip = "", False - if line[:1] in "-+": - staged_line = line - elif line[:1] == "?": - yield self.__diff_line(line, staged_line, line_styler[staged_line[:1]]) - staged_line, skip = "", False - elif not skip: - yield mute(" ...") - skip = True - if staged_line: - yield line_styler[staged_line[:1]](staged_line) - - def __diff_line( - self, marker_line: str, line: str, line_style: Callable[[Union[str, int]], str] + @property + def _ends(self) -> Dict[str, str]: + return {"\n": self._marker_new_line, "\r": self._marker_carriage} + + @property + def _context_line_count(self) -> int: + return 1 + + @property + def _context_line_max(self) -> int: + return self._context_line_count * 2 + + @property + def _marker_context_max(self) -> str: + return SYMBOL_ELLIPSIS + + @property + def _marker_new_line(self) -> str: + return SYMBOL_NEW_LINE + + @property + def _marker_carriage(self) -> str: + return SYMBOL_CARRIAGE + + def __diff_lines(self, a: str, b: str) -> Iterator[str]: + for line in self.__diff_data(a, b): + show_ends = ( + self.__strip_ends(line.a[1:]) == self.__strip_ends(line.b[1:]) + if line.is_complete + else False + ) + if line.has_snapshot: + yield self.__format_line( + line.a, line.diff_a, snapshot_style, snapshot_diff_style, show_ends + ) + if line.has_received: + yield self.__format_line( + line.b, line.diff_b, received_style, received_diff_style, show_ends + ) + yield from map(context_style, self.__limit_context(line.c)) + + def __diff_data(self, a: str, b: str) -> Iterator["DiffedLine"]: + staged_diffed_line: Optional["DiffedLine"] = None + for line in ndiff(a.splitlines(keepends=True), b.splitlines(keepends=True)): + is_context_line = line[0] == " " + is_snapshot_line = line[0] == "-" + is_received_line = line[0] == "+" + is_diff_line = line[0] == "?" + + if is_context_line or is_diff_line: + line = self.__strip_ends(line) + + if staged_diffed_line: + if is_diff_line: + if staged_diffed_line.has_received: + staged_diffed_line.diff_b = line + elif staged_diffed_line.has_snapshot: + staged_diffed_line.diff_a = line + # else: should never happen because then it would have + # encounted a diff line without any previously staged line + else: + should_unstage = ( + staged_diffed_line.is_complete + or (staged_diffed_line.has_snapshot and is_snapshot_line) + or (staged_diffed_line.has_received and is_received_line) + or (staged_diffed_line.is_context and not is_context_line) + ) + if should_unstage: + yield staged_diffed_line + staged_diffed_line = None + elif is_snapshot_line: + staged_diffed_line.a = line + elif is_received_line: + staged_diffed_line.b = line + elif is_context_line: + staged_diffed_line.c.append(line) + + if not staged_diffed_line: + if is_snapshot_line: + staged_diffed_line = DiffedLine(a=line) + elif is_received_line: + staged_diffed_line = DiffedLine(b=line) + elif is_context_line: + staged_diffed_line = DiffedLine(c=[line]) + # else: should never happen because then it would have + # encounted a diff line without any previously staged line + + if staged_diffed_line: + yield staged_diffed_line + + def __format_line( + self, + line: str, + diff_markers: str, + line_style: Callable[[str], str], + diff_style: Callable[[str], str], + show_ends: bool, ) -> str: + if show_ends: + for old, new in self._ends.items(): + line = line.replace(old, new) + else: + line = self.__strip_ends(line) return "".join( - emphasize(line_style(char)) if str(marker) in "-+^" else line_style(char) - for marker, char in zip_longest(marker_line.strip(), line) + diff_style(char) if str(marker) in "-+^" else line_style(char) + for marker, char in zip_longest(diff_markers.rstrip(), line) + if char is not None ) + def __limit_context(self, lines: List[str]) -> Iterator[str]: + yield from lines[: self._context_line_count] + num_lines = len(lines) + if num_lines: + if num_lines > self._context_line_max: + count_leading_whitespace: Callable[[str], int] = ( + lambda s: len(s) - len(s.lstrip()) # noqa: E731 + ) + if self._context_line_count: + num_space = ( + count_leading_whitespace(lines[self._context_line_count - 1]) + + count_leading_whitespace(lines[-self._context_line_count]) + ) // 2 + else: + num_space = count_leading_whitespace(lines[num_lines // 2]) + yield " " * num_space + self._marker_context_max + if self._context_line_count and num_lines > 1: + yield from lines[-self._context_line_count :] # noqa: E203 + + def __strip_ends(self, line: str) -> str: + return line.rstrip("".join(self._ends.keys())) + class AbstractSyrupyExtension(SnapshotSerializer, SnapshotFossilizer, SnapshotReporter): def __init__(self, test_location: "TestLocation"): diff --git a/src/syrupy/report.py b/src/syrupy/report.py index 7ed18065..859d07e5 100644 --- a/src/syrupy/report.py +++ b/src/syrupy/report.py @@ -6,7 +6,7 @@ from typing import ( TYPE_CHECKING, Any, - Generator, + Iterator, List, Set, ) @@ -131,7 +131,7 @@ def unused(self) -> "SnapshotFossils": return unused_fossils @property - def lines(self) -> Generator[str, None, None]: + def lines(self) -> Iterator[str]: summary_lines: List[str] = [] if self.num_failed: summary_lines.append( diff --git a/src/syrupy/terminal.py b/src/syrupy/terminal.py index 90209c23..5e62888a 100644 --- a/src/syrupy/terminal.py +++ b/src/syrupy/terminal.py @@ -23,14 +23,6 @@ def bold(text: Union[str, int]) -> str: return colored.stylize(text, colored.attr("bold")) -def mute(text: Union[str, int]) -> str: - return colored.stylize(text, colored.attr("dim")) - - -def emphasize(text: Union[str, int]) -> str: - return colored.stylize(bold(text), colored.attr("underlined")) - - def error_style(text: Union[str, int]) -> str: return bold(red(text)) @@ -41,3 +33,23 @@ def warning_style(text: Union[str, int]) -> str: def success_style(text: Union[str, int]) -> str: return bold(green(text)) + + +def snapshot_style(text: Union[str, int]) -> str: + return colored.stylize(text, colored.bg(225) + colored.fg(90)) + + +def snapshot_diff_style(text: Union[str, int]) -> str: + return colored.stylize(text, colored.bg(90) + colored.fg(225)) + + +def received_style(text: Union[str, int]) -> str: + return colored.stylize(text, colored.bg(195) + colored.fg(23)) + + +def received_diff_style(text: Union[str, int]) -> str: + return colored.stylize(text, colored.bg(23) + colored.fg(195)) + + +def context_style(text: Union[str, int]) -> str: + return colored.stylize(text, colored.attr("dim")) diff --git a/src/syrupy/utils.py b/src/syrupy/utils.py index f626d211..8010f37c 100644 --- a/src/syrupy/utils.py +++ b/src/syrupy/utils.py @@ -2,7 +2,7 @@ from pathlib import Path from typing import ( Any, - Generator, + Iterator, ) from .constants import SNAPSHOT_DIRNAME @@ -13,7 +13,7 @@ def in_snapshot_dir(path: Path) -> bool: return SNAPSHOT_DIRNAME in path.parts -def walk_snapshot_dir(root: str) -> Generator[str, None, None]: +def walk_snapshot_dir(root: str) -> Iterator[str]: for filepath in Path(root).rglob("*"): if not in_snapshot_dir(filepath): continue diff --git a/tests/__snapshots__/test_extension_base.ambr b/tests/__snapshots__/test_extension_base.ambr new file mode 100644 index 00000000..3f5517c4 --- /dev/null +++ b/tests/__snapshots__/test_extension_base.ambr @@ -0,0 +1,45 @@ +# name: TestSnapshotReporter.test_diff_lines[-0-SnapshotReporterNoContext] + ' + ... + - line 2 + + line 02 + ... + - line 04 + + line 4 + ... + ' +--- +# name: TestSnapshotReporter.test_diff_lines[-0-SnapshotReporter] + ' + line 0 + line 1 + - line 2 + + line 02 + line 3 + - line 04 + + line 4 + line 5 + ... + line 7 + ' +--- +# name: TestSnapshotReporter.test_diff_lines[-1-SnapshotReporterNoContext] + ' + ... + - line 3 + + line 3 + ... + ' +--- +# name: TestSnapshotReporter.test_diff_lines[-1-SnapshotReporter] + ' + line 0 + ... + line 2 + - line 3 + + line 3 + line 4 + ... + line 7 + ' +--- diff --git a/tests/__snapshots__/test_integration_default.ambr b/tests/__snapshots__/test_integration_default.ambr index 7a9ec1ce..b04fff09 100644 --- a/tests/__snapshots__/test_integration_default.ambr +++ b/tests/__snapshots__/test_integration_default.ambr @@ -8,12 +8,14 @@ def test_updated_1(snapshot): > assert snapshot == ['this', 'will', 'not', 'match'] E AssertionError: assert snapshot == received - E ... + E [ + E ... + E 'will', E - 'be', E - 'updated', E + 'not', E + 'match', - E ... + E ] test_file.py:17: AssertionError ________________________________ test_updated_2 ________________________________ @@ -23,11 +25,13 @@ def test_updated_2(snapshot): > assert ['this', 'will', 'fail'] == snapshot E AssertionError: assert received == snapshot - E ... - E + 'fail', + E [ + E ... + E 'will', E - 'be', + E + 'fail', E - 'updated', - E ... + E ] test_file.py:22: AssertionError ________________________________ test_updated_3 ________________________________ @@ -37,11 +41,13 @@ def test_updated_3(snapshot): > assert snapshot == ['this', 'will', 'be', 'too', 'much'] E AssertionError: assert snapshot == received - E ... + E [ + E ... + E 'be', E - 'updated', E + 'too', E + 'much', - E ... + E ] test_file.py:27: AssertionError ________________________________ test_updated_4 ________________________________ @@ -67,22 +73,30 @@ cause when there are a lot of changes you only want to see what changed you do not want to see this line or this line + this line should show up because it changes\n this line should show up because it changes color and this line does not exist in the first one ''' - E assert snapshot == received - E ... + E AssertionError: assert snapshot == received + E ' + E ... + E multiple line changes E - with some lines staying the same E + with some lines not staying the same E - intermittent changes that have to be ignore by the differ output E + intermittent changes so unchanged lines have to be ignored by the differ E - because when there are a lot of changes you only want to see changes E + cause when there are a lot of changes you only want to see what changed - E ... + E you do not want to see this line + E or this line + E - this line should show up because it changes␍␤ + E + this line should show up because it changes␤ + E E - this line should show up because it changes color E + this line should show up because it changes color E + and this line does not exist in the first one - E ... + E + E ' test_file.py:37: AssertionError --------------------------- snapshot report summary ---------------------------- diff --git a/tests/test_extension_base.py b/tests/test_extension_base.py new file mode 100644 index 00000000..2e543b14 --- /dev/null +++ b/tests/test_extension_base.py @@ -0,0 +1,31 @@ +import pytest + +from syrupy.extensions.base import SnapshotReporter + +from .utils import clean_output + + +class SnapshotReporterNoContext(SnapshotReporter): + @property + def _context_line_count(self) -> int: + return 0 + + +@pytest.mark.parametrize("Reporter", [SnapshotReporter, SnapshotReporterNoContext]) +class TestSnapshotReporter: + @pytest.mark.parametrize( + "a, b", + [ + ( + "line 0\nline 1\nline 02\nline 3\nline 4\r\nline 5\nline 6\nline 7", + "line 0\nline 1\nline 2\r\nline 3\nline 04\nline 5\nline 6\nline 7", + ), + ( + "line 0\nline 1\nline 2\nline 3\t\nline 4\nline 5\nline 6\nline 7", + "line 0\nline 1\nline 2\nline 3 \nline 4\nline 5\nline 6\nline 7", + ), + ], + ids=lambda _: "", + ) + def test_diff_lines(self, a, b, Reporter, snapshot): + assert "\n".join(map(clean_output, Reporter().diff_lines(a, b))) == snapshot diff --git a/tests/test_integration_default.py b/tests/test_integration_default.py index 7bd6ec6e..0ffde2d8 100644 --- a/tests/test_integration_default.py +++ b/tests/test_integration_default.py @@ -155,6 +155,7 @@ def test_updated_5(snapshot): because when there are a lot of changes you only want to see changes you do not want to see this line or this line + this line should show up because it changes\\r\\n \x1b[38;5;1mthis line should show up because it changes color\x1b[0m ''' """ @@ -199,6 +200,7 @@ def test_updated_5(snapshot): cause when there are a lot of changes you only want to see what changed you do not want to see this line or this line + this line should show up because it changes\\n \x1b[38;5;3mthis line should show up because it changes color\x1b[0m and this line does not exist in the first one '''