gh-108963: using random to generate unique string in sys.intern test by aisk · Pull Request #109491 · python/cpython
using random to generate unique string in sys.intern test
gh-108963: using random to generate unique string in sys.intern test
This change hides the issue that the test actually leaks memory: intern a string which is then only released at Python exit. Can we add a private deintern() function in _testcapi? Or run the test in a subprocess?
Well, i don't know that it's a big deal, this change is ooookay-ish.
| INTERN_NUMRUNS += 1 | ||
| self.assertRaises(TypeError, sys.intern) | ||
| s = "never interned before" + str(INTERN_NUMRUNS) | ||
| s = "never interned before" + str(random.random()) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer using random.randint(), for example 10**9 to have 9 random digits. I prefer to avoid float when possible 😁 Same below.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this increases the probability of random failure from 1/253 (about 1/1016) to 1/(9*10**8). If run the test every second, there will be a failure every 30 years in average.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine, the random.random() returns something like 0.35934878814573135, which have 18 digits (with the first zero). So can we just increase the max range to 10 ** 18 to keep the random resolution almost like as the random.random()?
| def test_intern(self): | ||
| self.assertRaises(TypeError, sys.intern) | ||
| s = "never interned before" + str(random.int(10**8, 10**9)) | ||
| s = "never interned before" + str(random.randint(10**8, 10**9)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intention is to generate random 9-digit numbers, use randrange() instead of randint().
| def test_intern(self): | ||
| self.assertRaises(TypeError, sys.intern) | ||
| s = "never interned before" + str(random.random()) | ||
| s = "never interned before" + str(random.int(10**8, 10**9)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random.int doesn't exist. What's the point of putting a minimum at 10**8? Why not 0?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a more recent commit in the PR fixed the bug. The minimal start is for ensure the random parts have 9 digits, I think it's more consistent.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think that the string length (number of random int digits) matters here, does it?
| INTERN_NUMRUNS += 1 | ||
| self.assertRaises(TypeError, sys.intern) | ||
| s = "never interned before" + str(INTERN_NUMRUNS) | ||
| s = "never interned before" + str(random.randrange(0, 10**18)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, for me 9 digits was already way enough. 18 digits is just too much 😁 You can just pass the max value:
| s = "never interned before" + str(random.randrange(0, 10**18)) | |
| s = "never interned before" + str(random.randrange(10**9)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It makes me sad to leak memory. But I'm not sure about a private API to de-intern a string. I'm not sure of the consequences.
Sorry, @aisk and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 44b1e4ea4842c6cdc1bedba7aaeb93f236b3ec08 3.12
Yhg1s pushed a commit that referenced this pull request
