GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-108303: Remove the non-test `Lib/test/reperf.py` by sobolevn · Pull Request #114356 · python/cpython

Conversation

@sobolevn

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

Uh oh!

There was an error while loading. Please reload this page.

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

  • Issue: Move test files into test subdirectories #108303

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn
pythongh-108303: Remove the non-test Lib/test/reperf.py
15c0618
@sobolevn
sobolevn requested a review from gpshead January 20, 2024 21:07
@bedevere-app bedevere-app Bot added the awaiting review label Jan 20, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 20, 2024
Move test files into test subdirectories #108303
Closed
@sobolevn sobolevn added skip news needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 20, 2024
gpshead
gpshead approved these changes Jan 20, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 20, 2024
gpshead
gpshead previously requested changes Jan 20, 2024
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

apparently test_embed.py runs this code?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting merge labels Jan 20, 2024
@bedevere-app

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Jan 21, 2024

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? :)
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits February 15, 2024 11:23
@sobolevn
Merge branch 'main' into issue-108303-reperf
d4387f1
@sobolevn
Use another file instead of reperf
93da596
@sobolevn
sobolevn force-pushed the issue-108303-reperf branch from 4752216 to 93da596 Compare February 15, 2024 08:25
@sobolevn

sobolevn commented Feb 15, 2024

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 🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn assigned ambv Feb 15, 2024
@sobolevn
sobolevn requested a review from ambv February 15, 2024 08:27
@sobolevn sobolevn unassigned ambv Feb 15, 2024
@sobolevn
sobolevn requested a review from gpshead February 15, 2024 08:27
@sobolevn
Merge branch 'main' into issue-108303-reperf
8c8b9c1
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 only 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

encukou
encukou approved these changes Sep 9, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Sep 9, 2024
@encukou encukou added awaiting changes and removed awaiting merge needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Sep 9, 2024
@sobolevn sobolevn removed the awaiting changes label Sep 9, 2024
sobolevn added 2 commits September 9, 2024 19:55
@sobolevn
Merge branch 'main' into issue-108303-reperf
b1cc660
@sobolevn
Fix tests
6a14de8
@sobolevn

sobolevn commented Sep 9, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou
encukou reviewed Sep 10, 2024
View reviewed changes
Comment thread Lib/test/_test_embed_structseq.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@encukou
encukou merged commit d7e8339 into python:main Sep 13, 2024
@encukou

encukou commented Sep 13, 2024

Copy link
Copy Markdown
Member

OK. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@encukou encukou encukou approved these changes
@ambv ambv Awaiting requested review from ambv
@gpshead gpshead Awaiting requested review from gpshead

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@sobolevn @encukou @gpshead @ambv @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.