bpo-45679: add `tuple` tests with `lru_cache` to `test_functools` by sobolevn · Pull Request #29339 · python/cpython

sobolevn · 2021-10-31T08:00:29Z

I've added several extra tests to make sure case in 45679 works as intended.

https://bugs.python.org/issue45701

Context: https://bugs.python.org/issue45679

sobolevn · 2021-11-03T11:08:12Z

rhettinger · 2021-11-04T00:01:49Z

I appreciate the effort to add tests but am dubious about these.

On a cold reading, I find it difficult to tell what guaranteed invariants are being checked or even what the test is doing. If one of these tests ever broke, it would take a while to figure out what the complaint was about.

Also, the assertions about the cache_info components seem to be over-specified. We make almost no promises about the cache hits and have more than once changed what counts as a hit.

My recommendation is to just add a couple of lines to test_lru_with_types():

# Verify distinct results for equivalent tuples with differing types
cached_repr = self.module.lru_cache(maxsize=maxsize, typed=True)(repr)
self.assertEqual(cached_repr((True, 'string')), "(True, 'string')")
self.assertEqual(cached_repr((1, 'string')), "(1, 'string')")

I don't think there is a need for a new test method that makes heroic efforts to test in combination that which has already been tested piece by piece. A single additional test will suffice to cover the notion, "that which works for scalars also works in containers".

rhettinger · 2021-11-04T00:16:00Z

Lib/test/test_functools.py

+                    self.assertEqual(cached.cache_info().misses, 2)
+
+    def test_lru_with_container_types_hash_collision(self):
+        # https://bugs.python.org/issue45701


We would normally only reference a bug if it pertained to a bug in the lru_cache itself. In this case, the referenced bug was in application code. Linking to it from this from here doesn't add any explanatory value.

sobolevn · 2021-11-04T07:30:47Z

@rhettinger thanks a lot for the detailed feedback!

My recommendation is to just add a couple of lines to test_lru_with_types():

# Verify distinct results for equivalent tuples with differing types
cached_repr = self.module.lru_cache(maxsize=maxsize, typed=True)(repr)
self.assertEqual(cached_repr((True, 'string')), "(True, 'string')")
self.assertEqual(cached_repr((1, 'string')), "(1, 'string')")

The thing is, it does not not work this way. And that's what we test here 🙂

>>> from functools import lru_cache
>>> r = lru_cache(typed=True)(repr)
>>> r((1, 2))
'(1, 2)'
>>> r((True, 2))
'(1, 2)'
>>> r((1.0, 2))
'(1, 2)'

Also, the assertions about the cache_info components seem to be over-specified. We make almost no promises about the cache hits and have more than once changed what counts as a hit.

I took the idea from the similar tests in this module. There're around 50 usages of .cache_info() and most of them assume hits / misses:

» ag '.cache_info()' Lib/test/test_functools.py 
1217:        self.assertEqual(fib.cache_info(),
1220:        self.assertEqual(fib.cache_info(),
1230:        hits, misses, maxsize, currsize = f.cache_info()
1242:        hits, misses, maxsize, currsize = f.cache_info()
1248:        hits, misses, maxsize, currsize = f.cache_info()
1253:        hits, misses, maxsize, currsize = f.cache_info()
1261:        hits, misses, maxsize, currsize = f.cache_info()
1272:        self.assertEqual(f.cache_info().maxsize, 0)
1277:        hits, misses, maxsize, currsize = f.cache_info()
1288:        self.assertEqual(f.cache_info().maxsize, 1)
1293:        hits, misses, maxsize, currsize = f.cache_info()
1304:        self.assertEqual(f.cache_info().maxsize, 2)
1310:        hits, misses, maxsize, currsize = f.cache_info()
1322:        self.assertEqual(square.cache_info().hits, 1)
1323:        self.assertEqual(square.cache_info().misses, 2)
1324:        self.assertEqual(square.cache_info().maxsize, 128)
1325:        self.assertEqual(square.cache_info().currsize, 2)
1348:        self.assertEqual(f.cache_info().currsize, 10)
1352:        self.assertEqual(f.cache_info().currsize, 10)
1366:        self.assertEqual(f.cache_info().hits, 1)
1386:        self.assertEqual(f.cache_info(), (0, 1, 1, 1))
1391:        self.assertEqual(f.cache_info(), (1, 1, 1, 1))
1396:        self.assertEqual(f.cache_info(), (1, 2, 1, 1))
1401:        self.assertEqual(f.cache_info(), (1, 3, 1, 1))
1451:        self.assertEqual(fib.cache_info(),
1454:        self.assertEqual(fib.cache_info(),
1463:        self.assertEqual(eq.cache_info(),
1495:            self.assertEqual(square.cache_info().hits, 4)
1496:            self.assertEqual(square.cache_info().misses, 4)
1516:                    self.assertEqual(cached.cache_info().hits, 2)
1517:                    self.assertEqual(cached.cache_info().misses, 2)
1548:                    self.assertEqual(cached.cache_info().hits, 3)
1549:                    self.assertEqual(cached.cache_info().misses, 1)
1561:        self.assertEqual(fib.cache_info(),
1564:        self.assertEqual(fib.cache_info(),
1575:        self.assertEqual(fib.cache_info(),
1578:        self.assertEqual(fib.cache_info(),
1588:        self.assertEqual(f.cache_info(),
1604:        hits, misses, maxsize, currsize = f.cache_info()
1627:            hits, misses, maxsize, currsize = f.cache_info()
1657:        self.assertEqual(f.cache_info(), (0, 0, m*n, 0))
1672:                self.assertEqual(f.cache_info(), (0, (i+1)*n, m*n, i+1))
1721:        self.assertEqual(X.f.cache_info(), (0, 0, 2, 0))
1726:        self.assertEqual(X.f.cache_info(), (4, 6, 2, 2))
1731:        self.assertEqual(X.f.cache_info(), (10, 10, 2, 2))
1736:        self.assertEqual(X.f.cache_info(), (15, 15, 2, 2))
1738:        self.assertEqual(a.f.cache_info(), X.f.cache_info())
1739:        self.assertEqual(b.f.cache_info(), X.f.cache_info())
1740:        self.assertEqual(c.f.cache_info(), X.f.cache_info())

I don't think there is a need for a new test method that makes heroic efforts to test in combination that which has already been tested piece by piece

I am not sure I follow 🙂
There's no test at the moment which test this corner case.

All this being said, I can easily change this to be a simple one / two lines test. But, I'll add some more explanation context to tests' docstrings first. Maybe it will be easier to read / maintain. And possibly we can keep more complex checks.

sobolevn · 2021-11-04T07:57:16Z

But, I'll add some more explanation context to tests' docstrings first.

Done! I've simplified tests as much as possible. Including:

I've removed maxsize checks. They are not important in this context
I've removed the first test, because it is not special. It just works
I've changed the second test to use repr as @rhettinger suggested. Now it looks more readable: it clearly indicates what we expect
I've also added some docstring explanation for the future readers

I hope it is good enough 🙂
If not, I would be happy to fix it!

rhettinger · 2021-11-04T14:57:40Z

The thing is, it does not not work this way.

Yes, now I see. The tested invariant is that typed applies to the container rather than its contents.

Would it suffice to add a single test case, test_typed_is_not_recursive():

>>> @lru_cache(typed=True)
... def myrepr(obj):
...     return repr(obj)
...
>>> myrepr(1)
'1'
>>> myrepr(True)
'True'
>>> myrepr((True, 'x'))
"(True, 'x')"
>>> myrepr((1, 'x'))
"(True, 'x')"
>>> class T(tuple):
...     pass
... 
>>> myrepr(T((1, 'x')))
"(1, 'x')"

rhettinger · 2021-11-05T19:53:09Z

Thank you for the PR.

sobolevn · 2021-11-05T20:07:30Z

Thanks a lot for the great simplification 👍

…thonGH-29339)

sobolevn requested a review from rhettinger as a code owner October 31, 2021 08:00

the-knights-who-say-ni added the CLA signed label Oct 31, 2021

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Oct 31, 2021

bpo-45701: add tuple tests with lru_cache to test_functools

9d8a4fd

sobolevn force-pushed the issue-45679-2 branch from cba8d02 to 9d8a4fd Compare November 3, 2021 11:06

rhettinger self-assigned this Nov 4, 2021

rhettinger reviewed Nov 4, 2021

View reviewed changes

Simplifies tests a bit

ea0968f

sobolevn added 2 commits November 5, 2021 16:32

Simplify test even more!

f289881

Add custom tuple subtype

4fea891

rhettinger approved these changes Nov 5, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 5, 2021

rhettinger merged commit 60b5333 into python:main Nov 5, 2021

bedevere-bot removed the awaiting merge label Nov 5, 2021

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021

bpo-45679: add tuple tests with lru_cache to test_functools (py…

cfab56c

…thonGH-29339)

remykarem pushed a commit to remykarem/cpython that referenced this pull request Jan 30, 2022

bpo-45679: add tuple tests with lru_cache to test_functools (py…

fe44a08

…thonGH-29339)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bpo-45679: add `tuple` tests with `lru_cache` to `test_functools`#29339

bpo-45679: add `tuple` tests with `lru_cache` to `test_functools`#29339
rhettinger merged 4 commits intopython:mainfrom
sobolevn:issue-45679-2

sobolevn commented Oct 31, 2021 •

edited

Loading

Uh oh!

sobolevn commented Nov 3, 2021

Uh oh!

rhettinger commented Nov 4, 2021 •

edited

Loading

Uh oh!

rhettinger Nov 4, 2021

Uh oh!

sobolevn commented Nov 4, 2021 •

edited

Loading

Uh oh!

sobolevn commented Nov 4, 2021

Uh oh!

rhettinger commented Nov 4, 2021

Uh oh!

rhettinger commented Nov 5, 2021

Uh oh!

sobolevn commented Nov 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

sobolevn commented Oct 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Nov 3, 2021

Uh oh!

rhettinger commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhettinger Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Nov 4, 2021

Uh oh!

rhettinger commented Nov 4, 2021

Uh oh!

rhettinger commented Nov 5, 2021

Uh oh!

sobolevn commented Nov 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sobolevn commented Oct 31, 2021 •

edited

Loading

rhettinger commented Nov 4, 2021 •

edited

Loading

sobolevn commented Nov 4, 2021 •

edited

Loading