Skip to content

Conversation

merlinz01
Copy link

@merlinz01 merlinz01 commented Nov 17, 2024

The function previously used a simple difflib.ndiff on top of a pprint.pformat of each dict, which resulted in very bad performance on large dicts and unclear assertion error outputs in many cases. This change formats the diffs in a more readable manner by inspecting the differences between the dicts, truncating long keys and values, and justifying values in the various groups of lines.

…DictEqual

The function previously used a simple difflib.ndiff on top of a pprint.pformat
of each dict, which resulted in very bad performance on large dicts and unclear
assertion error outputs in many cases. This change formats the diffs in a more
readable manner by inspecting the differences between the dicts, truncating
long keys and values, and justifying values in the various groups of lines.
@ghost
Copy link

ghost commented Nov 17, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Nov 17, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@merlinz01 merlinz01 changed the title gh-23474: Improve performance and error readability of unittest.TestCase.assertDictEqual gh-27434: Improve performance and error readability of unittest.TestCase.assertDictEqual Nov 17, 2024
@bedevere-app
Copy link

bedevere-app bot commented Nov 17, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@merlinz01 merlinz01 changed the title gh-27434: Improve performance and error readability of unittest.TestCase.assertDictEqual gh-99151: Improve performance and error readability of unittest.TestCase.assertDictEqual Nov 17, 2024
Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

Can you please provide benchmarks testing the old and new approaches?

Co-authored-by: Kirill Podoprigora <[email protected]>
@merlinz01
Copy link
Author

Comparing two dicts with 10000 keys and values of 20 random bytes each.

class TestDictCompare(unittest.TestCase):
    def test_compare_dicts(self):
        first = self.generate_dict()
        second = self.generate_dict()
        self.assertDictEqual(first, second)

    def generate_dict(self):
        length = 10000
        d = {}
        for _ in range(length):
            d[random.randbytes(20)] = random.randbytes(20)
        return d

The previous implementation takes 15.3 seconds:

F
======================================================================
FAIL: test_compare_dicts (__main__.TestDictCompare.test_compare_dicts)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/merlin/Projects/cpython/test_new_format.py", line 36, in test_compare_dicts
    self.assertDictEqual(
    ~~~~~~~~~~~~~~~~~~~~^
        first, second
        ^^^^^^^^^^^^^
    )
    ^
AssertionError: {b'\xbeA\r\xf3~L?\xa5\xf4\x97"\x94\x1f\x98T\xe[1244908 chars]xf0'} != {b'\x01\xfaz\x02r\xec\xe1\xdf\xba8\r\xdc\xd9\x[1247916 chars]x04'}
Diff is 12985674 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 1 test in 15.300s

FAILED (failures=1)

The new implementation takes 0.28 seconds:

F
======================================================================
FAIL: test_compare_dicts (__main__.TestDictCompare.test_compare_dicts)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/merlin/Projects/cpython/test_new_format.py", line 36, in test_compare_dicts
    self.assertDictEqual(
    ~~~~~~~~~~~~~~~~~~~~^
        first, second
        ^^^^^^^^^^^^^
    )
    ^
AssertionError: {b'_\x94|^\xb9e\x98\x86\xdeUd\xf3\x7f\xc5\x9[1246065 chars]x80'} != {b'\x16_W\xea}\xf1 \xaa`=\x06\xf2\xf4#\xe5\x[1247665 chars]xe7'}
Diff is 2197425 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 1 test in 0.277s

FAILED (failures=1)

@merlinz01
Copy link
Author

For the test in the linked issue, I'm getting 6.6 seconds with the old vs. 0.22 seconds with the new.

@merlinz01
Copy link
Author

merlinz01 commented Nov 18, 2024

An example of the new output:

{
    'eighteighteight': 'eight',
    'four'           : 4,
Keys in both dicts with differing values:
  - 'oneoneoneone': 1,
  + 'oneoneoneone': 2,
  - 'six': 6,
  + 'six': 'six',
Keys in the first dict but not the second:
  - 'seven': 'averyveryveryveryveryveryveryveryv[44 chars]alue',
  - 'two'  : 2,
Keys in the second dict but not the first:
  + 'averyveryveryveryveryveryveryveryl[22 chars]gkey': {'oneoneoneone': 1, 'two': 2, 'four[133 chars]ght'},
  + 'five'                                            : 5,
  + 'three'                                           : 3,
} : Hey, you passed dicts that were not equal!

(edited to reflect changes in fcfdd92)

@tomasr8
Copy link
Member

tomasr8 commented Nov 18, 2024

Thanks! That's a pretty nice speedup! I'm just wondering, do we want to show the Keys in both dicts with identical values? If they're identical, maybe we can just omit them form the diff output?

@merlinz01
Copy link
Author

I'll remove the explanation line for identical keys and values.

The old implementation doesn't omit anything, so if we omit values I think there should be some indication that there are more values than what is shown, rather than silently dropping them.
E.g.

{
(omitted 590 matching key-value pairs)
Keys in both dicts with differing values:
  ...

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Now that the diffing is more complex, I think it needs more tests to cover the possible outputs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants