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.

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

[test\test_basic.py] Use OleDLL instead of windll #764

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moi15moi
Copy link
Contributor

See #735

IUnknown to ICreateTypeLib2 because CreateTypeLib2 doesn't take IUnknown as a parameter.

@moi15moi moi15moi force-pushed the Use-WinDLL/OleDLL-for-test_basic branch from c8d1c7c to a9f0558 Compare January 25, 2025 20:06
@moi15moi moi15moi changed the title [test\test_basic.py] Use WinDLL instead of windll [test\test_basic.py] Use OleDLL instead of windll Jan 25, 2025
Copy link
Collaborator

@junkmd junkmd left a comment

Choose a reason for hiding this comment

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

Here’s my perspective on this test:

Given the objectives of test_refcounts and test_qi, any test double that serves as a pointer to IUnknown should suffice. However, I think it's better to avoid approaches that rely on specific custom interface definitions.
Since test_identity calls CreateTypeLib (that indirectly calls _CreateTypeLib2), I don’t see much value in testing _CreateTypeLib2 bound to ICreateTypeLib2 specifically in this context.

def test_identity(self):
# COM indentity rules
# these should be identical
a = POINTER(IUnknown)()
b = POINTER(IUnknown)()
self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
from comtypes.typeinfo import CreateTypeLib
# we do not save the lib, so no file will be created.
# these should NOT be identical
a = CreateTypeLib("blahblah")
b = CreateTypeLib("spam")

It might be worth considering defining a single-responsibility function to create this test double, duplicating _CreateTypeLib2 from typeinfo.py and make the “_CreateTypeLib2 that accepts POINTER(IUnknown)”.

What are your thoughts on this?

@moi15moi
Copy link
Contributor Author

moi15moi commented Jan 26, 2025

I prefer to adhere strictly to the actual parameters of CreateTypeLib2 to maintain accuracy and consistency.

Since IUnknown behaves like an abstract class, there is no direct way to test refcounts and QueryInterface and I think it's totally normal. However, I believe it would be beneficial to include a comment indicating that the purpose is to test the behavior of IUnknown even if we use ICreateTypeLib2.

What do you think?

@junkmd
Copy link
Collaborator

junkmd commented Jan 26, 2025

However, I believe it would be beneficial to include a comment indicating that the purpose is to test the behavior of IUnknown even if we use ICreateTypeLib2.

Indeed, comments can be a powerful tool in situations like this.

Considering a scenario where future maintainers might need to modify these tests, it’s important to document our thought process here.

Please make a commit to add comments.

IUnknown to ICreateTypeLib2 because CreateTypeLib2 doesn't take IUnknown as a parameter
@moi15moi moi15moi force-pushed the Use-WinDLL/OleDLL-for-test_basic branch from a9f0558 to fcd7e40 Compare January 26, 2025 03:17
@moi15moi
Copy link
Contributor Author

Please make a commit to add comments.

Done, let me know what you think.

@moi15moi moi15moi requested a review from junkmd January 26, 2025 03:19
@@ -20,17 +21,21 @@ def test_release(self):
POINTER(IUnknown)()

def test_refcounts(self):
p = POINTER(IUnknown)()
windll.oleaut32.CreateTypeLib2(1, "blabla", byref(p))
# Since IUnknown behaves like an abstract class, the reference counting behavior cannot be tested directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the previous code demonstrates, QueryInterface and refcount tests can be performed with this package’s IUnknown.

To better reflect the accurate situation, I think a comments like “COM interfaces can use AddRef, Release, and QueryInterface inherited from IUnknown” would be more appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants