Skip to content

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

Merged
encukou merged 5 commits into
python:mainfrom
junkmd:test_metacls_PyCSimpleType
Oct 25, 2024
Merged

gh-125783: Add tests to prevent regressions with the combination of ctypes and metaclasses.#125881
encukou merged 5 commits into
python:mainfrom
junkmd:test_metacls_PyCSimpleType

Conversation

@junkmd

@junkmdjunkmd commented Oct 23, 2024•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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

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

bedevere-appBot commented Oct 23, 2024

Copy link
Copy Markdown

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

ZeroIntensity commented Oct 24, 2024

Copy link
Copy Markdown
Member

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

@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
junkmdforce-pushed the test_metacls_PyCSimpleType branch from 1f4eecd to cf81f03CompareOctober 24, 2024 12:50
ZeroIntensity
ZeroIntensity reviewed Oct 24, 2024
View reviewed changes

@ZeroIntensityZeroIntensity 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.

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

@junkmdjunkmd mentioned this pull request Oct 24, 2024
Closed
ZeroIntensity
ZeroIntensity approved these changes Oct 24, 2024
View reviewed changes

@ZeroIntensityZeroIntensity 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.

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

@junkmd

junkmd commented Oct 24, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@encukouencukou 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.

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.

Comment threadLib/test/test_ctypes/test_c_simple_type_meta.py Outdated
Comment threadLib/test/test_ctypes/test_c_simple_type_meta.py Outdated
@encukou

encukou commented Oct 25, 2024

Copy link
Copy Markdown
Member

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

junkmdand others added 2 commits October 26, 2024 00:23
@junkmd@encukou
Apply suggestions from code review
47ec482
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@junkmd
Update comments.
4f1b74c
@junkmd

junkmd commented Oct 25, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@ZeroIntensity

ZeroIntensity commented Oct 25, 2024

Copy link
Copy Markdown
Member

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

@junkmd
junkmd requested a review from encukouOctober 25, 2024 16:12
@encukouencukou added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 25, 2024
@encukou

encukou commented Oct 25, 2024

Copy link
Copy Markdown
Member

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

@encukou
encukou merged commit 1384409 into python:mainOct 25, 2024
@miss-islington-app

miss-islington-appBot commented Oct 25, 2024

Copy link
Copy Markdown

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 <45822440+junkmd@users.noreply.github.com>
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 <45822440+junkmd@users.noreply.github.com>
@bedevere-app

bedevere-appBot commented Oct 25, 2024

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Oct 25, 2024
@bedevere-app

bedevere-appBot commented Oct 25, 2024

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot removed the needs backport to 3.12 only 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 <45822440+junkmd@users.noreply.github.com>
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 <45822440+junkmd@users.noreply.github.com>
Co-authored-by: AN Long <aisk@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@junkmdjunkmd mentioned this pull request Oct 29, 2024
Merged
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@junkmd@ebonnal
pythongh-125783: Add tests to prevent regressions with the combinatio…
a761341
…n of `ctypes` and metaclasses. (pythonGH-125881)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensityZeroIntensityZeroIntensity approved these changes
@encukouencukouAwaiting requested review from encukou

Assignees

No one assigned

Labels

skip newstestsTests in the Lib/test dirtopic-ctypes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@junkmd@ZeroIntensity@encukou