FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-108303: Remove the non-test `Lib/test/reperf.py` by sobolevn · Pull Request #114356 · python/cpython · GitHub

/ cpython Public

gh-108303: Remove the non-test Lib/test/reperf.py - #114356

Merged
encukou merged 6 commits into
python:mainfrom
sobolevn:issue-108303-reperf
Sep 13, 2024
Merged

gh-108303: Remove the non-test Lib/test/reperf.py#114356
encukou merged 6 commits into
python:mainfrom
sobolevn:issue-108303-reperf

Conversation

sobolevn commented Jan 20, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

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

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

apparently test_embed.py runs this code?

bedevere-app Bot commented Jan 20, 2024

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
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 force-pushed the issue-108303-reperf branch from 4752216 to 93da596 Compare February 15, 2024 08:25

Copy link
Copy Markdown
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 requested a review from ambv February 15, 2024 08:27
sobolevn requested a review from gpshead February 15, 2024 08:27
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 dismissed gpshead’s stale review September 9, 2024 14:44

test_embed no longer runs the code

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

sobolevn commented Sep 9, 2024
edited
Loading

Copy link
Copy Markdown
Member Author

@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 merged commit d7e8339 into python:main Sep 13, 2024

encukou commented Sep 13, 2024

Copy link
Copy Markdown
Member

OK. Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL