Make dataclasses/attrs comparison recursive, fixes #4675 #6835

iwanb · 2020-02-28T11:29:21Z

I extracted the equality check in its own function and I call it from the dataclass comparison function to get recursive comparisons.

It could also be used for dict comparison, but the issue mentioned there was already some mechanism for dicts (I didn't see it in the code though).

Fixes #4675.

bluetech

Nice! I really missed this in some previous projects where I had large elaborate attrs types.

It will be easier to evaluate this change if the test case is made a bit more complex. Specifically I'd suggest:

Keep the 2nd level diff you added.
Add another dataclass attribute (so 3 levels of nesting) with a differing attribute in that.

This will test more than one diff and more than one level of nesting.

iwanb · 2020-02-28T12:38:03Z

I also realized that the object could somehow be self-referencing, should I add a recursion limit or keep a set of already-seen object? I'll add a test for that as well.

iwanb · 2020-02-28T12:44:42Z

It fails with RecursionError, is that acceptable? I suppose if someone creates a self-referencing dataclass with the compare setting set on that field, we can assume it's not correct.

bluetech · 2020-02-28T12:57:59Z

I wonder if this is the first case of possible cycles in pytets's diff reporting (sorry -- I am too lazy to check 😴)? If this is already handled somewhere, I'd do the same. If it's the first case, then I do think it should be handled. RecursionError isn't very nice, cycles are perfectly legit after all.

iwanb · 2020-02-28T13:13:27Z

They are legit but e.g. the dataclasses.astuple or .asdict already recurse and assume that the dataclass is not recursive and it already only looks at fields with the compare attribute being True.

I don't see other cases of recursion, but I could add a depth limit for example (it just makes the code less clean).

src/_pytest/assertion/util.py

blueyed · 2020-02-28T18:40:26Z

I wonder if this is the first case of possible cycles in pytets's diff reporting

Usually repr() is not used directly, but via the notorious _pytest._io.saferepr.saferepr, which could be considered to be used here also, but it is notorious since it often shortens too much etc.
I think using pprint.safe_repr is a good fix for now probably.
Ref: #6723

blueyed · 2020-02-28T18:48:55Z

I think this should pass through some state maybe (which I've thought to be useful before already - instead of just "verbose"), so that it could stop on recursions itself / handle self-referencing objects.
(given that _compare_eq_any should be used for dicts then maybe also)

iwanb · 2020-03-02T11:01:16Z

I feel like to implement this properly, the hook itself should be able to return recursive comparisons to be done, and the self-reference issue could be handled at the call site. That way custom assertion rewrite would automatically be handled for recursive structure, and the implementation wouldn't have to worry about self-reference.

Maybe one backward compatible way to implement this would be to add a hook wrapper around pytest_assertrepr_compare which does the self-reference check, and allow pytest_assertrepr_compare to be called recursively from itself? The hookwrapper would be aware of the recursion and could prevent an infinite loop.

That's a bit more work of course, otherwise I'll just add a list of already seen objects in the internal comparison functions (well only the dataclass one for now).

iwanb · 2020-05-05T09:56:54Z

Any preferred solution for this?

nicoddemus

Thanks @iwanb!

While I agree we should handle recursion, I'm also happy with working to get the current work merged and iterate over the recursion problem separately. 👍

testing/test_assertion.py

src/_pytest/assertion/util.py

Zac-HD · 2020-06-02T08:28:50Z

@iwanb, @nicoddemus - how about we add @nicoddemus' last suggestion and then merge? It sounds like that will at least be an improvement over the status quo 🙂

iwanb · 2020-06-02T10:28:47Z

I missed the earlier reply, I've added the suggestions.

testing/test_assertion.py

src/_pytest/assertion/util.py

bluetech · 2020-06-02T13:33:34Z

Thanks @iwanb, when this is needed, it is very needed.

I once created an anonymized version of a big attrs type I have in a project, and I tried this PR on it.

Show

from __future__ import annotations
from typing import Optional, List
import attr
import enum


@attr.s(slots=True, frozen=True)
class Lorem:
    ipsum = attr.ib()
    dolor = attr.ib()
    sit = attr.ib()
    amet = attr.ib()
    consectetur = attr.ib()
    adipiscing = attr.ib()


@attr.s(slots=True, frozen=True)
class Fugiat:
    eiusmod = attr.ib()
    tempor = attr.ib()
    incididunt = attr.ib()
    labore = attr.ib()
    dolore = attr.ib()
    magna = attr.ib()
    aliqua = attr.ib()
    veniam = attr.ib()
    nostrud = attr.ib()
    exercitation = attr.ib()
    ullamco = attr.ib()
    laboris = attr.ib()
    commodo = attr.ib()
    consequat = attr.ib()
    aute = attr.ib()


@attr.s(slots=True, frozen=True)
class Invenire:
    irure = attr.ib()
    reprehenderit = attr.ib()
    voluptate = attr.ib()
    velit = attr.ib()
    esse = attr.ib()
    cillum = attr.ib()
    eepcillum = attr.ib()


@attr.s(slots=True, frozen=True)
class Tritani:
    nulla = attr.ib()
    name = attr.ib()
    value = attr.ib()
    pariatur = attr.ib()
    exceptuer = attr.ib()


@attr.s(slots=True, frozen=True)
class Laborum:
    type = attr.ib()
    urangulal = attr.ib()
    ipsumal = attr.ib()
    occaecat = attr.ib()
    cupidatat = attr.ib()
    proident = attr.ib()


class Aliquip(enum.IntEnum):
    Aliquip1 = 1
    Aliquip2 = 2
    Aliquip3 = 3
    Aliquip4 = 4
    Aliquip5 = 5


@attr.s(slots=True, frozen=True)
class Assentior:
    aliquip = attr.ib(type=Aliquip)
    culpa = attr.ib()
    fugiat = attr.ib(type=Optional[Fugiat])
    invenire = attr.ib(type=Optional[Invenire])
    deserunt = attr.ib()
    lorem = attr.ib(type=Optional[Lorem])
    mollit = attr.ib()
    laborums = attr.ib(type=Optional[List[Laborum]])
    tantas = attr.ib()
    nominati = attr.ib()
    fabulas = attr.ib()
    tritani = attr.ib(type=Optional[Tritani])

    replace = attr.evolve


@attr.s(slots=True, frozen=True)
class Dignissim:
    assentior = attr.ib(type=Assentior)
    new_cupidatat = attr.ib()
    laoreet = attr.ib()
    rationibus = attr.ib()

    replace = attr.evolve


obj = Dignissim(
    assentior=Assentior(
        aliquip=Aliquip.Aliquip1,
        culpa=6,
        fugiat=Fugiat(
            eiusmod='aaaaaaaaaaaaaaaa',
            tempor='bbbbbb',
            incididunt='CCCCCCCCCCCCCCCCCCC',
            labore='dddddddddd',
            dolore='eeeeeeeeee',
            magna='fffffffffff',
            aliqua='gggggggggggggg',
            veniam=None,
            nostrud=None,
            exercitation=None,
            ullamco=None,
            laboris=None,
            commodo=None,
            consequat=None,
            aute=None,
        ),
        invenire=Invenire(
            irure=53,
            reprehenderit=153,
            voluptate=242,
            velit=100,
            esse=5035,
            cillum=53,
            eepcillum=422,
        ),
        deserunt=True,
        lorem=Lorem(
            ipsum=b'',
            dolor=[
                'xx',
            ],
            sit=[
                'yy'
            ],
            amet=[
                'zz',
            ],
            consectetur=b'sdfsd',
            adipiscing=b'adfsdf',
        ),
        mollit=4294967294,
        laborums=[
            Laborum(
                type=23,
                urangulal=b"abc",
                ipsumal=None,
                occaecat=b'sdf',
                cupidatat=13,
                proident=None,
            ),
        ],
        tantas='sdfsdlxcv49249sdfs90sdf==',
        nominati=-1,
        fabulas=False,
        tritani=None,
    ),
    new_cupidatat=13,
    laoreet=1,
    rationibus=False,
)

def test_cmp():
    assert obj == obj.replace(assentior=obj.assentior.replace(culpa=7, aliquip=Aliquip.Aliquip3))

This is the output:

__________________________________________________________________________________ test_cmp __________________________________________________________________________________

    def test_cmp():
>       assert obj == obj.replace(assentior=obj.assentior.replace(culpa=7, aliquip=Aliquip.Aliquip3))
E       AssertionError: assert Dignissim(assentior=Assentior(aliquip=<Aliquip.Aliquip1: 1>, culpa=6, fugiat=Fugiat(eiusmod='aaaaaaaaaaaaaaaa', tempor='bbbbbb', incididunt='CCCCCCCCCCCCCCCCCCC', labore='dddddddddd', dolore='eeeeeeeeee', magna='fffffffffff', aliqua='gggggggggggggg', veniam=None, nostrud=None, exercitation=None, ullamco=None, laboris=None, commodo=None, consequat=None, aute=None), invenire=Invenire(irure=53, reprehenderit=153, voluptate=242, velit=100, esse=5035, cillum=53, eepcillum=422), deserunt=True, lorem=Lorem(ipsum=b'', dolor=['xx'], sit=['yy'], amet=['zz'], consectetur=b'sdfsd', adipiscing=b'adfsdf'), mollit=4294967294, laborums=[Laborum(type=23, urangulal=b'abc', ipsumal=None, occaecat=b'sdf', cupidatat=13, proident=None)], tantas='sdfsdlxcv49249sdfs90sdf==', nominati=-1, fabulas=False, tritani=None), new_cupidatat=13, laoreet=1, rationibus=False) == Dignissim(assentior=Assentior(aliquip=<Aliquip.Aliquip3: 3>, culpa=7, fugiat=Fugiat(eiusmod='aaaaaaaaaaaaaaaa', tempor='bbbbbb', incididunt='CCCCCCCCCCCCCCCCCCC', labore='dddddddddd', dolore='eeeeeeeeee', magna='fffffffffff', aliqua='gggggggggggggg', veniam=None, nostrud=None, exercitation=None, ullamco=None, laboris=None, commodo=None, consequat=None, aute=None), invenire=Invenire(irure=53, reprehenderit=153, voluptate=242, velit=100, esse=5035, cillum=53, eepcillum=422), deserunt=True, lorem=Lorem(ipsum=b'', dolor=['xx'], sit=['yy'], amet=['zz'], consectetur=b'sdfsd', adipiscing=b'adfsdf'), mollit=4294967294, laborums=[Laborum(type=23, urangulal=b'abc', ipsumal=None, occaecat=b'sdf', cupidatat=13, proident=None)], tantas='sdfsdlxcv49249sdfs90sdf==', nominati=-1, fabulas=False, tritani=None), new_cupidatat=13, laoreet=1, rationibus=False)
E         Matching attributes:
E         ['new_cupidatat', 'laoreet', 'rationibus']
E         Differing attributes:
E         assentior: Assentior(aliquip=<Aliquip.Aliquip1: 1>, culpa=6, fugiat=Fugiat(eiusmod='aaaaaaaaaaaaaaaa', tempor='bbbbbb', incididunt='CCCCCCCCCCCCCCCCCCC', labore='dddddddddd', dolore='eeeeeeeeee', magna='fffffffffff', aliqua='gggggggggggggg', veniam=None, nostrud=None, exercitation=None, ullamco=None, laboris=None, commodo=None, consequat=None, aute=None), invenire=Invenire(irure=53, reprehenderit=153, voluptate=242, velit=100, esse=5035, cillum=53, eepcillum=422), deserunt=True, lorem=Lorem(ipsum=b'', dolor=['xx'], sit=['yy'], amet=['zz'], consectetur=b'sdfsd', adipiscing=b'adfsdf'), mollit=4294967294, laborums=[Laborum(type=23, urangulal=b'abc', ipsumal=None, occaecat=b'sdf', cupidatat=13, proident=None)], tantas='sdfsdlxcv49249sdfs90sdf==', nominati=-1, fabulas=False, tritani=None) != Assentior(aliquip=<Aliquip.Aliquip3: 3>, culpa=7, fugiat=Fugiat(eiusmod='aaaaaaaaaaaaaaaa', tempor='bbbbbb', incididunt='CCCCCCCCCCCCCCCCCCC', labore='dddddddddd', dolore='eeeeeeeeee', magna='fffffffffff', aliqua='gggggggggggggg', veniam=None, nostrud=None, exercitation=None, ullamco=None, laboris=None, commodo=None, consequat=None, aute=None), invenire=Invenire(irure=53, reprehenderit=153, voluptate=242, velit=100, esse=5035, cillum=53, eepcillum=422), deserunt=True, lorem=Lorem(ipsum=b'', dolor=['xx'], sit=['yy'], amet=['zz'], consectetur=b'sdfsd', adipiscing=b'adfsdf'), mollit=4294967294, laborums=[Laborum(type=23, urangulal=b'abc', ipsumal=None, occaecat=b'sdf', cupidatat=13, proident=None)], tantas='sdfsdlxcv49249sdfs90sdf==', nominati=-1, fabulas=False, tritani=None)
E         
E         Recursive from previous comparison
E         Matching attributes:
E         ['fugiat',
E          'invenire',
E          'deserunt',
E          'lorem',
E          'mollit',
E          'laborums',
E          'tantas',
E          'nominati',
E          'fabulas',
E          'tritani']
E         Differing attributes:
E         aliquip: <Aliquip.Aliquip1: 1> != <Aliquip.Aliquip3: 3>
E         
E         Recursive from previous comparison
E         +<Aliquip.Aliquip1: 1>
E         -<Aliquip.Aliquip3: 3>
E         culpa: 6 != 7
E         
E         Recursive from previous comparison
E         +6
E         -7

x.py:169: AssertionError

I only have one quip about it:

I created two differing attributes. One level of the tree just starts immediately after the previous text (e.g. the culpa: 6 != 7 line above), which is a bit confusing, and can probably be made more confusing if change other attributes in the tree. Maybe whenever we recurse one level, we should add some indentation so the tree structure is revealed?

iwanb · 2020-06-02T13:50:41Z

It would for sure be nice to have a visual tree structure for the error, but it's a bigger change as the functions need to be made aware that they are recursive (e.g. with a counter).

testing/test_assertion.py

bluetech · 2020-06-02T13:54:13Z

It would for sure be nice to have a visual tree structure for the error, but it's a bigger change as the functions need to be made aware that they are recursive (e.g. with a counter).

No problem, I'll open an issue about after this is merged, to keep track.

bluetech · 2020-06-03T13:09:55Z

@nicoddemus I think this is ready to merge, pending your review. I'll open a follow up issue after merge.

nicoddemus · 2020-06-09T18:01:48Z

Thanks @iwanb and sorry for the delay!

iwanb · 2020-06-09T18:04:07Z

No problem, thanks for fixing it up!

bluetech · 2020-06-10T22:41:48Z

Follow up issue in #7348.

iwanb force-pushed the master branch from a55bd1c to d19f8c6 Compare February 28, 2020 11:32

bluetech reviewed Feb 28, 2020

View reviewed changes

iwanb force-pushed the master branch from d19f8c6 to aa61330 Compare February 28, 2020 12:55

blueyed reviewed Feb 28, 2020

View reviewed changes

src/_pytest/assertion/util.py Show resolved Hide resolved

nicoddemus reviewed May 5, 2020

View reviewed changes

testing/test_assertion.py Show resolved Hide resolved

src/_pytest/assertion/util.py Show resolved Hide resolved

iwanb force-pushed the master branch from aa61330 to 7fd8f96 Compare June 2, 2020 10:28

RonnyPfannschmidt requested review from nicoddemus and bluetech June 2, 2020 13:05

bluetech reviewed Jun 2, 2020

View reviewed changes

testing/test_assertion.py Outdated Show resolved Hide resolved

src/_pytest/assertion/util.py Outdated Show resolved Hide resolved

iwanb force-pushed the master branch from 7fd8f96 to 7d3d5db Compare June 2, 2020 13:41

bluetech reviewed Jun 2, 2020

View reviewed changes

testing/test_assertion.py Outdated Show resolved Hide resolved

bluetech approved these changes Jun 3, 2020

View reviewed changes

nicoddemus approved these changes Jun 9, 2020

View reviewed changes

ibriquem and others added 3 commits June 9, 2020 14:39

Make dataclasses/attrs comparison recursive, fixes pytest-dev#4675

e2e7f15

Update testing/test_assertion.py

09988f3

Update CHANGELOG

5a78df4

nicoddemus force-pushed the master branch from cbafcb0 to 5a78df4 Compare June 9, 2020 17:43

nicoddemus added 2 commits June 9, 2020 14:48

Fix mypy checks

c229d6f

Fix typo

10cee92

nicoddemus merged commit c83801e into pytest-dev:master Jun 9, 2020

bluetech mentioned this pull request Jun 10, 2020

Improve recursive diff output for comparison asserts on dataclasses/attrs #7348

Closed

westurner mentioned this pull request Feb 9, 2021

Functional/Unit/Integration Tests? sugarlabs/Pippy#84

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make dataclasses/attrs comparison recursive, fixes #4675 #6835

Make dataclasses/attrs comparison recursive, fixes #4675 #6835

iwanb commented Feb 28, 2020 •

edited by bluetech

bluetech left a comment

iwanb commented Feb 28, 2020

iwanb commented Feb 28, 2020

bluetech commented Feb 28, 2020

iwanb commented Feb 28, 2020

blueyed commented Feb 28, 2020 •

edited

blueyed commented Feb 28, 2020

iwanb commented Mar 2, 2020

iwanb commented May 5, 2020

nicoddemus left a comment

Zac-HD commented Jun 2, 2020

iwanb commented Jun 2, 2020

bluetech commented Jun 2, 2020

iwanb commented Jun 2, 2020

bluetech commented Jun 2, 2020

bluetech commented Jun 3, 2020

nicoddemus commented Jun 9, 2020

iwanb commented Jun 9, 2020

bluetech commented Jun 10, 2020

Make dataclasses/attrs comparison recursive, fixes #4675 #6835

Make dataclasses/attrs comparison recursive, fixes #4675 #6835

Conversation

iwanb commented Feb 28, 2020 • edited by bluetech

bluetech left a comment

Choose a reason for hiding this comment

iwanb commented Feb 28, 2020

iwanb commented Feb 28, 2020

bluetech commented Feb 28, 2020

iwanb commented Feb 28, 2020

blueyed commented Feb 28, 2020 • edited

blueyed commented Feb 28, 2020

iwanb commented Mar 2, 2020

iwanb commented May 5, 2020

nicoddemus left a comment

Choose a reason for hiding this comment

Zac-HD commented Jun 2, 2020

iwanb commented Jun 2, 2020

bluetech commented Jun 2, 2020

iwanb commented Jun 2, 2020

bluetech commented Jun 2, 2020

bluetech commented Jun 3, 2020

nicoddemus commented Jun 9, 2020

iwanb commented Jun 9, 2020

bluetech commented Jun 10, 2020

iwanb commented Feb 28, 2020 •

edited by bluetech

blueyed commented Feb 28, 2020 •

edited