Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Merged
rhettinger merged 4 commits into
python:mainpython/cpython:mainfrom
sobolevn:issue-45679-2sobolevn/cpython:issue-45679-2Copy head branch name to clipboard
Nov 5, 2021
Merged

bpo-45679: add tuple tests with lru_cache to test_functools#29339
rhettinger merged 4 commits into
python:mainpython/cpython:mainfrom
sobolevn:issue-45679-2sobolevn/cpython:issue-45679-2Copy head branch name to clipboard

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 sobolevn requested a review from rhettinger as a code owner October 31, 2021 08:00
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Oct 31, 2021
@sobolevn

sobolevn commented Nov 3, 2021

Copy link
Copy Markdown
Member Author

CC @Fidget-Spinner

@rhettinger

rhettinger commented Nov 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 rhettinger self-assigned this Nov 4, 2021
rhettinger
rhettinger reviewed Nov 4, 2021
View reviewed changes
Comment thread Lib/test/test_functools.py Outdated
self.assertEqual(cached.cache_info().misses, 2)

def test_lru_with_container_types_hash_collision(self):
# https://bugs.python.org/issue45701

@rhettinger rhettinger Nov 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

sobolevn commented Nov 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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

sobolevn commented Nov 4, 2021

Copy link
Copy Markdown
Member Author

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

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

  1. I've removed maxsize checks. They are not important in this context
  2. I've removed the first test, because it is not special. It just works
  3. I've changed the second test to use repr as @rhettinger suggested. Now it looks more readable: it clearly indicates what we expect
  4. 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

rhettinger commented Nov 4, 2021

Copy link
Copy Markdown
Contributor

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 rhettinger merged commit 60b5333 into python:main Nov 5, 2021
@rhettinger

rhettinger commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

Thank you for the PR.

@sobolevn

sobolevn commented Nov 5, 2021

Copy link
Copy Markdown
Member Author

Thanks a lot for the great simplification 👍

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
@sobolevn @remykarem
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
@sobolevn @remykarem
bpo-45679: add tuple tests with lru_cache to test_functools (py…
fe44a08 
…thonGH-29339)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes

Assignees

@rhettinger rhettinger

Labels

tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sobolevn @rhettinger @the-knights-who-say-ni @bedevere-bot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.