Skip to content

Conversation

@Alexander3
Copy link

Some libs like graphene in my case return OrderedDicts istead of normal dicts.

I have assert like this:

    assert result.data == {
        'campaign': {
            'categories': [
                {'name': 'suggested', 'description': 'represent'},
                {'name': 'test category', 'description': 'simply'},
            ]
        }
    }

And it's hard to spot the difference in test results
Zrzut ekranu z 2019-11-25 10-26-19
Pycharm has even "See difference option":
Zrzut ekranu z 2019-11-25 10-26-44

I will write some tests when I'll have more time, for now please tell me what you think about this idea.

@adamchainz
Copy link
Collaborator

I'm not sure this is the responsibility of this library. pytest-icdiff focusses on giving you a better diff experience. It shouldn't be modifying the data.

If want to compare output OrderedDicts to dicts, I think you're better using a helper in your tests so you can write assert normal_dicts(result.data) == { ....

@Alexander3
Copy link
Author

Yes, but using helper in over 1000 tests makes them less readable, and also slower, because it would convert OrderedDicts even when they are equal. I tried to override pytest_assertrepr_compare in my conftest.py (as I understand it's fired only when assert fails), but it's not working because of pytest-icdiff. Looks like hooks added by libs take precedence. It's weird that both hooks got executed, but only output from pytest-icdiff was used, but anyway I think that if equality operator treats dicts and OrderedDicts the same, we can convert them here to give better experience to user. Since they are equal anyway I don't think of it like modification of data.

@hjwp
Copy link
Owner

hjwp commented Nov 26, 2019

this sounds interesting. i'd be pretty happy to just yolo-convert the ordereddict to normaldicts and then show the diff? maybe add a message to say that's what we did...

@hjwp hjwp force-pushed the master branch 3 times, most recently from cbacb25 to 621b15e Compare August 9, 2022 20:27
@hjwp hjwp changed the base branch from master to main December 5, 2023 11:17
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.

3 participants