gh-57879: Increase test coverage for pstats.py

[tigercosmos]

Original issue: #57879
This work is based on the previous work by @furkanonder on #103928
Mainly fixed comments in the previous PR

@corona10 could you help review?

• Issue: Increase test coverage for pstats.py #57879

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

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 skip news label instead.

[corona10]

Maybe @barneygale will be more expert in this area. Would you like to take a look?

[tigercosmos]

@barneygale @corona10 would you like to help review the PR?

[corona10]

I will try to take a look until next week.

[corona10]

@gaogaotiantian Would you like to take a look?

[gaogaotiantian]

Do you have the coverage report before and after? It seems like this tries to test a bunch of non-documented helper functions, which is something I'm a bit hesitated to do. They should be (mostly) covered by the existing test of test_profile (I hope).

[tigercosmos]

Do you have the coverage report before and after? It seems like this tries to test a bunch of non-documented helper functions, which is something I'm a bit hesitated to do. They should be (mostly) covered by the existing test of test_profile (I hope).

true ... seems the coverage is the same

Coverage report (lines and percentages)

With PR:

  318    18%   profile  
  579    44%   pstats   

Without PR:

  318    16%   profile  
  579    43%   pstats  

[tigercosmos]

Oh, the "lines" column is the total "valid lines", which excludes empty lines and comments I believe. The cov% column is the actual coverage percent. But pstats.py is not only covered by test_pstats.py, could you do a before/after on pstats.py with python -m test test_profile test_pstats.py --coverage ?

here is the result... seems no difference ...

After:
318    48%   profile   (/home/mujin/cpython/Lib/profile.py)
579    64%   pstats   (/home/mujin/cpython/Lib/pstats.py)

Before:
318    48%   profile   (/home/mujin/cpython/Lib/profile.py)
579    63%   pstats   (/home/mujin/cpython/Lib/pstats.py)

[gaogaotiantian]

1% is change :)

Like I said, the number on the first column is the total valid line number of the source file - which won't change. The percentage is the covered ratio, and it did change, so this PR increased some coverage, which is good.

However, I don't like the way it directly tries to cover some code by calling internal functions. Let's remove TupleCompTestCase which does not add any coverage at all, and UtilsTestCase which adds some coverage but does it in a non-ideal way. If we want more coverage, we should do it with the public APIs if possible (and it's definitely possible here).

So, remove those two classes, the changes for dump and load can stay as they are public APIs. After that, I think it's ready to go.

BTW, to view the detailed coverage status, use ./python -m test test_profile test_pstats --coverage --coverdir=coverage and all the coverage data of the files will be displayed in your coverage/ dir.

[tigercosmos]

@gaogaotiantian after the change, actually, we just increased the coverage a little bit 😅
load_stats is called during __init__, so actually dump_stats is new in this PR.

coverage change: before/after

197,198c197,198
< >>>>>>         with open(filename, 'wb') as f:
< >>>>>>             marshal.dump(self.stats, f)
---
>     2:         with open(filename, 'wb') as f:
>     1:             marshal.dump(self.stats, f)

154,157c154,157
<    20:         if not self.stats:
< >>>>>>             raise TypeError("Cannot create or construct a %r object from %r"
< >>>>>>                             % (self.__class__, arg))
<    20:         return
---
>    27:         if not self.stats:
>     2:             raise TypeError("Cannot create or construct a %r object from %r"
>     1:                             % (self.__class__, arg))
>    26:         return

[tigercosmos]

@gaogaotiantian tested. should be fine.

[gaogaotiantian]

Still did not fix the file create issue. If you want to create a temporary file without context and delete=True, you have to put a try ... finally block immediately after the file creation. No statement between the try block and the file creation

temp_storage_new = tempfile.NamedTemporaryFile(delete=False)
try:
    self.stats.dump_stats(filename=temp_storage_new.name)
    tmp_stats = pstats.Stats(temp_storage_new.name)
    # some other code
finally:
    temp_storage_new.close()
    os.remove(temp_storage_new.name)

If you do something like this:

temp_storage_new = tempfile.NamedTemporaryFile(delete=False)
self.stats.dump_stats(filename=temp_storage_new.name)
try:
    tmp_stats = pstats.Stats(temp_storage_new.name)
    # some other code
finally:
    temp_storage_new.close()
    os.remove(temp_storage_new.name)

Then if self.stats.dump_stats(filename=temp_storage_new.name) raised an exception, the finally block would not execute, and your temp file won't be cleared properly.

So could you change that in your test? Thanks.

[tigercosmos]

@gaogaotiantian sorry I didn't understand correctly. I fixed the try block now.

[gaogaotiantian]

Hi @corona10 , I think the PR is ready to go. Could you take another look? Thanks!

[gaogaotiantian]

Oh and @tigercosmos you need to sign the CLA. Please check the reply from the bot (it's the first reply in this PR).

[corona10]

Hi @corona10 , I think the PR is ready to go. Could you take another look? Thanks!

I will take a look in 1-2 days.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @corona10 for commit acd9c6a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[corona10]

lgtm

[tigercosmos]

thank you very much @gaogaotiantian @corona10