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

Detailed assert failure introspection for attrs and dataclasses objects #3776

Merged
merged 14 commits into from
Nov 22, 2018

Conversation

alysivji
Copy link
Member

@alysivji alysivji commented Aug 3, 2018

Closes #3632

Adds richer equality comparison on AssertionError for objects creating using either attrs or dataclasses.


Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is just a guideline):

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the features branch for new features and removals/deprecations.
  • Include new tests or update existing tests when applicable.
  • Include documentation when adding new features.

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

  • Add yourself to AUTHORS in alphabetical order;


@dataclass
class SimpleDataObject(object):
field_a = field()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially used type annotations here, but tests failed for 2.7 so decided to use field as we only care about output, not the type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like 3.7 requires type annotation. Thoughts on a good way of getting both in there?

I can edit the class dict's __annotations__ item

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.502% when pulling 1e0c6ce on alysivji:attrs-n-dataclasses into a76cc8f on pytest-dev:features.

@@ -120,6 +120,12 @@ def isdict(x):
def isset(x):
return isinstance(x, (set, frozenset))

def isdatacls(obj):
return hasattr(obj, "__dataclass_fields__")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use getattr(x, ..., None) is not None or something like that instead, hasattr hides errors

@alysivji alysivji force-pushed the attrs-n-dataclasses branch 2 times, most recently from c8f5c44 to 5252680 Compare September 8, 2018 20:32
@codecov
Copy link

codecov bot commented Sep 8, 2018

Codecov Report

Merging #3776 into features will increase coverage by 0.04%.
The diff coverage is 98.03%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #3776      +/-   ##
============================================
+ Coverage     95.84%   95.88%   +0.04%     
============================================
  Files           111      111              
  Lines         24928    25126     +198     
  Branches       2438     2462      +24     
============================================
+ Hits          23892    24092     +200     
+ Misses          737      730       -7     
- Partials        299      304       +5
Flag Coverage Δ
#docs 29.31% <25.49%> (+0.36%) ⬆️
#doctesting 29.31% <25.49%> (+0.36%) ⬆️
#linting 29.31% <25.49%> (+0.36%) ⬆️
#linux 95.71% <98.03%> (+0.08%) ⬆️
#nobyte 92.58% <81.37%> (+1.36%) ⬆️
#numpy 93.36% <80.39%> (+51.72%) ⬆️
#pexpect 41.7% <4.9%> (+0.07%) ⬆️
#py27 93.95% <81.37%> (-0.06%) ⬇️
#py34 92.1% <81.37%> (+0.01%) ⬆️
#py35 92.12% <81.37%> (+0.01%) ⬆️
#py36 92.14% <81.37%> (-1.78%) ⬇️
#py37 94.09% <98.03%> (+1.94%) ⬆️
#trial 93.36% <80.39%> (+51.72%) ⬆️
#windows 94.1% <98.03%> (+0.85%) ⬆️
#xdist 93.95% <98.03%> (+0.38%) ⬆️
Impacted Files Coverage Δ
testing/test_assertion.py 97.5% <100%> (+0.38%) ⬆️
src/_pytest/assertion/util.py 97.54% <93.75%> (-0.58%) ⬇️
src/_pytest/helpconfig.py 97.24% <0%> (-1.84%) ⬇️
src/_pytest/config/__init__.py 94.14% <0%> (-0.87%) ⬇️
src/_pytest/config/findpaths.py 94.11% <0%> (-0.73%) ⬇️
src/_pytest/python.py 95.27% <0%> (-0.25%) ⬇️
testing/test_terminal.py 99.8% <0%> (-0.2%) ⬇️
src/_pytest/terminal.py 92.85% <0%> (-0.18%) ⬇️
testing/test_pluginmanager.py 99.58% <0%> (-0.03%) ⬇️
src/_pytest/main.py 96.96% <0%> (-0.01%) ⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b131214...d52ea4b. Read the comment docs.

@alysivji
Copy link
Member Author

alysivji commented Sep 9, 2018

@RonnyPfannschmidt @nicoddemus

I got this feature working and (mostly) tested, but am not sure how to test Python 3.7 code. I used the testdir fixture to create .py files for py37 tests, but I'm not sure how I can load the built-in plugins by name.

This is what I have so far:

@pytest.mark.skipif(sys.version_info < (3, 7), reason="Dataclasses in Python3.7+")
def test_dataclasses(self, testdir):
    testdir.makepyfile(
        """
        pytest_plugins = ["_pytest.assertion"]

        from dataclasses import dataclass, field

        def mock_config():
            class Config(object):
                verbose = False

                def getoption(self, name):
                    if name == "verbose":
                        return self.verbose
                    raise KeyError("Not mocked out: %s" % name)

            return Config()

        def callequal(left, right, verbose=False):
            config = mock_config()
            config.verbose = verbose
            return plugin.pytest_assertrepr_compare(config, "==", left, right)

        @dataclass
        class SimpleDataObject(object):
            field_a: int = field()
            field_b: str = field()

        left = SimpleDataObject(1, "b")
        right = SimpleDataObject(1, "c")

        lines = callequal(left, right)
        assert lines[1].startswith("Omitting 1 identical item")
        assert "Common attributes" not in lines
        for line in lines[1:]:
            assert "field_a" not in line
    """
    )
    result = testdir.runpytest()
    result.assert_outcomes(passed=4)

Am I structuring the tests correctly? Not sure if I'm thinking about this the right way. Would appreciate if you could point me in the right direction. Thank you so much for your help!

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall good work, the core of the implementation and the testing done look solid to me,

when the extraction part is re-factored this one is merge-able, even if we need to get back at the diff style later on

@@ -319,6 +329,41 @@ def _compare_eq_dict(left, right, verbose=False):
return explanation


def _compare_eq_class(left, right, verbose, type=None):
if type == "data":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets please change the magical strings to named functions that do the extraction and always require passing them

class_name = left.__class__.__name__
explanation += [("Differing attributes:")]
for k in diff:
explanation += [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i disagree with the chosen display style but cant provide any meaningful alternative yet - i would like to see some kind of diff style display using multiple lines

@alysivji alysivji changed the title Detailed assert failure introspection for attrs and dataclasses objects [WIP] Detailed assert failure introspection for attrs and dataclasses objects Sep 10, 2018
@RonnyPfannschmidt
Copy link
Member

@alysivji gentle ping - i'd like this to get into a mergable state - aka named functions done, and created a follow-up issue for the display of the diffs

would you like to do that, or can we pick it up?

@alysivji
Copy link
Member Author

alysivji commented Nov 7, 2018

Thanks for the ping. I will be sprinting at PyCon Canada on Monday and Tuesday next week and hope to get it done then.

I got the requested changes completed am not sure how to test Python3.7 code. I posted a question earlier but looking at it now, it's not too clear in what I am asking.

I'll take some time to refamiliarize myself with the code to clarify the question I have before this weekend. Hope to hit the ground first thing Monday morning.

@RonnyPfannschmidt
Copy link
Member

@alysivji the example you posted looks good to go/use wrt python3.7 - we might want to consider using the copy_example feature of pytester later on to have the actual file reside in the examples dir instead of a multiline sting in python

also thanks for the update and have a great time at the sprint

@alysivji alysivji force-pushed the attrs-n-dataclasses branch from 5252680 to 2bffd68 Compare November 12, 2018 21:47
@alysivji
Copy link
Member Author

alysivji commented Nov 12, 2018

@RonnyPfannschmidt Got most of the changes done. Wanted to throw it into CI and get eyes on it before I get back to polishing it tomorrow.

Will include output + other info in a message tmrw.

@alysivji
Copy link
Member Author

Been waiting a few hours for the AppVeyor builds to start, but sprints are winding down and I think I'm gonna have the same error pop up in the CI. Might as well ask the question.

My commit from yesterday failed CI: failure_demo.py is being run using Python 2.7 and I added an example with type annotations; this results in a SyntaxError. How should I handle this?

Error on AppVeyor

I also changed the formatting of the diffs. Looks a lot more readable to me.

    def test_equal_data_classes():
        example1 = SimpleDataObject(1, 'b')
        example2 = SimpleDataObject(1, 'b')

        assert example1 == example2

        example3 = SimpleDataObject(1, 'xc')
>       assert example1 == example3
E       AssertionError: assert SimpleDataObj..., field_b='b') == SimpleDataObje... field_b='xc')
E         Omitting 1 identical items, use -vv to show
E         Differing attributes:
E         field_b: 'b' != 'xc'

    def test_equal_attrs():
        example1 = SimpleDataObjectAttrs(1, 'b')
        example2 = SimpleDataObjectAttrs(1, 'b')

        assert example1 == example2

        example3 = SimpleDataObjectAttrs(2, 'xc')
>       assert example1 == example3
E       AssertionError: assert SimpleDataObj..., field_b='b') == SimpleDataObje... field_b='xc')
E         Differing attributes:
E         field_a: 1 != 2
E         field_b: 'b' != 'xc'

with -vv:

    def test_equal_data_classes():
        example1 = SimpleDataObject(1, 'b')
        example2 = SimpleDataObject(1, 'b')

        assert example1 == example2

        example3 = SimpleDataObject(1, 'xc')
>       assert example1 == example3
E       AssertionError: assert SimpleDataObj..., field_b='b') == SimpleDataObje... field_b='xc')
E         Matching attributes:
E         ['field_a']
E         Differing attributes:
E         field_b: 'b' != 'xc'


    def test_equal_attrs():
        example1 = SimpleDataObjectAttrs(1, 'b')
        example2 = SimpleDataObjectAttrs(1, 'b')

        assert example1 == example2

        example3 = SimpleDataObjectAttrs(2, 'xc')
>       assert example1 == example3
E       AssertionError: assert SimpleDataObj..., field_b='b') == SimpleDataObje... field_b='xc')
E         Differing attributes:
E         field_a: 1 != 2
E         field_b: 'b' != 'xc'

@alysivji
Copy link
Member Author

@RonnyPfannschmidt I need a hand getting this into a mergeable state

All the changes have been made, but it looks like failure_demo.py is being run under Py27 which does not allow for type annotations as required by the dataclass introspection this PR added.

How can I fix this?

We some examples now use type annotations
@nicoddemus
Copy link
Member

@alysivji thanks for following up with this. I've committed a change so doctesting runs in Python 3, should be fixed now. 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work!

@alysivji alysivji changed the title [WIP] Detailed assert failure introspection for attrs and dataclasses objects Detailed assert failure introspection for attrs and dataclasses objects Nov 19, 2018
@nicoddemus
Copy link
Member

@RonnyPfannschmidt anything else you would like to add here or can we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants