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-127183: Add _ctypes.CopyComPointer tests #127184

Merged
merged 5 commits into from
Nov 25, 2024
Merged

gh-127183: Add _ctypes.CopyComPointer tests #127184

merged 5 commits into from
Nov 25, 2024

Conversation

junkmd
Copy link
Contributor

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

I would like to backport this to 3.12 and 3.13 as well.
This is internal-only, so I don’t think it needs a NEWS entry.

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels Nov 23, 2024
@bedevere-app bedevere-app bot mentioned this pull request Nov 23, 2024
Closed
encukou
encukou reviewed Nov 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.
I see CopyComPointer is undocumented. Would you be willing to add some documentation for it as well?

Lib/test/test_ctypes/test_win32_com_foreign_func.py Outdated
dst_orig = create_shelllink_persist(self.IPersist)
dst = self.IPersist()
CopyComPointer(dst_orig, byref(dst))
dst_orig.Release() # The refcount of `dst_orig` is 1 here.
Copy link
Member

Choose a reason for hiding this comment

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

Could the refcount be checked using Release's return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, since this assertion is not costly, it is better to simply call assertEqual than to leave a comment about it.
I have changed these lines.

@junkmd
Copy link
Contributor Author

junkmd commented Nov 25, 2024

Would you be willing to add some documentation for it as well?

That's my intention, but as discussed in gh-126686, I believe publicizing it is also necessary for proper documentation.

Before moving forward with documentation or publicizing, I want to backport this test to 3.13 and 3.12, where CopyComPointer, albeit private, already exists.
Therefore, I aim to keep this PR in a state where it can be merged as-is when miss-islington backports it.

@encukou encukou added skip news needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Nov 25, 2024
@encukou encukou merged commit c7f1e3e into python:main Nov 25, 2024
41 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 Nov 25, 2024
@junkmd @miss-islington
pythongh-127183: Add _ctypes.CopyComPointer tests (pythonGH-127184)
8ff7119 
* Make `create_shelllink_persist` top level function.

* Add `CopyComPointerTests`.

* Add more tests.

* Update tests.

* Add assertions for `Release`'s return value.
(cherry picked from commit c7f1e3e)

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

bedevere-app bot commented Nov 25, 2024

GH-127251 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 Nov 25, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2024
@junkmd @miss-islington
pythongh-127183: Add _ctypes.CopyComPointer tests (pythonGH-127184)
c334007 
* Make `create_shelllink_persist` top level function.

* Add `CopyComPointerTests`.

* Add more tests.

* Update tests.

* Add assertions for `Release`'s return value.
(cherry picked from commit c7f1e3e)

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

bedevere-app bot commented Nov 25, 2024

GH-127252 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 Nov 25, 2024
@encukou
Copy link
Member

encukou commented Nov 25, 2024

Makes sense. Thank you for the test!

@junkmd junkmd deleted the add_copy_com_pointer_tests branch November 25, 2024 14:10
This was referenced Nov 25, 2024
Open
Open
encukou pushed a commit that referenced this pull request Nov 26, 2024
@miss-islington @junkmd
[3.13] gh-127183: Add _ctypes.CopyComPointer tests (GH-127184) (GH-…
0f77357 
…127251)

gh-127183: Add `_ctypes.CopyComPointer` tests (GH-127184)

* Make `create_shelllink_persist` top level function.

* Add `CopyComPointerTests`.

* Add more tests.

* Update tests.

* Add assertions for `Release`'s return value.
(cherry picked from commit c7f1e3e)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this pull request Nov 26, 2024
@miss-islington @junkmd
[3.12] gh-127183: Add _ctypes.CopyComPointer tests (GH-127184) (GH-…
08531ac 
…127252)

gh-127183: Add `_ctypes.CopyComPointer` tests (GH-127184)

* Make `create_shelllink_persist` top level function.

* Add `CopyComPointerTests`.

* Add more tests.

* Update tests.

* Add assertions for `Release`'s return value.
(cherry picked from commit c7f1e3e)

Co-authored-by: Jun Komoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou left review comments

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

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