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-126384: Add tests to verify the behavior of basic COM methods. #126610

Merged
merged 12 commits into from
Nov 22, 2024
Merged

gh-126384: Add tests to verify the behavior of basic COM methods. #126610

merged 12 commits into from
Nov 22, 2024

Conversation

junkmd
Copy link
Contributor

@junkmd junkmd commented Nov 9, 2024 •
edited
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 9, 2024
@bedevere-app bedevere-app bot mentioned this pull request Nov 9, 2024
Closed
@junkmd junkmd mentioned this pull request Nov 9, 2024
Closed
@junkmd
Copy link
Contributor Author

junkmd commented Nov 10, 2024

I renamed utility functions for testing to conform with PEP8.

Also I added URLs in comments to documentation on COM specifications for developers who may have limited experience with COM and may review or maintain this code in the future.

@junkmd junkmd mentioned this pull request Nov 11, 2024
Merged
encukou
encukou reviewed Nov 18, 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 tests!
I only have a slight idea of how COM works, but, they look good to me!

However, I would like to simplify the supporting Python code, though: we don't need to test CPython internal types, or custom descriptors using __get_name__.

@junkmd junkmd requested a review from encukou November 18, 2024 14:49
@junkmd
Copy link
Contributor Author

junkmd commented Nov 20, 2024

In #126686, COMError became public, but I would like this test to be backported to 3.13 and 3.12, where COMError remains private.

If possible, I would appreciate it if we could merge this as is, and after the backport PR is submitted by the bot, I will prepare a follow-up PR reflecting the fact that COMError is now public.

picnixz
picnixz reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Can we have assertion equality being tested as actual == expected instead of expected == actual? I think we usually do the first form rather than the second one.

Lib/test/test_ctypes/test_win32_com_foreign_func.py Show resolved Hide resolved
encukou
encukou approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thank you!

I don't think it's worth it to reorder assertion arguments, but if you want to do it I won't stop you :)

@encukou
Copy link
Member

encukou commented Nov 21, 2024

[Edit: I deleted a misguided suggestion]

@encukou
Copy link
Member

encukou commented Nov 21, 2024

!buildbot Windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit cc07c44 🤖

The command will test the builders whose names match following regular expression: Windows

The builders matched are:

  • ARM64 Windows Non-Debug PR
  • AMD64 Windows11 Bigmem PR
  • AMD64 Windows10 PR
  • AMD64 Windows11 Non-Debug PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Windows11 Refleaks PR
  • ARM64 Windows PR

@junkmd
Copy link
Contributor Author

junkmd commented Nov 21, 2024

I don’t have a strong preference for the order of actual == expected or expected == actual.
If it’s mergeable as is, please go ahead and merge it.

In my native language, Japanese, the equivalent concept is expressed as “予実” (yojitsu), with the characters for “expected” and “actual” written in that order.
That might be why I wrote it in this order.

picnixz
picnixz approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Let's not burden ourselves with an additional commit. I was just surprised to see it written like that but I won't die from it :) (and since it's a new file anyway, it can have its own style).

By the way, do we need a if __name__ == '__main__' block executing the unittest as is?

@junkmd
Copy link
Contributor Author

junkmd commented Nov 21, 2024

I added the block.

@picnixz
Copy link
Contributor

picnixz commented Nov 21, 2024

Ah thank you. I wasn't sure whether it was needed or not. But one build bot is failing I think?

@junkmd
Copy link
Contributor Author

junkmd commented Nov 21, 2024

I’m not sure which bot you’re referring to, but all the CI checks have passed, and there’s a green ✔ as well.

@picnixz
Copy link
Contributor

picnixz commented Nov 21, 2024

!buildbot Windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 6c60512 🤖

The command will test the builders whose names match following regular expression: Windows

The builders matched are:

  • ARM64 Windows Non-Debug PR
  • AMD64 Windows11 Bigmem PR
  • AMD64 Windows10 PR
  • AMD64 Windows11 Non-Debug PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Windows11 Refleaks PR
  • ARM64 Windows PR

@junkmd
Copy link
Contributor Author

junkmd commented Nov 21, 2024

I misunderstood the type of bot running in CI.
I thought all the bots specified with !buildbot Windows were running in CI.

I’m not sure why the bot is failing.
There are no changes to production code this time.
Looking at the bot’s traceback, it seems that something related to f-strings isn’t working correctly, but the test code I added this time doesn’t use any f-strings.

@junkmd
Copy link
Contributor Author

junkmd commented Nov 22, 2024

!buildbot Windows

@bedevere-bot
Copy link

You don't have write permissions to trigger a build

@encukou
Copy link
Member

encukou commented Nov 22, 2024

Sorry, I had to step away yesterday.

By the way, do we need a if name == 'main' block executing the unittest as is?

No, that's not necessary in new tests. But it doesn't hurt.

I misunderstood the type of bot running in CI.

The Buildbot tests run after merge, as they can be slow or unreliable. And they run on volunteer-maintained hardware; for security we only run them on reviewed code.
I triggered them in order to check the test on several versions of Windows, e.g. 32-bit or ARM.
Sometimes they find issues in uncommon setups. Usually, some fail for unrelated reasons; those can be ignored. (GitHub unfortunately shows a scary-looking red message if any reported tests fail.)

@encukou encukou merged commit 7725c03 into python:main Nov 22, 2024
34 checks passed
@encukou encukou added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Nov 22, 2024
@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.13.
🐍🍒⛏🤖

@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.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 22, 2024
@junkmd @miss-islington
pythongh-126384: Add tests to verify the behavior of basic COM method…
8670935 
…s. (pythonGH-126610)

(cherry picked from commit 7725c03)

Co-authored-by: Jun Komoda <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 22, 2024
@junkmd @miss-islington
pythongh-126384: Add tests to verify the behavior of basic COM method…
9262ef8 
…s. (pythonGH-126610)

(cherry picked from commit 7725c03)

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

bedevere-app bot commented Nov 22, 2024

GH-127159 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 22, 2024
@bedevere-app
Copy link

bedevere-app bot commented Nov 22, 2024

GH-127160 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 22, 2024
@junkmd junkmd deleted the add_com_method_tests branch November 22, 2024 21:27
encukou pushed a commit that referenced this pull request Nov 25, 2024
@miss-islington @junkmd
[3.13] gh-126384: Add tests to verify the behavior of basic COM metho…
f0c47ea 
…ds. (GH-126610) (GH-127159)

(cherry picked from commit 7725c03)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this pull request Nov 25, 2024
@miss-islington @junkmd
[3.12] gh-126384: Add tests to verify the behavior of basic COM metho…
f7e587d 
…ds. (GH-126610) (GH-127160)

(cherry picked from commit 7725c03)

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

@picnixz picnixz picnixz approved these changes

@encukou encukou encukou approved these changes

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.
5 participants
@junkmd @encukou @bedevere-bot @picnixz @ZeroIntensity