-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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.
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
gh-126384: Add tests to verify the behavior of basic COM methods. #126610
Conversation
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. |
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.
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__
.
In #126686, 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 |
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.
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.
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.
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 :)
[Edit: I deleted a misguided suggestion] |
!buildbot Windows |
🤖 New build scheduled with the buildbot fleet by @encukou for commit cc07c44 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
I don’t have a strong preference for the order of In my native language, Japanese, the equivalent concept is expressed as “予実” (yojitsu), with the characters for “expected” and “actual” written in that order. |
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.
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?
I added the block. |
Ah thank you. I wasn't sure whether it was needed or not. But one build bot is failing I think? |
I’m not sure which bot you’re referring to, but all the CI checks have passed, and there’s a green ✔ as well. |
!buildbot Windows |
🤖 New build scheduled with the buildbot fleet by @picnixz for commit 6c60512 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
I misunderstood the type of bot running in CI. I’m not sure why the bot is failing. |
!buildbot Windows |
You don't have write permissions to trigger a build |
Sorry, I had to step away yesterday.
No, that's not necessary in new tests. But it doesn't hurt.
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. |
GH-127159 is a backport of this pull request to the 3.13 branch. |
GH-127160 is a backport of this pull request to the 3.12 branch. |
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.