Add support to display field names in namedtuple diffs. #7553

tirkarthi · 2020-07-28T13:54:32Z

Here is a quick checklist that should be present in PRs.

Include documentation when adding new features.
Include new tests or update existing tests when applicable.
Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

Fixes Show namedtuple field name in assertion error message #7527

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
Add yourself to AUTHORS in alphabetical order.

The-Compiler · 2020-07-28T14:01:59Z

I think we should fall back to the remaining tuple elements

What do you mean? If _fields somehow doesn't match up with the tuple length? I'd rather just have an additional len(obj._fields) == len(obj) check or so then (handling TypeError if len isn't available).

graingert · 2020-07-28T14:18:03Z

src/_pytest/assertion/util.py

@@ -423,6 +431,8 @@ def _compare_eq_cls(
        fields_to_check = [
            field.name for field in all_fields if getattr(field, ATTRS_EQ_FIELD)
        ]
+    elif isnamedtuple(left):
+        fields_to_check = left._fields


moved from: #7527 (comment)

FWIW, I like the "._fields" approach but I think it should fallback to regular tuple comparison if the attributes are not exhausted.

Also I think we should just change the display of fields using _fields, and continue looking up items in the tuple by index/iteration

FWIW, I like the "._fields" approach but I think it should fallback to regular tuple comparison if the attributes are not exhausted.

Is this about checking the length of _fields and the object itself in the other comment?

Also I think we should just change the display of fields using _fields, and continue looking up items in the tuple by index/iteration

Sorry, I don't get this. Is this about _fields being potentially used for some other purpose in custom subclass of tuple and to rely on iterating the namedtuple by index?

graingert · 2020-07-28T14:23:58Z

src/_pytest/assertion/util.py

@@ -423,6 +431,8 @@ def _compare_eq_cls(
        fields_to_check = [
            field.name for field in all_fields if getattr(field, ATTRS_EQ_FIELD)
        ]
+    elif isnamedtuple(left):
+        fields_to_check = left._fields


@The-Compiler

I think we should fall back to the remaining tuple elements

What do you mean? If _fields somehow doesn't match up with the tuple length? I'd rather just have an additional len(obj._fields) == len(obj) check or so then (handling TypeError if len isn't available).

I think it might need len(set(obj._fields)) == len(obj)

or something like this?

fields_and_items = [ (field if getattr(obj, field) is item else i), item for i, (field, item) in enumerate(zip_longest(obj._fields, obj)) ]

asottile · 2020-07-28T20:27:38Z

testing/test_assertion.py

@@ -981,6 +982,30 @@ class SimpleDataObjectTwo:
        assert lines is None


+class TestAssert_reprcompare_namedtuple:
+    def test_namedtuple(self) -> None:
+        SimpleDataObject = namedtuple("SimpleDataObject", ["field_a", "field_b"])


since collections is already imported this could be collections.namedtuple

collections.abc is imported. I don't see collections being imported. Are you suggesting to just do import collections and use collections.namedtuple?

import collections.abc is approximately equivalent to

importlib.import_module("collections.abc") import collections

TIL, I mostly prefer it to be explicit. Even on import collections the pre-commit removes it. So I have made the changes as suggested.

the hook removes it because import a and import a.b.c are duplicate imports

tekumara · 2020-07-30T04:03:13Z

Thanks so much for working on this @tirkarthi! I'd love to see this feature land.

What's the best way to test this? I pulled your fork and tried it out but I couldn't get the field names to show.

tirkarthi · 2020-07-30T11:20:48Z

@tekumara You're welcome. I can see the changes with the example from the report #7527 . I tried cloning and did python setup.py install. Can you check the relevant installed files to see if the patch is there? The patch is also small that it extends on the work already done for dataclass and attrs.

bluetech

Good idea! LGTM, except some comments on the tests.

bluetech · 2020-08-14T06:54:43Z

src/_pytest/assertion/util.py

@@ -412,9 +418,11 @@ def _compare_eq_cls(
    left: Any,
    right: Any,
    verbose: int,
-    type_fns: Tuple[Callable[[Any], bool], Callable[[Any], bool]],
+    type_fns: Tuple[


Not related to this PR, but this type_fns argument is really odd, it is always the same and can just be dropped & inlined instead.

If you'd like to do this that'd be nice (in a separate commit), but you don't have to :)

FWIW it was added in a663f60 as part of #3776 - maybe @alysivji remembers what the intention behind it was?

Believe I didn't inline the arguments as the code would have been formatted over multiple lines.

bluetech · 2020-08-14T06:57:44Z

testing/test_assertion.py

+        for line in lines[2:]:
+            assert "field_a" not in line
+
+    def test_comparing_two_different_namedtuple(self):


This test tests python itself, not pytest. What did you intend to test here?

bluetech · 2020-08-14T06:58:51Z

testing/test_assertion.py

+        right = SimpleDataObject(1, "c")
+
+        lines = callequal(left, right)
+        assert lines is not None


I think it would be best to just compare all of lines (i.e. assert lines == [...]).

testing/test_assertion.py

graingert · 2020-08-14T09:37:21Z

testing/test_assertion.py

@@ -981,6 +981,36 @@ class SimpleDataObjectTwo:
        assert lines is None


+class TestAssert_reprcompare_namedtuple:
+    def test_namedtuple(self) -> None:


Can you also add some tests with objects that have _fields, are tuples, but are not NamedTuples?

Also some tests were _fields doesn't match __getattr__ and len(type(o)_fields) != len(o)

tirkarthi · 2020-08-19T14:17:15Z

@graingert Good points that illustrate it can result in false positives. I have come to below definition of isnameduple as below but this will still cause problems in below test case. I feel adding too much cases in isnamedtuple will cause also slowdown the diff output since isnamedtuple. There could be namedtuple like objects as below that might cause surprising outputs. I guess the PR is moving more towards the line where its not feasible to display the output user wants with no standard way to detect namedtuples.

def isnamedtuple(obj: Any) -> bool:
    fields = getattr(obj, "_fields", None)
    return (
        isinstance(obj, tuple) # Check for tuple
        and fields is not None # Check for _fields
        and issequence(fields) # Check that its iterable
        and len(obj) == len(fields) # Check for len of both
        and all(hasattr(obj, field) for field in fields) # Check all attrs are present
    )

2 elements from constructor. 2 attributes as _fields with argument to constructor and _fields being different

def test_comparing_custom_object_fields_set_different_arity() -> None:
    class SimpleDataObjectOne(tuple):
        _fields = ["field_a", "field_b"]
    
    left = SimpleDataObjectOne((1, 2))
    left.field_a = "a"
    left.field_b = "d"
    right = SimpleDataObjectOne((2, 1))
    left.field_a = "a"
    left.field_b = "b"

    assert left == right

Current pytest output

pytest -vvv /tmp/test_namedtuple1.py -k test_comparing_custom_object_fields_set_different_arity
=============================== test session starts ===============================
platform linux -- Python 3.8.0, pytest-5.4.1, py-1.8.1, pluggy-0.13.1 -- /root/py38-venv/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/root/pytest/.hypothesis/examples')
rootdir: /tmp
plugins: django-3.9.0, forked-1.1.3, env-0.6.2, hypothesis-5.20.2, xdist-1.31.0, aiohttp-0.3.0, mock-3.1.1, timeout-1.3.4, cov-2.8.1, pythonpath-0.7.3
collected 5 items / 4 deselected / 1 selected                                     

../../tmp/test_namedtuple1.py::test_comparing_custom_object_fields_set_different_arity FAILED [100%]

==================================== FAILURES =====================================
_____________ test_comparing_custom_object_fields_set_different_arity _____________

    def test_comparing_custom_object_fields_set_different_arity() -> None:
        class SimpleDataObjectOne(tuple):
            _fields = ["field_a", "field_b"]
    
        left = SimpleDataObjectOne((1, 2))
        left.field_a = "a"
        left.field_b = "d"
        right = SimpleDataObjectOne((2, 1))
        left.field_a = "a"
        left.field_b = "b"
    
>       assert left == right
E       assert (1, 2) == (2, 1)
E         At index 0 diff: 1 != 2
E         Full diff:
E         - (2, 1)
E         + (1, 2)

/tmp/test_namedtuple1.py:68: AssertionError
============================= short test summary info =============================
FAILED ../../tmp/test_namedtuple1.py::test_comparing_custom_object_fields_set_different_arity
========================= 1 failed, 4 deselected in 0.14s =========================

With patch though length and attrs are present the util function might get confused with _fields being same and the arguments to constructor to be different.

pytest -vvv /tmp/test_namedtuple1.py -k test_comparing_custom_object_fields_set_different_arity
=============================== test session starts ===============================
platform linux -- Python 3.9.0b5, pytest-6.0.0rc2.dev72+gbecc9f941.d20200819, py-1.9.0, pluggy-0.12.0 -- /root/py39-venv/bin/python3.9
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/root/pytest/.hypothesis/examples')
rootdir: /tmp
plugins: hypothesis-4.43.1, cov-2.8.1
collected 5 items / 4 deselected / 1 selected                                     

../../tmp/test_namedtuple1.py::test_comparing_custom_object_fields_set_different_arity FAILED [100%]

==================================== FAILURES =====================================
_____________ test_comparing_custom_object_fields_set_different_arity _____________

    def test_comparing_custom_object_fields_set_different_arity() -> None:
        class SimpleDataObjectOne(tuple):
            _fields = ["field_a", "field_b"]
    
        left = SimpleDataObjectOne((1, 2))
        left.field_a = "a"
        left.field_b = "d"
        right = SimpleDataObjectOne((2, 1))
        left.field_a = "a"
        left.field_b = "b"
    
>       assert left == right
E       AssertionError: assert (1, 2) == (2, 1)
E         (pytest_assertion plugin: representation of details failed: /root/pytest/src/_pytest/assertion/util.py:448: AttributeError: 'SimpleDataObjectOne' object has no attribute 'field_a'.
E          Probably an object has a faulty __repr__.)

/tmp/test_namedtuple1.py:68: AssertionError
============================= short test summary info =============================
FAILED ../../tmp/test_namedtuple1.py::test_comparing_custom_object_fields_set_different_arity
========================= 1 failed, 4 deselected in 0.29s =========================

The-Compiler · 2020-08-19T14:22:57Z

IMHO we shouldn't care (or bikeshed) too much about cases which are very unlikely to happen. At least the Debian Code Search has no results at all for _fields =.

We should make sure pytest doesn't explode entirely when comparing them, but the "representation of details failed" message from above seems quite reasonable to me.

tirkarthi · 2020-08-19T14:34:29Z

I have seen projects using _fields_. There is one project schematics that seems to use _fields from initial grep like https://github.com/schematics/schematics/blob/90dee53fd1d5c29f2c947bec6f5ffe5f74305ab1/tests/test_datastructures.py#L95 . Searching for _fields in around 2000 top PyPI projects do return 236 matches. Some of them are tuple of tuples where they are filtered. I haven't explored all instances but full log at https://gist.github.com/tirkarthi/e71470c370a1910da58adfc1da8d5dad . Let me know if you need any other pattern searches.

rg -t py ' _fields =' | wc
    236     978   11409

bluetech · 2020-08-28T10:19:45Z

IMO the current check return isinstance(obj, tuple) and getattr(obj, "_fields", None) is not None is sufficient, but if we want to be more confident we can also check for existence of _field_defaults.

bluetech · 2020-10-28T14:00:19Z

@tirkarthi do you think you'll have time to work on this? If not, I can finish it. I think it would be a nice feature for the next release.

tirkarthi · 2020-10-28T15:29:40Z

@bluetech Sorry, I lost track of the discussion and might not be able to continue. Feel free to use the patch as needed. Thanks :)

It doesn't serve any purpose that I am able to discern.

bluetech

I pushed an updated version.

Note that this is strictly restricted to the case where the namedtuple types match, which avoids most of the pitfalls, while still catching 95% of the cases, in particular the "proto-dataclass" case.

nicoddemus

Jumping a bit late into the party, but thanks everyone!

Left just a small suggestion to the CHANGELOG message, other than that LGTM. 👍

changelog/7527.improvement.rst

tirkarthi · 2020-11-01T03:09:57Z

Thanks @bluetech and everybody for the review :)

graingert reviewed Jul 28, 2020

View reviewed changes

asottile reviewed Jul 28, 2020

View reviewed changes

bluetech requested changes Aug 14, 2020

View reviewed changes

graingert reviewed Aug 14, 2020

View reviewed changes

assertion/util: remove unhelpful type_fns indirection

5913cd2

It doesn't serve any purpose that I am able to discern.

bluetech force-pushed the namedtuple-diff branch from becc9f9 to a83cc96 Compare October 30, 2020 20:06

bluetech reviewed Oct 30, 2020

View reviewed changes

bluetech force-pushed the namedtuple-diff branch from a83cc96 to 4487b17 Compare October 30, 2020 20:18

bluetech approved these changes Oct 31, 2020

View reviewed changes

nicoddemus approved these changes Oct 31, 2020

View reviewed changes

changelog/7527.improvement.rst Outdated Show resolved Hide resolved

Add support to display field names in namedtuple diffs.

9a0f4e5

bluetech force-pushed the namedtuple-diff branch from 4487b17 to 9a0f4e5 Compare October 31, 2020 12:42

bluetech merged commit 1c18fb8 into pytest-dev:master Oct 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to display field names in namedtuple diffs. #7553

Add support to display field names in namedtuple diffs. #7553

tirkarthi commented Jul 28, 2020

The-Compiler commented Jul 28, 2020

graingert Jul 28, 2020

tirkarthi Jul 28, 2020

graingert Jul 28, 2020 •

edited

asottile Jul 28, 2020

tirkarthi Jul 29, 2020

graingert Jul 29, 2020 •

edited

tirkarthi Jul 29, 2020

asottile Jul 29, 2020

tekumara commented Jul 30, 2020

tirkarthi commented Jul 30, 2020

bluetech left a comment

bluetech Aug 14, 2020

The-Compiler Aug 14, 2020

alysivji Aug 14, 2020 •

edited

bluetech Aug 14, 2020

bluetech Aug 14, 2020

graingert Aug 14, 2020

tirkarthi commented Aug 19, 2020

The-Compiler commented Aug 19, 2020

tirkarthi commented Aug 19, 2020

bluetech commented Aug 28, 2020

bluetech commented Oct 28, 2020

tirkarthi commented Oct 28, 2020

bluetech left a comment

nicoddemus left a comment

tirkarthi commented Nov 1, 2020

Add support to display field names in namedtuple diffs. #7553

Add support to display field names in namedtuple diffs. #7553

Conversation

tirkarthi commented Jul 28, 2020

The-Compiler commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jul 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Jul 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekumara commented Jul 30, 2020

tirkarthi commented Jul 30, 2020

bluetech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alysivji Aug 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Aug 19, 2020

The-Compiler commented Aug 19, 2020

tirkarthi commented Aug 19, 2020

bluetech commented Aug 28, 2020

bluetech commented Oct 28, 2020

tirkarthi commented Oct 28, 2020

bluetech left a comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

tirkarthi commented Nov 1, 2020

graingert Jul 28, 2020 •

edited

graingert Jul 29, 2020 •

edited

alysivji Aug 14, 2020 •

edited