Direction of diff (use of +/-) for strings intentional?

[ctheune]

This is on pytest 3.4.2, I don't think other environmental parameters are relevant.

I've noticed a few times that I'm struggling with the +/- usage of long string diffs (-v) as that I never can immediately understand what was expected and what was found.

First, pytest documentation shows the same style of writing assertions as I do, in the form of:

    assert my_result == 'bob'

So the left hand would hold the value the UUT gave me and the right hand is what the test is expecting it to be. If you use this for long strings and -v for showing diffs this ends up in the following:

    assert app.mailer_mock.messages[0][2] == """\
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Subject: Re: Hilfe
To: dh@example.com
From: support@flyingcircus.io
In-Reply-To: 12345
Message-ID: <msg@flyingcircus.io>

Dear Customer,

we could not find a valid SLA for you. If you are sure to have one,
please check if you provided the correct PIN.

Best regards,
the Flying Circus support team
"""

Output:

AssertionError: assert 'Content-Type...upport team\n' == 'Content-Type:...upport team\n'
    Content-Type: text/plain; charset="utf-8"
    Content-Transfer-Encoding: 7bit
    MIME-Version: 1.0
    Subject: Re: Hilfe
    To: dh@example.com
  + From: support@flyingcircus.io
    In-Reply-To: 12345
    Message-ID: <msg@flyingcircus.io>
  - From: support@flyingcircus.io

    Dear Customer,

    we could not find a valid SLA for you. If you are sure to have one,
    please check if you provided the correct PIN.

    Best regards,
    the Flying Circus support team

When looking at that output, I would like to interpret it as "the + shows me what the test found that was there but not expected" and "the - shows me what the test expected but is missing".

Obviously this comes down to the +/- being a convention that implies that the left hand side shows the expected value and the right hand side the result of the UUT. However, writing it down the other way makes the test IMHO less readable and is also known as "yoda expressions" and usually frowned upon [citation needed].

This issue is probably more about discussion and understanding intent and stance from pytest's perspective and likely won't change the way its reported. Although I could also imagine adding a configuration knob for this behaviour.

[RonnyPfannschmidt]

as far as i understand,/vaugely remember the intent here is to support the common spelling on assert expected == computed

@flub and @benjaminp may remember details on when and how it was chosen without digging trough the project history

[flub]

Huh, I always thought it was computed == expected, at least that's how I tend to write my tests. It matches Python APIs like isinstance(my_thing, ExpectedThing) as well. And according to @ctheune our docs even seem to agree on this.

To be honest, I don't think there was ever much thought put into the choice. The code did difflib.ndiff(left, right) because that's just the I-didnt-think-about-this way to write it and the output looked sane. I've also always read the output as "this is the patch needed to make the output match the expectation", but appreciate this is pretty subjective.

I'm kind of tempted to say that it's too late for this anyway, I'm even tempted to argue against a configuration knob as then the output of pytest is dependent on some hidden setting somewhere. So when you're in a new project or just see a paste somewhere you get to guess which way they used this, if you even realised this existed in the first place.

[RonnyPfannschmidt]

from my pov making this one sane is a good reason for a major version bump
lets document and wire up the sane expectation, then look at timeframes

after all its ore of an ux issue, not a consumed api as far as i understod

[ctheune]

@flub I understand that thought about leaving it as-is, however, from a UI perspective I think it might be better in the long run to admit mistakes, fix them and move on. If the majority thinks that the UI issue isn't worthwhile and I'm the onlly one bothered by the current state of affairs then I won't hold a grudge against that decision. ;)

However, if nobody actually relies on the usability because it's broken, then you could also just fix it and accept that the current confusing state of affairs might be a bit longer confusing for a transition period until the expectation settles for the new / "better" version ... not sure whether a major version bump is rectified by this, but I guess at least a minor number would definitely be in order ...

[benjaminp]

I don't recall there being any good reason for this initially. Proposed change seems good to me.

[The-Compiler]

I've also always found the diff outputs confusing in some way, and usually looked at the expected text (or even printed it) to make things clearer to me. I've never put much thought into it, but this might have been why!

FWIW I also do assert foo() == expected and would expect (hah!) most people to do it that way in pytest. self.assertEqual(expected, actual) is probably common with unittest.py though, probably because JUnit does it that way.

[flub]

Well, seems like there's a quorum for the change!

[nicoddemus]

@The-Compiler

I've never put much thought into it, but this might have been why!

Exactly what I was thinking before I reached your comment. 😆

I'm definitely 👍 about changing this, this always bothered me and I never realized the reason, like @The-Compiler said. I also don't think we need a major version bump either, it is an improvement on how we output things, we just need a clear changelog entry and a note to the docs.

[nicoddemus]

@RonnyPfannschmidt why did you label this as "backward compatibility" and scheduled for 4.0? Do you fear people will get confused when faced with different pytest versions?

IMHO this can be in the next feature release, I don't think any test suite will break because of this: it is a change to how the diff is produced and we have changed the console output of pytest in minor releases several times in the past (version number to plugins, how pytest.approx shows some diffs, progress indicator, how all diffs if under 10 lines or so, the list goes on).

[flub]

Somewhat relevant to this: https://stackoverflow.com/questions/49868038/can-someone-explain-what-are-the-and-in-the-error-output-of-pytest

[ghost]

Just as an idea, perhaps some other formats could be considered. I'm wondering if there's some other way of writing the output that's more immediately obvious.

For example, there are a bunch of arrow characters (← ↑ → ↓ ↔ ▲ ▼ ◀ ▶ ⟸ ⟹ ⟺ ⟻ ⟼ , and many more). Perhaps even longer strings in --verbose mode could be used.

Just figured I'd mention the idea in case it inspires anybody : )

    def test_wat_plus():
        # This failure's output includes a +, since the right-hand side has more stuff.
>       assert 'one\ntwo\nthree\n' == 'one\ntwo\ntwo-anna-half\nthree\n'
E       AssertionError: assert 'one\ntwo\nthree\n' == 'one\ntwo\ntwo-anna-half\nthree\n'
E           one
E           two
E       -▶+ two-anna-half
E           three

test_wat.py:3: AssertionError
__________________________________________________________________________________________________________________ test_wat_minus __________________________________________________________________________________________________________________

    def test_wat_minus():
        # This failure's output includes a -, since the left-hand side has more stuff.
>       assert 'one\ntwo\ntwo-anna-half\nthree\n' == 'one\ntwo\nthree\n'
E       AssertionError: assert 'one\ntwo\ntw...half\nthree\n' == 'one\ntwo\nthree\n'
E           one
E           two
E       +◀- two-anna-half
E           three

test_wat.py:8: AssertionError
============================================================================================================= 2 failed in 0.18 seconds =============================================================================================================

[mcarans]

I think assert actual = expected is a more natural way of writing asserts. Is there a plan to introduce this change to pytest?

[RonnyPfannschmidt]

there is a rough "consent" to get it in but no "plan" to speak of

[sscherfke]

@RonnyPfannschmidt @flub Could this maybe be changed in pytest 6.0? I’d be willing to help you out. :)

[RonnyPfannschmidt]

@sscherfke absolutely, i'd like to have @nicoddemus ack this as well

[nicoddemus]

Agreed, thanks @sscherfke for offering to help. 👍

[flub]

Anyone please feel free to contribute this, so please go ahead @sscherfke! There's clear consensus and implementing it shouldn't be crazy hard.

[blueyed]

I actually think it is fine like it is: you have left and right, and as with diffs it uses "-" for the left side, "+" for the right.

I agree with what @flub said in #3333 (comment):

I've also always read the output as "this is the patch needed to make the output match the expectation", but appreciate this is pretty subjective.

I think it is better to have this adhere to "left vs. right", than assuming "expected should be on the right, and we want to show the difference from the expected to the actual".

I do not think "I've also always found the diff outputs confusing in some way, and usually looked at the expected text (or even printed it) to make things clearer to me. I've never put much thought into it, but this might have been why!" is an argument really, given that diff output can be confusing in general, which has been improved slightly since then.

To be clear, I think this:

    def test():
>       assert [1, 2, 3] == [1, 3]
E       assert [1, 2, 3] == [1, 3]
E         At index 1 diff: 2 != 3
E         Left contains one more item: 3
E         Full diff:
E           [
E            1,
E         -  2,
E            3,
E           ]

Is better than:

    def test():
>       assert [1, 2, 3] == [1, 3]
E       assert [1, 2, 3] == [1, 3]
E         At index 1 diff: 2 != 3
E         Left contains one more item: 3
E         Full diff:
E           [
E            1,
E         +  2,
E            3,
E           ]

And while the issue title explicitly mentions strings I do not think anyone means / wants to change this only for strings, do you?

[mcarans]

I share the common understanding that has actual on the left ie. assert actual == expected and hence I find this:

    def test():
>       assert [1, 2, 3] == [1, 3]
E       assert [1, 2, 3] == [1, 3]
E         At index 1 diff: 2 != 3
E         Left contains one more item: 3
E         Full diff:
E           [
E            1,
E         +  2,
E            3,
E           ]

is clearer than:

    def test():
>       assert [1, 2, 3] == [1, 3]
E       assert [1, 2, 3] == [1, 3]
E         At index 1 diff: 2 != 3
E         Left contains one more item: 3
E         Full diff:
E           [
E            1,
E         -  2,
E            3,
E           ]

The first one reads logically to me as the actual having an additional 2 compared to the expected. The second one is confusing - I doubt the majority of people think in terms of what patch to apply.

[blueyed]

Ok, I guess then this should become an option if changed, and might then define the chars even for "left" and "right", to support e.g. special chars/strings suggested in #3333 (comment).
FWIW in #3721 (comment) it was suggested on include a "legend".

[flub]

I'm strongly against making this configurable. We'll never be able to read a diff anymore.

[sscherfke]

I agree with @flub and, as I see it, there is already a consensus for the following semantics:

# "2" is expected but is missing in the result
E           [
E            1,
E         -  2,
E            3,
E           ]

# "2" is expected but the result list contains "4" instead.
E           [
E            1,
E         -  2,
E         +  4,
E            3,
E           ]

# "4" is not expected to be in the result list
E           [
E            1,
E         +  4,
E            3,
E           ]

[nicoddemus]

I'm strongly against making this configurable.

Definitely. This will just increase the confusion when seeing the output from other users in posts, forums, CI logs, etc.

[sscherfke]

Shall I target the feature branch or is shall I create a new one for v6?

[nicoddemus]

features branch, thanks!

[sscherfke]

This is not just a “replace the args in the diff() call”. Several functions in _pytest.assertion.util have to be touch and I guess I’ll also have to write some new tests.

I’m also going to rename left/right to result/expected everywhere in that file and thus encode the convention assert result is expected in that file for better readability. Is that okay for you?

[nicoddemus]

Sure! Thanks for taking the time to tackle this. 👍

[nedbat]

This is going to be a disruptive change for those of us that did write tests so that the diffs make sense.

[sscherfke]

I have written several tests that check the new behavior and also serve as good-to-read examples. Only after I changed pytest's diff output, I found other tests that already check the expected diff output.

That question is: Shall I keep my tests since they explicitly only test the diff output and are easy-to-read examples or shall I delete them since they don't increase test coverage?

"""
Tests and examples for correct "+/-" usage in error diffs.

See https://github.com/pytest-dev/pytest/issues/3333 for details.

"""
import pytest


TESTCASES = [
    (   # Compare lists, one item differs
        """
        def test_this():
            result =   [1, 4, 3]
            expected = [1, 2, 3]
            assert result == expected
        """,
        """
        >       assert result == expected
        E       assert [1, 4, 3] == [1, 2, 3]
        E         At index 1 diff: 4 != 2
        E         Full diff:
        E         - [1, 2, 3]
        E         ?     ^
        E         + [1, 4, 3]
        E         ?     ^
        """,
    ),
    (   # Compare lists, one extra item
        """
        def test_this():
            result =   [1, 2, 3]
            expected = [1, 2]
            assert result == expected
        """,
        """
        >       assert result == expected
        E       assert [1, 2, 3] == [1, 2]
        E         Result contains one more item: 3
        E         Full diff:
        E         - [1, 2]
        E         + [1, 2, 3]
        E         ?      +++
        """,
    ),
    (   # Compare lists, one item missing
        """
        def test_this():
            result =   [1, 3]
            expected = [1, 2, 3]
            assert result == expected
        """,
        """
        >       assert result == expected
        E       assert [1, 3] == [1, 2, 3]
        E         At index 1 diff: 3 != 2
        E         Expected contains one more item: 3
        E         Full diff:
        E         - [1, 2, 3]
        E         ?     ---
        E         + [1, 3]
        """,
    ),
    (   # Compare tuples
        """
        def test_this():
            result =   (1, 4, 3)
            expected = (1, 2, 3)
            assert result == expected
        """,
        """
        >       assert result == expected
        E       assert (1, 4, 3) == (1, 2, 3)
        E         At index 1 diff: 4 != 2
        E         Full diff:
        E         - (1, 2, 3)
        E         ?     ^
        E         + (1, 4, 3)
        E         ?     ^
        """,
    ),
    (   # Compare sets
        """
        def test_this():
            result =   {1, 4, 3}
            expected = {1, 2, 3}
            assert result == expected
        """,
        """
        >       assert result == expected
        E       assert {1, 3, 4} == {1, 2, 3}
        E         Extra items in the result set:
        E         4
        E         Extra items in the expected set:
        E         2
        E         Full diff:
        E         - {1, 2, 3}
        E         ?     ^  ^
        E         + {1, 3, 4}
        E         ?     ^  ^
        """,
    ),
    (   # Compare dicts with differing keys
        """
        def test_this():
            result =   {1: 'spam', 3: 'eggs'}
            expected = {1: 'spam', 2: 'eggs'}
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert {1: 'spam', 3: 'eggs'} == {1: 'spam', 2: 'eggs'}
        E         Common items:
        E         {1: 'spam'}
        E         Result contains 1 more item:
        E         {3: 'eggs'}
        E         Expected contains 1 more item:
        E         {2: 'eggs'}
        E         Full diff:
        E         - {1: 'spam', 2: 'eggs'}
        E         ?             ^
        E         + {1: 'spam', 3: 'eggs'}
        E         ?             ^
        """,
    ),
    (   # Compare dicts with differing values
        """
        def test_this():
            result =   {1: 'spam', 2: 'eggs'}
            expected = {1: 'spam', 2: 'bacon'}
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert {1: 'spam', 2: 'eggs'} == {1: 'spam', 2: 'bacon'}
        E         Common items:
        E         {1: 'spam'}
        E         Differing items:
        E         {2: 'eggs'} != {2: 'bacon'}
        E         Full diff:
        E         - {1: 'spam', 2: 'bacon'}
        E         ?                 ^^^^^
        E         + {1: 'spam', 2: 'eggs'}
        E         ?                 ^^^^
        """,
    ),
    (   # Compare dicts with differing items
        """
        def test_this():
            result =   {1: 'spam', 2: 'eggs'}
            expected = {1: 'spam', 3: 'bacon'}
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert {1: 'spam', 2: 'eggs'} == {1: 'spam', 3: 'bacon'}
        E         Common items:
        E         {1: 'spam'}
        E         Result contains 1 more item:
        E         {2: 'eggs'}
        E         Expected contains 1 more item:
        E         {3: 'bacon'}
        E         Full diff:
        E         - {1: 'spam', 3: 'bacon'}
        E         ?             ^   ^^^^^
        E         + {1: 'spam', 2: 'eggs'}
        E         ?             ^   ^^^^
        """,
    ),
    (   # Compare data classes
        """
        from dataclasses import dataclass

        @dataclass
        class A:
            a: int
            b: str

        def test_this():
            result =   A(1, 'spam')
            expected = A(2, 'spam')
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert A(a=1, b='spam') == A(a=2, b='spam')
        E         Matching attributes:
        E         ['b']
        E         Differing attributes:
        E         a: 1 != 2
        """,
    ),
    (   # Compare attrs classes
        """
        import attr

        @attr.s(auto_attribs=True)
        class A:
            a: int
            b: str

        def test_this():
            result =   A(1, 'spam')
            expected = A(1, 'eggs')
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert A(a=1, b='spam') == A(a=1, b='eggs')
        E         Matching attributes:
        E         ['a']
        E         Differing attributes:
        E         b: 'spam' != 'eggs'
        """,
    ),
    (   # Compare strings
        """
        def test_this():
            result =   "spmaeggs"
            expected = "spameggs"
            assert result == expected
        """,
        """
        >       assert result == expected
        E       AssertionError: assert 'spmaeggs' == 'spameggs'
        E         - spameggs
        E         ?    -
        E         + spmaeggs
        E         ?   +
        """,
    ),
    (   # Test "no in" string
        """
        def test_this():
            result =   "spam bacon eggs"
            assert "bacon" not in result
        """,
        """
        >       assert "bacon" not in result
        E       AssertionError: assert 'bacon' not in 'spam bacon eggs'
        E         'bacon' is contained here:
        E           spam bacon eggs
        E         ?      +++++
        """,
    ),
]


@pytest.mark.parametrize('code, expected', TESTCASES)
def test_error_diff(code, expected, testdir):
    expected = [l.lstrip() for l in expected.splitlines()]
    p = testdir.makepyfile(code)
    result = testdir.runpytest(p, '-vv')
    result.stdout.fnmatch_lines(expected)
    assert result.ret == 1

[sscherfke]

@nedbat I think these changes will be released with pytest 6, so there will be some time for deprecation :)

[mcarans]

@sscherfke If your tests are easier to read than the existing, then it would make sense to me to keep yours. Perhaps it's easiest to leave them and let the person who reviews your PR comment on it.

[nedbat]

In my project, I've tried hard to make the asserts use assert expected == actual precisely because that order caused the pytest diff to make sense. When pytest 6 lands, I will switch them back. It's annoying, but is manageable.

[nicoddemus]

Can you elaborate on why you've given a "laugh" reaction to @nedbat's comment, please?

Oh I didn't mean to be dismissive, sorry if it came across that way. I thought @nedbat was being cheeky/humourous like: "well that was so wrong that I managed around that, and now that you've fixed it it will be more trouble for me" kind of way.

[blueyed]

I can get used to it myself I guess.. :)

To try this out already it can be simulated (I assume) via the following (e.g. in a conftest.py) - it also adds colors (hackish, of course):

@pytest.hookimpl(hookwrapper=True)
def pytest_assertrepr_compare(config, op, left, right):
    outcome = yield
    result = outcome.get_result()
    if not result:
        return
    lines = result[0]
    for idx, line in enumerate(lines):
        if line.startswith("+ "):
            result[0][idx] = "- {}".format(line[2:])
        elif line.startswith("- "):
            result[0][idx] = "+ {}".format(line[2:])

    # Colors.
    for idx, line in enumerate(lines):
        if line.startswith("+ "):
            result[0][idx] = "\x1b[31m{}\x1b[0m".format(line)
        elif line.startswith("- "):
            result[0][idx] = "\x1b[32m{}\x1b[0m".format(line)
        elif line.startswith("? "):
            result[0][idx] = "\x1b[33m{}\x1b[0m".format(line)

    outcome.force_result(result)

With #6673 however the order is changed (which I guess I cannot get used to (FWIW) ;)): https://github.com/pytest-dev/pytest/pull/6673/files#diff-3537ef5ebd330a3eebd2b4bae9d85c33R22-R28