Expand warnings output for ResourceWarning - #9682
Conversation
|
Here's the output for the test in the suite: |
db8dd8e to
92da79a
Compare
92da79a to
cdd827b
Compare
fschulze
left a comment
There was a problem hiding this comment.
Nice! Thanks for the PR, you beat me to it 😉
| # Use a leading new line to better separate the (large) output | ||
| # from the traceback to the previous warning text. | ||
| msg += ( | ||
| f"\nObject allocated at (most recent call first):\n{formatted_tb}" |
There was a problem hiding this comment.
I'm not sure most recent call first is correct here, maybe just remove it?
| ) | ||
| if warning_message.source is not None: | ||
| try: | ||
| import tracemalloc |
There was a problem hiding this comment.
Are there platforms which don't have tracemalloc available?
There was a problem hiding this comment.
Pypy3 CI failed with ImportError: cannot import _tracemalloc, so I went ahead and assumed that was the case, but didn't look into official sources TBH.
There was a problem hiding this comment.
It is likely that PyPy doesn't support it at all due to differing memory management.
bluetech
left a comment
There was a problem hiding this comment.
Looks great to me! I left one comment.
| except ImportError: | ||
| has_tracemalloc = False | ||
|
|
||
| pytester.makepyfile( |
There was a problem hiding this comment.
I think this test (tracemalloc not enabled) will fail if the person running the pytest test suite enables tracemalloc.
There was a problem hiding this comment.
Hmm why do you say that? And do you mean running in CPython?
I tried it here:
python -X tracemalloc=20 -m pytest testing\test_warnings.py
And it passes.
There was a problem hiding this comment.
Note the try/except is checking if tracemalloc is available at all, which is not on PyPy for example, not if tracemalloc is enabled or not.
There was a problem hiding this comment.
I guess python -X tracemalloc=20 doesn't transfer, but an envvar does, so try this:
PYTHONTRACEMALLOC=100 pytest testing/test_warnings.py -k resource
There was a problem hiding this comment.
Ahh got it, good catch. 😁
Updated the test, thanks.
Fix #9644
cc @fschulze