Skip to content
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-108303: Remove the non-test Lib/test/reperf.py #114356

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 20, 2024

Looks like this file was not touched for at least 12 years: https://github.com/python/cpython/commits/main/Lib/test/reperf.py

It does not seem useful. If it is, I can instead move it to Tool/scripts.

Refs #114354

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently test_embed.py runs this code?

@bedevere-app
Copy link

bedevere-app bot commented Jan 20, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sobolevn
Copy link
Member Author

Ok, looks like it was used recently in #110769 by @ambv
I think that this has nothing to do with reperf, it just needs any rather complex module.

I propose to:

  • replace test_embed's code to generate the module from code, so it would have no depedencies
  • still remove this module, we are cleaning things up, aren't we? :)

@sobolevn sobolevn force-pushed the issue-108303-reperf branch from 4752216 to 93da596 Compare February 15, 2024 08:25
@sobolevn
Copy link
Member Author

I am now using another file instead of reperf, it is already used in test_embed and is useful for this test as well: it prints the information, it has some imports, it does some checks.

So, we can remove the unneeded reperf 🎉

@sobolevn sobolevn requested a review from ambv February 15, 2024 08:27
@sobolevn sobolevn requested a review from gpshead February 15, 2024 08:27
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.11 only security fixes labels May 9, 2024
@encukou encukou dismissed gpshead’s stale review September 9, 2024 14:44

test_embed no longer runs the code

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd rather not backport the removal.

@sobolevn
Copy link
Member Author

sobolevn commented Sep 9, 2024

@encukou looks like this PR #113601 influenced the CI. I fixed tests, so now both ./python.exe -m test test_embed and ./python.exe Lib/test/_test_embed_structseq.py pass:

» ./python.exe Lib/test/_test_embed_structseq.py            
..
----------------------------------------------------------------------
Ran 2 tests in 0.000s

OK
Tests passed

Please, take another look :)

@encukou encukou merged commit d7e8339 into python:main Sep 13, 2024
32 checks passed
@encukou
Copy link
Member

encukou commented Sep 13, 2024

OK. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants