-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-57879: Increase test coverage for pstats.py #103928
Conversation
@@ -30,12 +33,35 @@ class StatsTestCase(unittest.TestCase): | |||
def setUp(self): | |||
stats_file = support.findfile('pstats.pck') | |||
self.stats = pstats.Stats(stats_file) | |||
to_compile = 'import os' | |||
self.temp_storage = tempfile.mktemp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't tempfile.mktemp
deprecated? https://docs.python.org/3/library/tempfile.html#tempfile.mktemp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
def test_load_equivalent_to_init(self): | ||
empty = pstats.Stats() | ||
empty.load_stats(self.temp_storage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like your new tests both dump to and load from this temp file, functionality wise it works for now, but I think we should "isolate" the tests as much as possible? Reordering the execution of the tests could cause this load_stats
to load different files - it could be the file created at setUp
, or it could be the file created in test_dump_and_load_works_correctly
. Again, it works because the test does not care what's in the file, as long as it's valid, it's just a bit convoluted. It seems like the profile created in setUp
is only (potentially) used in this test? Maybe consider to move that piece into the unittest directly? The other test will overwrite thie file anyway I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored by: Andrea Crotti