gh-158234: Fix typo in tracemalloc cumulative mode error message - #158233
Tony123-tech wants to merge 1 commit into
Conversation
|
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 |
|
No NEWS entry needed: neither change is user-visible. The |
Tony123-tech
changed the title
|
The typo fix is user-visible only in the sense that the error message |
be75241 to
efe7bd4
Compare
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I'm ok with this change because if we ever change the callers, we might have some weird surprises. So I'm not against this.
At least, the typo fix is real IMO and if others disagree with the defensive change, we can still keep the typo fix.
StanFromIreland
left a comment
StanFromIreland
left a comment
There was a problem hiding this comment.
Since we're fixing the typo, I won't object to the defensive change, but let's not add a test.
Also, I would like to highlight that without the typo, we would not accept the other change.
| def test_compare_to_does_not_mutate_group(self): | ||
| # gh-158232: _compare_grouped_stats() used to pop from old_group, | ||
| # silently draining the caller's dict. | ||
| tb_a = traceback(('f.py', 1)) | ||
| tb_b = traceback(('f.py', 2)) | ||
| tb_c = traceback(('f.py', 3)) | ||
|
|
||
| old = {tb_a: tracemalloc.Statistic(tb_a, 100, 2), | ||
| tb_b: tracemalloc.Statistic(tb_b, 50, 1)} | ||
| new = {tb_a: tracemalloc.Statistic(tb_a, 200, 3), | ||
| tb_c: tracemalloc.Statistic(tb_c, 10, 1)} | ||
| old_copy = dict(old) | ||
|
|
||
| tracemalloc._compare_grouped_stats(old, new) | ||
|
|
||
| self.assertEqual(old, old_copy) | ||
|
|
There was a problem hiding this comment.
| def test_compare_to_does_not_mutate_group(self): | |
| # gh-158232: _compare_grouped_stats() used to pop from old_group, | |
| # silently draining the caller's dict. | |
| tb_a = traceback(('f.py', 1)) | |
| tb_b = traceback(('f.py', 2)) | |
| tb_c = traceback(('f.py', 3)) | |
| old = {tb_a: tracemalloc.Statistic(tb_a, 100, 2), | |
| tb_b: tracemalloc.Statistic(tb_b, 50, 1)} | |
| new = {tb_a: tracemalloc.Statistic(tb_a, 200, 3), | |
| tb_c: tracemalloc.Statistic(tb_c, 10, 1)} | |
| old_copy = dict(old) | |
| tracemalloc._compare_grouped_stats(old, new) | |
| self.assertEqual(old, old_copy) |
I'd prefer we drop the test. The name is wrong, it's placed oddly, and it refers to a different issue. It also tests an implementation detail.
StanFromIreland
changed the title
tracemalloc cumulative mode error message
3 participants
Two small issues in Lib/tracemalloc.py:
_compare_grouped_stats() mutates its old_group argument by popping
keys from it. After the call, old_group is missing every key that
was also in new_group. The current caller (Snapshot.compare_to)
does not reuse the dict, so the bug is latent, but the function
should not modify its arguments. Fix: copy the dict at the top.
A typo in the error message raised by _group_by():
"cumulative mode cannot by used" should be
"cumulative mode cannot be used".
No NEWS entry needed: neither change is user-visible.