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

assertion diffs for multiline-string can become unreadable soup #6757

Open
wimglenn opened this issue Feb 18, 2020 · 7 comments
Open

assertion diffs for multiline-string can become unreadable soup #6757

wimglenn opened this issue Feb 18, 2020 · 7 comments
Labels
topic: rewrite related to the assertion rewrite mechanism type: enhancement new feature or API change, should be merged into features branch

Comments

@wimglenn
Copy link
Member

wimglenn commented Feb 18, 2020

The diff on multiline strings when a string from capysys is differing from expected just by indentation level and/or some leading/trailing whitespace is not great, I think we can do better.

Here's a reproducer, I just dumped some output of fortune | cowsay into a raw string for example purposes. Intentionally wrong "expected" to demonstrate the issue: it adds 4 space indentation, i.e. hardcoded in a multiline string within a function body but forgetting to use textwrap.dedent on it.

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


def test_cowsay_output(capsys):
    print(actual)
    expected = r"""
     ________________________________________
    / You have Egyptian flu: you're going to \
    \ be a mummy.                            /
     ----------------------------------------
            \   ^__^
             \  (oo)\_______
                (__)\       )\/\
                    ||----w |
                    ||     ||
    """
    captured = capsys.readouterr()
    assert captured.out == expected

When running with increased verbosity -vv, it's often complicated to figure out what the issue is without stepping into a debugger. The initial assertion is on one line which may get wrapped in the terminal over multiple lines:

E       assert ('\n'\n ' ________________________________________\n'\n "/ You have Egyptian flu: you're going to \\\n"\n '\\ be a mummy.                            /\n'\n ' ----------------------------------------\n'\n '        \\   ^__^\n'\n '         \\  (oo)\\_______\n'\n '            (__)\\       )\\/\\\n'\n '                ||----w |\n'\n '                ||     ||\n'\n '\n') == ('\n'\n '     ________________________________________\n'\n "    / You have Egyptian flu: you're going to \\\n"\n '    \\ be a mummy.                            /\n'\n '     ----------------------------------------\n'\n '            \\   ^__^\n'\n '             \\  (oo)\\_______\n'\n '                (__)\\       )\\/\\\n'\n '                    ||----w |\n'\n '                    ||     ||\n'\n '    ')

However the output appears like the actual was a tuple ('\n'\n ' ____ ...

If you look closer you see it is not even valid Python syntax, just a confusing repr corruption, and the multi-line diff that follows isn't too readable either because of the way the changes are interspersed line by line.

Screen Shot 2020-02-17 at 11 59 27 PM

How about an option for the user to suppress pytest's attempts to make a rich diff, and to just to print one, and then print the other, between some horizontal "margins" such as this?

==================================== 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 |
                    ||     ||
    
================================================================================
@blueyed blueyed added topic: rewrite related to the assertion rewrite mechanism type: enhancement new feature or API change, should be merged into features branch labels Feb 18, 2020
@nicoddemus
Copy link
Member

How about an option for the user to suppress pytest's attempts to make a rich diff, and to just to print one, and then print the other, between some horizontal "margins" such as this?

I like this presentation, but I would like to avoid another option if possible.

How about if we have a threshold for differences based on the total number of characters (say 20%+), in which case we fallback to not displaying a diff but show texts horizontally as you suggest?

@wimglenn
Copy link
Member Author

That sounds ok. Or perhaps there can just be presentation included, in addition, after the rich diff for high enough verbosity levels.

Something that just occurred to me, does pytest have any opinion, introspection, or convention about which is the "actual" and which is the "expected"? I suppose it might have to be just be presented "left" and then "right".

@blueyed
Copy link
Contributor

blueyed commented Feb 20, 2020

One good heuristic here would also be to try with indentation removed (textwrap.dedent), and report it as "matches with different indent".

Ratio is also good, of course.

FWIW I have this in my conftest to print more with verbosity 3 - very helpful to adjust existing tests:

@pytest.hookimpl(hookwrapper=True)
def pytest_assertrepr_compare(config: Config, op: str, left: Any, right: Any) -> Optional[List[str]]:
    verbose = config.option.verbose

    outcome = yield
    result = outcome.get_result()  # type: Optional[List[str]]
    if not result:
        return
    lines = result[0]

    …

    if verbose > 2:
        import pprint
        full = pprint.saferepr(right).splitlines()
        output = [
            "Expected:",
        ] + full
        lines.extend(output)
        full = pprint.saferepr(left).splitlines()
        output = [
            "Actual:",
        ] + full
        lines.extend(output)
        print(left)

    outcome.force_result(result)

does pytest have any opinion, introspection, or convention about which is the "actual" and which is the "expected"? I suppose it might have to be just be presented "left" and then "right".

See #3333 / d59adc6

@piotrhm
Copy link
Contributor

piotrhm commented Apr 15, 2020

I played a bit with different outputs for large strings. Here are my thoughts:

  1. Making something similar to:

Screenshot from 2020-04-15 14-45-43

Increase usability only for string with reasonable size. If we want to keep this convention we should batch output - it would be more readable.

  1. Side by side approach. Base version:

Screenshot from 2020-04-15 14-46-22

Version with "?" part:

Screenshot from 2020-04-15 15-31-33

Screenshot from 2020-04-16 11-32-43

Definitely more complicated to implement but represents the best readability.

I am interested in solving this issue. Which version is more suitable?

@wimglenn
Copy link
Member Author

wimglenn commented Apr 15, 2020

My preference would be for a vertical presentation, not side-by-side. It has a few wins in basic practicality

  • ease of copy-pasting the content
  • no complications for long lines (side-by-side necessitates double the terminal width avail, or horizontal scrollbar)
  • simpler to implement

I also don't want to see any extra diff characters inline like "E" on the start of the line or stuff like "?", "-", "++++" within the presentation.

@wimglenn
Copy link
Member Author

wimglenn commented Jun 1, 2024

It is a shame that the PR stalled.

I have this workaround in conftest.py which may be useful for others finding the issue:

def pytest_assertrepr_compare(config, op, left, right):
    # https://docs.pytest.org/en/latest/reference/reference.html#pytest.hookspec.pytest_assertrepr_compare
    if isinstance(left, str) and isinstance(right, str) and op == "==":
        left_lines = left.splitlines(keepends=True)
        right_lines = right.splitlines(keepends=True)
        if len(left_lines) > 1 or len(right_lines) > 1:
            width, _ = shutil.get_terminal_size(fallback=(80, 24))
            width = max(width, 40) - 10
            lines = [
                "When comparing multiline strings:",
                f" LEFT ({len(left)}) ".center(width, "="),
                *left_lines,
                f" RIGHT ({len(right)}) ".center(width, "="),
                *right_lines,
            ]
            return lines

@Pierre-Sassoulas
Copy link
Contributor

I searched for possible diff algorithms to replace the default difflib algo for the final result. Any of git diff 4 base algorithms (myers, minimal, patience or histogram) will highlight each line of the string (effectively displaying all the lines of the old string above all the line of the new string). Using git --word-diff will show nothing in the given example (white spaces are ignored). This or as blueeyed said above, textwrap.dedent being equal, could be another indicator that yet another diff algo is required. In the case of indentation issue, using something like git diff --word-diff-regex="[ ]+|[^ ]+" you can get the following diff :

gitworddiffwithregex

There's specialized diff plugins for git (https://github.com/so-fancy/diff-so-fancy), but I did not check them in detail. Adding a fancy diff for each case might not be sufficiently better than simply displaying both strings alongside each other (what a git diff would do) when the ratio of diff to display become high.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rewrite related to the assertion rewrite mechanism type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

5 participants