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.

Sign up for GitHub

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

Jump to bottom

gh-125783: Add tests to prevent regressions with the combination of ctypes and metaclasses. #125881

Merged
merged 5 commits into from
Oct 25, 2024
Merged

gh-125783: Add tests to prevent regressions with the combination of ctypes and metaclasses. #125881

merged 5 commits into from
Oct 25, 2024

Conversation

junkmd
Copy link
Contributor

@junkmd junkmd commented Oct 23, 2024 •
edited by bedevere-app bot
Loading

I would like to backport this to 3.12 and 3.13 as well.

@bedevere-app bedevere-app bot added the tests Tests in the Lib/test dir label Oct 23, 2024
@bedevere-app bedevere-app bot mentioned this pull request Oct 23, 2024
Closed
@bedevere-app
Copy link

bedevere-app bot commented Oct 23, 2024

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.

@ZeroIntensity
Copy link
Member

This is an internal-only change, I don't think it needs a NEWS entry.

junkmd added 2 commits October 24, 2024 12:49
@junkmd
Add a test.
80deddb 
Such an implementation is used in `IUnknown` of `comtypes`.
@junkmd
Add a test.
cf81f03 
Such an implementation is used in `CoClass` of `comtypes`.
@junkmd junkmd force-pushed the test_metacls_PyCSimpleType branch from 1f4eecd to cf81f03 Compare October 24, 2024 12:50
ZeroIntensity
ZeroIntensity reviewed Oct 24, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

I would really rather not use _ in local names here, it makes it unclear that we're testing something public.

@junkmd junkmd mentioned this pull request Oct 24, 2024
Closed
ZeroIntensity
ZeroIntensity approved these changes Oct 24, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Thank you! Take my review as provisional--I am concerned about the use of _pointer_type_cache, as that's private, but it looks like it's not terribly abused in this case (I think?)

@ZeroIntensity ZeroIntensity requested a review from encukou October 24, 2024 13:28
@junkmd
Copy link
Contributor Author

junkmd commented Oct 24, 2024

comtypes has been using _pointer_type_cache since the first commit (an impressive 18 years ago!).
https://github.com/enthought/comtypes/blob/938fbd50d2077c6bd524790a9ed798c3990cdfc0/comtypes/__init__.py#L230-L231

This was also discussed in #125783, and it seems that these implementations were made with careful consideration, as the originator of both comtypes and ctypes is the same person.

encukou
encukou reviewed Oct 25, 2024
View reviewed changes
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.

Thank you for the test! I'd only like to change the names and add some comments.

The main thing we're testing here instantiation of a class in its own __new__, not the early return, that's there to avoid recursion.

Lib/test/test_ctypes/test_c_simple_type_meta.py Outdated Show resolved Hide resolved
Lib/test/test_ctypes/test_c_simple_type_meta.py Outdated Show resolved Hide resolved
@encukou
Copy link
Member

encukou commented Oct 25, 2024

I plan to commit the suggestions and merge next week, if there are no objections.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 25, 2024

Can we apply needs backport to 3.12 and needs backport to 3.13 labels to this PR?

@ZeroIntensity
Copy link
Member

I'm not sure if we backport increased test coverage.

@junkmd junkmd requested a review from encukou October 25, 2024 16:12
@encukou encukou added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 25, 2024
@encukou
Copy link
Member

encukou commented Oct 25, 2024

We generally do, mostly to make it easier to backport later changes that need tests.

@encukou encukou merged commit 1384409 into python:main Oct 25, 2024
35 checks passed
@miss-islington-app
Copy link

Thanks @junkmd for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 25, 2024
@junkmd @miss-islington
pythongh-125783: Add tests to prevent regressions with the combinatio…
51ace8a 
…n of `ctypes` and metaclasses. (pythonGH-125881)

(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 25, 2024
@junkmd @miss-islington
pythongh-125783: Add tests to prevent regressions with the combinatio…
ad5349e 
…n of `ctypes` and metaclasses. (pythonGH-125881)

(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 25, 2024

GH-125987 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 25, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 25, 2024

GH-125988 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 25, 2024
@junkmd junkmd deleted the test_metacls_PyCSimpleType branch October 25, 2024 23:22
This was referenced Oct 26, 2024
Closed
Open
encukou pushed a commit that referenced this pull request Oct 28, 2024
@miss-islington @junkmd
[3.13] gh-125783: Add tests to prevent regressions with the combinati…
86ec8aa 
…on of `ctypes` and metaclasses. (GH-125881) (#125987)

gh-125783: Add tests to prevent regressions with the combination of `ctypes` and metaclasses. (GH-125881)
(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this pull request Oct 29, 2024
@miss-islington @junkmd
@aisk @erlend-aasland
[3.12] gh-125783: Add tests to prevent regressions with the combinati…
bce9df9 
…on of `ctypes` and metaclasses. (GH-125881) (GH-125988)

cherry picked from commit 1384409
by Jun Komoda

Also: Add test_ctypes/_support.py from 3.13+
This partially backports be89ee5
(#113727)
by AN Long


Co-authored-by: Jun Komoda <[email protected]>
Co-authored-by: AN Long <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@junkmd junkmd mentioned this pull request Oct 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

@encukou encukou Awaiting requested review from encukou

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir topic-ctypes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@junkmd @ZeroIntensity @encukou