Implement record pointers as method parameters of a Dispatch Interface

[geppi]

to get server side modifications of a record marshaled back to the client.

With a COM Dual Interface it is possible to get server side changes to record members marshaled back to the client.
This requires that the type library specifies a record pointer as the interface method parameter.
The comtypes implementation for Dual Interfaces does properly take the type library specification into account and passes the record by reference in that case.

However, for a pure IDispatch interface only passing a record by value in form of a copy is implemented.

This pull request adds handling of a record pointer to the implementation of the automation interface by retrieving the records ITypeInfo interface and passing it together with the record pointer in a VARIANT to the interface method.

[junkmd]

Thank you for your contribution.
I have a few questions and requests.

• Is there a COM specification document that serves as the basis for this behavior?
• Can you tell us about a use case that requires this feature?
  I assume this PR is likely to solve a problem you are facing, but please tell us about it if there are no issues with NDA or licensing.
• How can this behavior be tested?
  If possible, please write test cases in comtypes/test/test_variant.py.
  We would like to confirm whether this change will break backward compatibility or not.

[geppi]

Let's start with the use case because that's the easiest.

I'm working with an optical engineering application from Photon Engineering called FRED which does offer a COM interface for scripting. The FRED type library defines various structures to describe optical elements and geometries and a large number of the COM interface methods expect a structure pointer as an [in, out] parameter and modify the structure members on the server side. The modified structures are required as the result of the method call on the client side.

With the current implementation of dispatch interfaces in comtypes it is only possible to pass a structure/record by value.
Therefore the modifications applied by the COM interface methods on the server side are not visible on the client side. The structures/records are unchanged.

As far as the COM specification is concerned, I can only say that since Windows NT 4.0 SP4 the type library marshaler used by automation interfaces does support structures as parameters. Also comtypes supports passing parameters which are defined as [in, out] structure pointers in a type library when using a Dual interface and server side changes to the structures are then visible on the client side in that case.

For pure dispatch interfaces a complicating factor is that the method parameters have to be packaged into a VARIANT.
The specification of the VARIANT Type Constants does not forbid the combination of VT_RECORD | VT_BYREF. Using this VARIANT type with a record pointer packaged together with a pointer to its ITypeInfo interface works as expected.

Finally the test case which is probably the hardest part for me since I'm not a professional full time developer.
Looking for an example, I've searched the code under comtypes/test for a test case of passing a record by value in a VARIANT which is already implemented in automation.py lines 349-359.
Unfortunately I failed to find something and therefore I'm a little clueless how a test case for my pull request should look like.

I had written a small COM server in C++ to develop my patch and I can test the functionality of comtypes with my patch against this server. Also a Python client using comtypes with the patch does properly work with the FRED optical engineering application and gets the server side structure modifications marshaled back to the client side.

A hint how a test case should look like would be very much appreciated.

[junkmd]

I wasn't very familiar with FRED, but from the information below, I could indeed confirm that there is a COM interface.
https://photonengr.com/fred-automation-with-matlab/

From your explanation, it seems that your patch will indeed be useful for this project.

As you say, testing proprietary software's COM type libraries is difficult.
However,

I had written a small COM server in C++ to develop my patch and I can test the functionality of comtypes with my patch against this server. Also, a Python client using comtypes with the patch does properly work with the FRED optical engineering application and gets the server-side structure modifications marshaled back to the client side.

If that's the case, it will be helpful for testing.

I envision a test workflow like the following:

1. Build the COM type library from this C++ code (and something like .sln files) with GitHub Actions, and register it.
2. Call comtype.client.GetModule in the Python test code, instantiate the defined interface in the generated module with CreateObject/CoCreateInstance, and verify its behavior.

I think this tool will be useful.
https://github.com/microsoft/setup-msbuild

I recommend trying out the Actions workflow configuration in your public repository before adding commits to the comtypes fork repository. By making that repository visible to the community, you might receive advice and tips.

This is also my first time attempting such a thing, so let's ask the community for help if we run into trouble.

Best regards.

[geppi]

OK, please lets double check if I correctly understand what you're proposing.

The Python test case for the record pointer parameters should run against the C++ COM server.
Does that mean that I need to open another PR to also get the C++ COM server code checked into the comtypes repo?
Moreover, also the github Actions workflow will need to be included, right?

I'm currently completely unfamiliar with github Actions and first need to learn what's required.
Also my COM server is written in pure C++ unmanaged code, so no .NET and simply compiled with a Makefile using nmake.
I need to check how this fits into an Actions workflow.

So if that's what you're proposing I'm happy to dive into all this but it will for sure take some time.

Best regards

[junkmd]

Thank you for your cooperation with this project.

The Python test case for the record pointer parameters should run against the C++ COM server.

This is exactly what we should aim for.

Does that mean that I need to open another PR to also get the C++ COM server code checked into the comtypes repo?

It's better to include the tests that verify the behavior of the added or changed features in the same PR for coherence, so there's not necessarily a need to post another PR. It would be sufficient if the added tests fail when the production code changes are removed from this PR.

However, if you are concerned about applying a large number of changes in one patch, it's also okay to split the PR into two, one for the test code and one for the production code.
In that case, in the first PR that only changes the test code, decorate the failing tests with skip or expectedFailure.
Then, in the second PR that changes the production code, remove those decorators from the tests that are now successful.
Either way, please proceed in the way you prefer.

Moreover, also the github Actions workflow will need to be included, right?

That's correct. Building the COM server within the CI environment is essential, as without it, CI testing cannot be performed.

I'm currently completely unfamiliar with github Actions and first need to learn what's required.
Also my COM server is written in pure C++ unmanaged code, so no .NET and simply compiled with a Makefile using nmake.
I need to check how this fits into an Actions workflow.

I'm happy to help with your learning, of course.
In fact, it simply involves adding your files to this patch and incorporating the build commands you use in your environment into the autotest.yml file of the current workflow.

I believe you would probably make changes to the workflow like this:

comtypes/.github/workflows/autotest.yml

Lines 22 to 30 in 6b81d07

	- name: install comtypes
	run: |
	pip install --upgrade setuptools
	python setup.py install
	pip uninstall comtypes -y
	python test_pip_install.py
	- name: unittest comtypes
	run: |
	python -m unittest discover -v -s ./comtypes/test -t comtypes\test

      - name: install comtypes
        run: |
          pip install --upgrade setuptools
          python setup.py install
          pip uninstall comtypes -y
          python test_pip_install.py
+     - name: Set up MSBuild
+       uses: microsoft/setup-msbuild@v2
+     - name: Build RecordInfo testing COM server with nmake
+       run: |
+        cd path/to/your/code
+        nmake /f Makefile
+        cd ../../../..
+     - name: Register COM Server
+       run: regsvr32 /s destination/of/dll/output/YourComServer.dll
      - name: unittest comtypes
        run: |
          python -m unittest discover -v -s ./comtypes/test -t comtypes\test
+     - name:Unregister COM Server
+       run: regsvr32 /u destination/of/dll/output/YourComServer.dll

I recommend that you first try the above-mentioned steps.

Best regards

[geppi]

Thank you for the example, it is very helpful.
Nevertheless, bear with me while I work on the test case.

[junkmd]

You’re welcome.

Such tests are rare and challenging among the many OSS projects.

Therefore, if you encounter any problems while writing tests or adding/changing CI workflows, please feel free to share with the community.

[geppi]

OK, I've cleaned up the C++ COM server code a little now and would be ready to give the proposed testing workflow a try.
However, I'm not sure what the best place would be for the C++ source code.

There is a sub-directory source which contains code for what seems to be an ATL COM server for testing. Is this used somewhere?
The C++ COM server source code could probably go into a sub-directory below this source directory. Maybe it would then make sense to also put the ATL COM server code into its own sub-directory.

Since this C++ COM server is an out of process server it is required to run it as Administrator for registering and unregistering. As far as I understand the test workflow does run under the Administrator user, right?

The COM server will not be part of the released comtypes package. However, the testing code is part of the package. That would mean that tests targeting the out of process COM server will fail when a user will run the test code from the package, right?

[junkmd]

Good catch. Thank you!

There is a sub-directory source which contains code for what seems to be an ATL COM server for testing. Is this used somewhere?

The ROOT/source is referenced in test_avmc.py.
However, this test is currently skipped before it runs, so the C++ code, including Avmc.cpp, is not currently used in this project.

Once upon a time, most of the comtypes tests were broken, so in #267, #271, and #298, we made some tests runnable in CI.
However, at that point, we were unable to make test_avmc.py and several other tests executable.

comtypes/comtypes/test/test_avmc.py

Lines 1 to 16 in c3a242d

	import unittest
	from comtypes.client import CreateObject
	from comtypes.test.find_memleak import find_memleak
	@unittest.skip(
	"This test does not work. Apparently it's supposed to work with the 'avmc' stuff "
	"in comtypes/source, but it doesn't. It's not clear to me why."
	)
	class Test(unittest.TestCase):
	"""Test COM records"""
	def test(self):
	# The ATL COM dll
	avmc = CreateObject("AvmcIfc.Avmc.1")

I think that if this COM server and test are revived, it might be possible to guarantee the quality of the COM server part that we have not been able to guarantee so far.
However, to keep changes to a minimum, let's focus on "record pointers" for now and postpone considering these revivals.

The C++ COM server source code could probably go into a sub-directory below this source directory. Maybe it would then make sense to also put the ATL COM server code into its own sub-directory.

I agree with this point.
For confirmation, I executed the following to move the current COM server codebase to a subdirectory, but the existing unskipped tests did not break.

git mv source/ tmp/
mkdir source/
git mv tmp/ source/avmc

Since this C++ COM server is an out of process server it is required to run it as Administrator for registering and unregistering. As far as I understand the test workflow does run under the Administrator user, right?

Your understanding is probably correct.
I can find several repositories running regsvr32 on the workflow.

The COM server will not be part of the released comtypes package. However, the testing code is part of the package. That would mean that tests targeting the out of process COM server will fail when a user will run the test code from the package, right?

Even in the current tests, there are tests that depend on proprietary software that the developers in this community may not have. However, those are skipped if the software is not installed. test_excel is an example of this.

comtypes/comtypes/test/test_excel.py

Lines 20 to 26 in c3a242d

	try:
	GetModule(("{00020813-0000-0000-C000-000000000046}",)) # Excel libUUID
	from comtypes.gen.Excel import xlRangeValueDefault
	IMPORT_FAILED = False
	except (ImportError, OSError):
	IMPORT_FAILED = True

comtypes/comtypes/test/test_excel.py

Lines 131 to 132 in c3a242d

	@unittest.skipIf(IMPORT_FAILED, "This depends on Excel.")
	class Test_LateBind(BaseBindTest, unittest.TestCase):

At this stage, for tests that depend on your COM server, I think it is required the same kind of conditional branch.

Please feel free to include your code in this patch.
I suspect that for both you and me, there are many things that we won’t know until we run it actually in the CI environment.

[geppi]

I added the source files for the COM-server and a unittest file. The next step would be to modify the test workflow.

[geppi]

I amended the last commit to fix the formatting errors.

[junkmd]

Thank you for your contribution.
I believe your COM server codebase will be a very useful asset to this project.
I am not very familiar with C++, but I still think it is good code that clearly shows how the implementation of the COM server affects the behavior of comtypes.
"Inside COM" has been out of print for a long time, but I know that the Japanese translated version is stored in the National Diet Library of Japan.
I plan to make some free time in the near future to go and read it as it seems to be useful for this project.

I have a few questions and requests.

• How exactly are you building and registering these C++ codes placed here on your local environment?
  If we set up the workflow to do the same thing you are doing in your environment, I think the build on GitHub Actions will also go well.
• Can you change the name of the COM server for testing from ComtypesTestLib?
  Considering that this COM server only implements functionality for Structure with a _recordinfo_ attribute, it seems unbalanced in terms of responsibility to give this name.
  Also, if we increase the test COM servers in the future and give them the same name, comtypes may generate Python wrapper modules with duplicate names.
  Even if it becomes redundant, I think it would be more conducive to the extensibility of this project to be able to express a single responsibility that matches the implemented functionality and test target, such as ComtypesDispIfcRecordTestLib.
• Can you also change the subdirectory name (like with the COM server name)?
• Can you implement a method on the COM server that covers elif isinstance(value, Structure) and hasattr(value, "_recordinfo_"):?
  To my understanding, there should be no COM type libraries that generate a module containing a Structure with a _recordinfo_ attribute through GetModule among those currently used for automatic testing in this project.
  Similarly, there should be no test to verify that it enters that branch when tagVARIANT._set_value is called.
  (I confirmed by inserting print into the code below in my environment, but there were no tests that entered that branch)
  

  comtypes/comtypes/automation.py

  Lines 349 to 359 in 211c6bf

  	elif isinstance(value, Structure) and hasattr(value, "_recordinfo_"):
  	guids = value._recordinfo_
  	from comtypes.typeinfo import GetRecordInfoFromGuids
  	ri = GetRecordInfoFromGuids(*guids)
  	self.vt = VT_RECORD
  	# Assigning a COM pointer to a structure field does NOT
  	# call AddRef(), have to call it manually:
  	ri.AddRef()
  	self._.pRecInfo = ri
  	self._.pvRecord = ri.RecordCreateCopy(byref(value))

  
  As you commented before, it is a part that you referred to, but it should not be guaranteed by automatic testing at present.
  So if it's not too much trouble, I would like to ask you to add the tests.

Best regards

[geppi]

"Inside COM" has been out of print for a long time, but I know that the Japanese translated version is stored in the National Diet Library of Japan.

Yeah, I was lucky to find a cheap used copy. It is a great resource to understand the COM basics. Another book I can recommend is "Essential COM, Don Box, Addison Wesley 1998, ISBN: 0-201-63446-5" written by one of the creators and probably the authoritative book on the concepts and architecture of COM.

How exactly are you building and registering these C++ codes placed here on your local environment?

I simply run nmake in the source directory from within a Visual Studio Developer Command Shell. This has all the environment variables properly set to access the required system header files and libs. Apart from that, the code is pretty much self contained. I have a modified workflow ready based on your suggestion which uses: microsoft/setup-msbuild@v2. As far as I understand this should set up a similar environment like the VS Developer Command Shell.

Can you change the name of the COM server for testing from ComtypesTestLib?

Of course I can do that. However, i think this out of process COM-server could be used for more than just testing the record pointer parameters. You would just need to add methods with the parameter types you like to test. The places to edit for this are:

• SERVER.IDL to include the method in the type library
• CMPNT.H to declare the method as virtual HRESULT __stdcall
• CMPNT.CPP to implement the method

This would be pretty straight forward.

Can you also change the subdirectory name (like with the COM server name)?

Yes. However, I would propose to first focus on making the mechanics work. The names can all be changed in a final commit before merging. I'm open for suggestions but would prefer renaming everything in one step.

Can you implement a method on the COM server that covers elif isinstance(value, Structure) and hasattr(value, "recordinfo"):?

Do you mean implementing a method that expects a record instead of a record pointer?
This is indeed not be a big deal and goes along with what I said above about extending the scope of the out of process COM-server for testing. However, please lets focus on integrating this into the testing workflow before extending it.

I will next push the modified workflow file.

[junkmd]

Thank you for adding the workflow.

Indeed, as you pointed out, it's more sensible to prioritize ensuring that the existing tests pass before considering renaming the COM server or adding methods to handle records instead of record pointers, following the flow of test-driven development.

For now, I'll give your written workflow a try.
If nmake doesn't work as expected on GHA, I'd recommend using ilammy/msvc-dev-cmd@v1 instead of microsoft/setup-msbuild@v2.

[geppi]

Looks like I misunderstood the meaning of the $GITHUB_WORKSPACE workflow environment variable.
Obviously it puts me into the root of the 'D:' drive of the (windows-2019, 3.10, x86) virtual machine.

[geppi]

Can you help me please what I need to do to assure that I change into the proper source directory?

[junkmd]

Let's try this out and see if it works.

[geppi]

Does it need a backslash to find the server.exe???
Or is the change to the source/OutProcSrv directory local to a particular 'run' and I need to put it at the beginning of the server registration block again?

[geppi]

OK, then lets leave unregistering the OutProc COM server in the workflow but remove the cd ../../ lines.
I've amended the last commit again.

Shall we extend the scope of this pull request to cover testing records passed by value via a dispinterface as well?
I could add a simple method to the COM server code and a call to this method in the unittest.

However, if we do this I would propose to rename the unittest file to 'test_dispifc_records.py'.

Finally, please let me know which names you would prefer for the type library, component and interfaces considering that other tests could make use of the OutProc COM server as well.

[junkmd]

OK, then lets leave unregistering the OutProc COM server in the workflow but remove the cd ../../ lines.
I've amended the last commit again.

Thank you.

Shall we extend the scope of this pull request to cover testing records passed by value via a dispinterface as well? I could add a simple method to the COM server code and a call to this method in the unittest.

However, if we do this I would propose to rename the unittest file to 'test_dispifc_records.py'.

Yes, let's proceed with that.
With this, the COM server will serve as a test double that covers isinstance(foo, Structure) and hasattr(foo, "_recordinfo_") conditions with high cohesion and single responsibility.

Finally, please let me know which names you would prefer for the type library, component, and interfaces considering that other tests could make use of the OutProc COM server as well.

I think the type library is might be ComtypesDispIfcRecordTestLib, as I proposed earlier. This suggests its relevance to the Python test module named 'test_dispifc_records.py'.

For each object, I suggest the following names based on the Python wrapper classes generated in comtypes.gen, aiming for clarity and future-proofing with redundant yet distinguishable names and prefixes.
I'm aware that naming is NOT my strongest suit.
So feel free to provide feedback without hesitation.

• IComtypesTest -> IComtypesDispIfcRecordTest
• IDualComtypesTest -> IDualComtypesDispIfcRecordTest
• T_TEST_RECORD -> StructComtypesDispIfcRecordTest
• Component -> CoComtypesDispIfcRecordTest

It's past midnight in Japan, so I'm about to turn in for the night. I expect to be able to respond again after lunchtime tomorrow.

Thank you for your continued cooperation.

[geppi]

I'm afraid that IDualComtypesDispIfcRecordTest would be kind of a misnomer. It's misleading because it carries 'dual' and abbreviated 'dispinterface' in its name.
I agree that it's a good idea to provide the information about the type of interface and what it's used for in the name. Therefore I would propose the follwowing names for the interfaces.

• IComtypesTest -> IDispComtypesRecordTest
• IDualComtypesTest -> IDualComtypesRecordTest

However, I'm afraid the name you suggest for the type library would be a little bit too limiting.
The COM server can easily be extended for other tests. For example, I'm very interested to make SAFEARRAY pointers work as method parameters via a dispinterface. The methods to test this could be published via another interface that would follow the naming scheme suggested above. The type library name should therefore be much more generic and not just look like it's limited to record testing. I would propose ComtypesCppOutProcTestSrvLib (uuhh what a name :-).

Regarding the component name I think we should not make it too specific but still let it tell the scope of functionality this component provides. Also connected with the component name are the 'ProgID' and the 'friendly name of the component which are all defined in 'SERVER.CPP'. I would propose the following names:

• Component -> CoComtypesDispIfcTests
• friendly name: Comtypes component for dispinterface tests
• ProgID: Comtypes.Test.Dispinterfaces.1
• Version-independent ProgID: Comtypes.Test.Dispinterfaces.1

In a future step, this would for example allow to add the interfaces IDualComtypesSafearrayTest and IDispComtypesSafearrayTest to this component and even to extend the functionality of the server beyond the testing of dispinterfaces by adding another component with a different scope.

[junkmd]

Thank you for your feedback.

I'm afraid that IDualComtypesDispIfcRecordTest would be kind of a misnomer. It's misleading because it carries 'dual' and abbreviated 'dispinterface' in its name. I agree that it's a good idea to provide the information about the type of interface and what it's used for in the name. Therefore I would propose the following names for the interfaces.

• IComtypesTest -> IDispComtypesRecordTest
• IDualComtypesTest -> IDualComtypesRecordTest

Indeed, as you pointed out, having "Dual" and "Disp" in the interface name at the same time can cause confusion.
I agree with renaming the interfaces to these names.

However, I'm afraid the name you suggest for the type library would be a little bit too limiting. The COM server can easily be extended for other tests. For example, I'm very interested to make SAFEARRAY pointers work as method parameters via a dispinterface. The methods to test this could be published via another interface that would follow the naming scheme suggested above. The type library name should therefore be much more generic and not just look like it's limited to record testing. I would propose ComtypesCppOutProcTestSrvLib (uuhh what a name :-).

As you say, the name of the type library could certainly be something like ComtypesCppOutProcTestSrvLib, which is not too limiting of responsibility.
Even if there is a large number of components and interfaces in the same COM library, if they are not dependent on each other, future maintainers will not be confused.
What's important is to keep the codebase managed in this repository simple, keep the cost of CI small, and make it easy to contribute.
Since building a COM library takes a considerable amount of time, it makes sense to reuse one COM library for testing.

Regarding the component name I think we should not make it too specific but still let it tell the scope of functionality this component provides. Also connected with the component name are the 'ProgID' and the 'friendly name of the component which are all defined in 'SERVER.CPP'. I would propose the following names:

• Component -> CoComtypesDispIfcTests
• friendly name: Comtypes component for dispinterface tests
• ProgID: Comtypes.Test.Dispinterfaces.1
• Version-independent ProgID: Comtypes.Test.Dispinterfaces.1

In a future step, this would for example allow to add the interfaces IDualComtypesSafearrayTest and IDispComtypesSafearrayTest to this component and even to extend the functionality of the server beyond the testing of dispinterfaces by adding another component with a different scope.

On the other hand, I think we should operate so that "the interfaces implemented by the component" (NOTE: In other words, it is "the collection of interfaces provided by the component" or "the content that is assigned to the _com_interfaces_ attribute of CoClass subclasses") are as few as possible and changes to the component once defined are as few as possible.
I am concerned that when future maintainers try to add new test targets, changes to add interfaces to existing components will break existing tests.
I think it is appropriate for one component to correspond to one test perspective or target, so I think a separate component should be prepared for SAFEARRAY tests.
Based on the above, I propose a redundant name that limits the role, such as CoComtypesRecordTest, for the component.

Best regards

[junkmd]

Memorandum

At the point of c000104

Modules generated from the COM type library

_07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0.py

wrapper module

# -*- coding: mbcs -*-

from ctypes import *
import comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0
from comtypes import (
    _check_version, BSTR, CoClass, COMMETHOD, dispid, DISPMETHOD, GUID
)
from ctypes.wintypes import VARIANT_BOOL
from ctypes import HRESULT
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing import Any, Tuple
    from comtypes import hints


_lcid = 0  # change this if required
typelib_path = 'D:\\a\\comtypes\\comtypes\\source\\OutProcSrv\\server.tlb'
WSTRING = c_wchar_p



class IComtypesTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IDispatch):
    _case_insensitive_ = True
    _iid_ = GUID('{033E4C10-0A7F-4E93-8377-499AD4B6583A}')
    _idlflags_ = []
    _methods_ = []

    if TYPE_CHECKING:  # dispmembers
        def InitRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...


class T_TEST_RECORD(Structure):
    _recordinfo_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0, 0, '{00FABB0F-5691-41A6-B7C1-11606671F8E5}')


IComtypesTest._disp_methods_ = [
    DISPMETHOD(
        [dispid(1)],
        None,
        'InitRecord',
        (['in', 'out'], POINTER(T_TEST_RECORD), 'test_record')
    ),
]

T_TEST_RECORD._fields_ = [
    ('question', BSTR),
    ('answer', c_int),
    ('needs_clarification', VARIANT_BOOL),
]

assert sizeof(T_TEST_RECORD) == 16, sizeof(T_TEST_RECORD)
assert alignment(T_TEST_RECORD) == 8, alignment(T_TEST_RECORD)


class Library(object):
    """Comtypes Test COM Server 1.0 Type Library"""
    name = 'ComtypesTestLib'
    _reg_typelib_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0)


class IDualComtypesTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IDispatch):
    """IDualComtypesTest Interface"""
    _case_insensitive_ = True
    _iid_ = GUID('{0C4E01E8-4625-46A2-BC4C-2E889A8DBBD6}')
    _idlflags_ = ['dual', 'nonextensible', 'oleautomation']

    if TYPE_CHECKING:  # commembers
        def InitRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...


IDualComtypesTest._methods_ = [
    COMMETHOD(
        [dispid(1)],
        HRESULT,
        'InitRecord',
        (['in', 'out'], POINTER(T_TEST_RECORD), 'test_record')
    ),
]

################################################################
# code template for IDualComtypesTest implementation
# class IDualComtypesTest_Impl(object):
#     def InitRecord(self):
#         '-no docstring-'
#         #return test_record
#


class Component(CoClass):
    """Component Class"""
    _reg_clsid_ = GUID('{06571915-2431-4CA3-9C01-53002B060DAB}')
    _idlflags_ = []
    _typelib_path_ = typelib_path
    _reg_typelib_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0)


Component._com_interfaces_ = [IDualComtypesTest, IComtypesTest]

__all__ = [
    'IDualComtypesTest', 'typelib_path', 'Library', 'IComtypesTest',
    'T_TEST_RECORD', 'Component'
]

_check_version('1.4.2', 1716475069.367991)

ComtypesTestLib.py

friendly module

from enum import IntFlag

import comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0 as __wrapper_module__
from comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0 import (
    IDualComtypesTest, BSTR, COMMETHOD, HRESULT, DISPMETHOD, _lcid,
    WSTRING, dispid, typelib_path, VARIANT_BOOL, Library,
    IComtypesTest, T_TEST_RECORD, _check_version, Component, GUID,
    CoClass
)


__all__ = [
    'IDualComtypesTest', 'typelib_path', 'Library', 'IComtypesTest',
    'T_TEST_RECORD', 'Component'
]

Test that fails when (if) the changes to tagVARIANT._set_value are reverted

traceback

======================================================================
ERROR: test (test_dispifc_recordptr.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\comtypes\comtypes\comtypes\test\test_dispifc_recordptr.py", line 36, in test
    dispifc.InitRecord(byref(test_record))
  File "D:\a\comtypes\comtypes\comtypes\_memberspec.py", line 495, in func
    return obj.Invoke(memid, _invkind=1, *args, **kw)  # DISPATCH_METHOD
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 874, in Invoke
    dp = self.__make_dp(_invkind, *args)
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 849, in __make_dp
    array[i].value = a
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 396, in _set_value
    self.vt = _ctype_to_vartype[type(ref)] | VT_BYREF
KeyError: <class 'comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0.T_TEST_RECORD'>

[junkmd]

Thank you for the addition.
It's great work that the test double can cover not only the record pointer, but also when a record is passed to the method.

The following my feedback is NOT a request to change the code exactly as stated and immediately.
It's a suggestion to split into multiple tests for each focus, rather than overwriting local variables in one test, to make future maintenance easier.
You may have been considering doing the same thing in a refactoring step, but I thought I'd share my thoughts just in case.

[geppi]

Looks like everything is in place now and the only thing left to fix is the naming of the interfaces, component and type library.

Since you're the maintainer of this repo the decision is up to you. Nevertheless, I gave this a second thought and here are my five cent on this topic.

I would suggest to call the interfaces simply IDualRecordParamTest and IDispRecordParamTest.
These names tell what kind of interface it is and what they're good for.
Since COM interfaces are decoupled from the actual implementation there is no need to burden the names with the name of the component or the application. These interfaces could be implemented in the future by a different COM server based on ATL or .NET. They could even be used and implemented by some other project.

Regarding the name of the type library, I think that the monster ComtypesCppOutProcTestSrvLib could be prettified to just ComtypesCppTestSrvLib. I doubt that there will ever be the requirement and implementation of an InProc C++ COM server for testing.

The component naming depends on how you want to handle other tests. Since interfaces should not be modified or extended after their publication it is required to define new interfaces for future tests. However, it is not required to implement each of these interfaces by a different component. I would favor to keep the implementation for related interfaces in the same component, e.g. for testing SAFEARRAY parameters to dispmehtods. I don't think that there is less chance to break an existing test by moving the implementation of a new interface into a different component. It even requires more code to be added and more places in the C++ code to be modified. Therefore I'm still in favor of the names:

• Component: CoComtypesDispIfcTests
• friendly name: Comtypes component for dispinterface tests
• ProgID: Comtypes.Test.Dispinterfaces.1
• Version-independent ProgID: Comtypes.Test.Dispinterfaces

The final decision is up to you.
Please just provide me the list with all names that should be changed so that this PR can finally be merged.

[junkmd]

First, I will add/change the missing parts to the Python test code.

Thank you for your feedback on the comments.
Regarding the COM server, I am currently organizing my thoughts, and I appreciate your patience while I work on it.

[geppi]

No need to hurry. I understand that all this needs to be considered carefully.
Thinking again about the component naming, the following would probably make it more specific while still covering related interfaces:

• Component: CoComtypesDispIfcParamTests
• friendly name: Comtypes component for dispinterface parameter tests
• ProgID: Comtypes.Test.DispinterfaceParams.1
• Version-independent ProgID: Comtypes.Test.DispinterfaceParams

[junkmd]

Thank you for your feedback.

I would suggest to call the interfaces simply IDualRecordParamTest and IDispRecordParamTest.

I agree with this.
It clearly says that these are test doubles used for tests related to records.
I think they are good names.

Regarding the name of the type library, I think that the monster ComtypesCppOutProcTestSrvLib could be prettified to just ComtypesCppTestSrvLib. I doubt that there will ever be the requirement and implementation of an InProc C++ COM server for testing.

I also agree with this.
Even if we add an InProc and C++ implemented COM server as a test double in the future, I think it should be named based on different characteristics or test perspectives, and it should be.

The component naming depends on how you want to handle other tests. Since interfaces should not be modified or extended after their publication it is required to define new interfaces for future tests. However, it is not required to implement each of these interfaces by a different component. I would favor to keep the implementation for related interfaces in the same component, e.g. for testing SAFEARRAY parameters to dispmehtods. I don't think that there is less chance to break an existing test by moving the implementation of a new interface into a different component. It even requires more code to be added and more places in the C++ code to be modified. Therefore I'm still in favor of the names:

• Component: CoComtypesDispIfcTests
• friendly name: Comtypes component for dispinterface tests
• ProgID: Comtypes.Test.Dispinterfaces.1
• Version-independent ProgID: Comtypes.Test.Dispinterfaces.1

Thinking again about the component naming, the following would probably make it more specific while still covering related interfaces:

• Component: CoComtypesDispIfcParamTests
• friendly name: Comtypes component for dispinterface parameter tests
• ProgID: Comtypes.Test.DispinterfaceParams.1
• Version-independent ProgID: Comtypes.Test.DispinterfaceParams

As you pointed out, it is a problem that the places where C++ code needs to be added increase and it becomes difficult to contribute.
Therefore, I agree with your proposal to name the component CoComtypesDispIfcTestsCoComtypesDispIfcParamTests and that multiple interfaces will be implemented.
Considering this, I think it would be good to name C++ filenames like SERVER.CPP in accordance with CoComtypesDispIfcTestsCoComtypesDispIfcParamTests.

Also, I have written suggestions in the review comments to prevent fragile tests from being added by future contributors.

[junkmd]

Thank you.
I think that with a few more changes, such as renaming the structure and adding comments, it will meet the necessary quality to merge.

CoComtypesDispIfcParamTests.cpp(102) is fixed.

[junkmd]

Memorandum

At the point of 4eee732

Modules generated from the COM type library

comtypes/gen/_07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0.py

wrapper module

# -*- coding: mbcs -*-

from ctypes import *
import comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0
from comtypes import (
    _check_version, BSTR, CoClass, COMMETHOD, dispid, DISPMETHOD, GUID
)
from ctypes import HRESULT
from ctypes.wintypes import VARIANT_BOOL
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing import Any, Tuple
    from comtypes import hints


_lcid = 0  # change this if required
typelib_path = 'D:\\a\\comtypes\\comtypes\\source\\CppTestSrv\\server.tlb'
WSTRING = c_wchar_p



class IDualRecordParamTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IDispatch):
    """Dual Interface for testing record parameters."""
    _case_insensitive_ = True
    _iid_ = GUID('{0C4E01E8-4625-46A2-BC4C-2E889A8DBBD6}')
    _idlflags_ = ['dual', 'nonextensible', 'oleautomation']

    if TYPE_CHECKING:  # commembers
        def InitRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...
        def VerifyRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...


class StructRecordParamTest(Structure):
    _recordinfo_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0, 0, '{00FABB0F-5691-41A6-B7C1-11606671F8E5}')


IDualRecordParamTest._methods_ = [
    COMMETHOD(
        [dispid(1)],
        HRESULT,
        'InitRecord',
        (['in', 'out'], POINTER(StructRecordParamTest), 'test_record')
    ),
    COMMETHOD(
        [dispid(2)],
        HRESULT,
        'VerifyRecord',
        (['in'], POINTER(StructRecordParamTest), 'test_record'),
        (['out', 'retval'], POINTER(VARIANT_BOOL), 'result')
    ),
]

################################################################
# code template for IDualRecordParamTest implementation
# class IDualRecordParamTest_Impl(object):
#     def InitRecord(self):
#         '-no docstring-'
#         #return test_record
#
#     def VerifyRecord(self, test_record):
#         '-no docstring-'
#         #return result
#

StructRecordParamTest._fields_ = [
    ('question', BSTR),
    ('answer', c_int),
    ('needs_clarification', VARIANT_BOOL),
]

assert sizeof(StructRecordParamTest) == 16, sizeof(StructRecordParamTest)
assert alignment(StructRecordParamTest) == 8, alignment(StructRecordParamTest)


class CoComtypesDispIfcParamTests(CoClass):
    """Comtypes component for dispinterface parameter tests."""
    _reg_clsid_ = GUID('{06571915-2431-4CA3-9C01-53002B060DAB}')
    _idlflags_ = []
    _typelib_path_ = typelib_path
    _reg_typelib_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0)


class IDispRecordParamTest(comtypes.gen._00020430_0000_0000_C000_000000000046_0_2_0.IDispatch):
    """Dispinterface for testing record parameters."""
    _case_insensitive_ = True
    _iid_ = GUID('{033E4C10-0A7F-4E93-8377-499AD4B6583A}')
    _idlflags_ = []
    _methods_ = []

    if TYPE_CHECKING:  # dispmembers
        def InitRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...
        def VerifyRecord(self, test_record: hints.Incomplete) -> hints.Incomplete: ...


CoComtypesDispIfcParamTests._com_interfaces_ = [IDualRecordParamTest, IDispRecordParamTest]


class Library(object):
    """Comtypes C++ Test COM Server 1.0 Type Library."""
    name = 'ComtypesCppTestSrvLib'
    _reg_typelib_ = ('{07D2AEE5-1DF8-4D2C-953A-554ADFD25F99}', 1, 0)


IDispRecordParamTest._disp_methods_ = [
    DISPMETHOD(
        [dispid(1)],
        None,
        'InitRecord',
        (['in', 'out'], POINTER(StructRecordParamTest), 'test_record')
    ),
    DISPMETHOD(
        [dispid(2)],
        VARIANT_BOOL,
        'VerifyRecord',
        (['in'], POINTER(StructRecordParamTest), 'test_record')
    ),
]

__all__ = [
    'CoComtypesDispIfcParamTests', 'StructRecordParamTest',
    'IDispRecordParamTest', 'IDualRecordParamTest', 'Library',
    'typelib_path'
]

_check_version('1.4.2', 1716906364.199293)

comtypes/gen/ComtypesCppTestSrvLib.py

friendly module

from enum import IntFlag

import comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0 as __wrapper_module__
from comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0 import (
    CoComtypesDispIfcParamTests, DISPMETHOD, CoClass, _lcid,
    StructRecordParamTest, IDispRecordParamTest, GUID, BSTR,
    IDualRecordParamTest, Library, VARIANT_BOOL, HRESULT,
    typelib_path, COMMETHOD, _check_version, dispid, WSTRING
)


__all__ = [
    'CoComtypesDispIfcParamTests', 'StructRecordParamTest',
    'IDispRecordParamTest', 'IDualRecordParamTest', 'Library',
    'typelib_path'
]

Test that fails when (if) the changes to tagVARIANT._set_value are reverted

traceback

======================================================================
ERROR: test_byref (test_dispifc_records.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\comtypes\comtypes\comtypes\test\test_dispifc_records.py", line 43, in test_byref
    dispifc.InitRecord(byref(test_record))
  File "D:\a\comtypes\comtypes\comtypes\_memberspec.py", line 495, in func
    return obj.Invoke(memid, _invkind=1, *args, **kw)  # DISPATCH_METHOD
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 874, in Invoke
    dp = self.__make_dp(_invkind, *args)
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 849, in __make_dp
    array[i].value = a
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 396, in _set_value
    self.vt = _ctype_to_vartype[type(ref)] | VT_BYREF
KeyError: <class 'comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0.StructRecordParamTest'>

======================================================================
ERROR: test_pointer (test_dispifc_records.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\comtypes\comtypes\comtypes\test\test_dispifc_records.py", line 57, in test_pointer
    dispifc.InitRecord(pointer(test_record))
  File "D:\a\comtypes\comtypes\comtypes\_memberspec.py", line 495, in func
    return obj.Invoke(memid, _invkind=1, *args, **kw)  # DISPATCH_METHOD
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 874, in Invoke
    dp = self.__make_dp(_invkind, *args)
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 849, in __make_dp
    array[i].value = a
  File "D:\a\comtypes\comtypes\comtypes\automation.py", line 401, in _set_value
    self.vt = _ctype_to_vartype[type(ref)] | VT_BYREF
KeyError: <class 'comtypes.gen._07D2AEE5_1DF8_4D2C_953A_554ADFD25F99_0_1_0.StructRecordParamTest'>

----------------------------------------------------------------------

[junkmd]

I have incorporated the test perspective into the method name of the test case.

Although there is only one test case in this module, I have also named it according to its perspective.

[junkmd]

I found it off that the component name is CoComtypesDispIfcParamTests and the ProgID is "Comtypes.DispIfcParamTests", even though both the Dispatch and Dual interfaces are implemented.

However, after going through the following thought process, I no longer find it off.

There are indeed differences between DispMethods and ComMethods even if with the same name, such as being able to change field values by passing not only pointers and references but also structures to IDualRecordParamTest.InitRecord, and not causing reflection by passing not only structures but also pointers and references to IDualRecordParamTest.VerifyRecord ¹.

On the other hand, the ability to change the field value by passing a record pointer to InitRecord, and the field value does not change when a record is passed to VerifyRecord, is the same for both IDualRecordParamTest and IDispRecordParamTest.
I think this meets the Liskov substitution principle.
Considering that the Dual interface is derived from IDispatch, I no longer find the component name problematic. If the name is ...DispIfc..., it might prevent future maintainers from implementing interfaces that only support early binding in this component.

If you agree with this, I will approve this patch.
If you feel it inconsistent, please add commits such as name changes.

What do you think?

Best regards

Footnotes

1. These are not bugs in comtypes, but specifications based on the implementation of the COM server and COM type library.
   Also, this based on the principles of COM, where the Dispatch interface supports late binding, and the Dual interface supports not only late binding but also early binding. ↩

[geppi]

I think your statement makes sense and if there would be "non dispinterfaces" added in the future they should definitely be implemented in a different component.

Do you want the commits in this PR be squashed into a single commit before merging?

On the one hand a single commit would not "pollute" the history with our "development process" on the other hand it might be useful to have some of the information contained in the process history. I have no experience with this and therefore I'm wondering what's best practice? Would the PR history still be visible if a squashed single commit would be merged into main?

[junkmd]

I think your statement makes sense and if there would be "non dispinterfaces" added in the future they should definitely be implemented in a different component.

Thank you.
It’s hard to judge by oneself whether a name is appropriate or how it will be interpreted by others.
Since I think the name given to an object is harder to change than the behavior implemented, I wanted to confirm here that there is no discrepancy in understanding the name between the author and the reviewer.

Do you want the commits in this PR be squashed into a single commit before merging?

In this project, with the collaborator permissions I have, I can only choose "squash and merge".
The default commit message when "squash and merge" on GitHub is a multi-line text that combines the messages in the commits to be squashed, and it also includes the PR number, etc.
This can sometimes make it cumbersome to inherit the change history with git mv or resolve conflicts.
But, from the commit message, we can trace which PR added the commit to main branch, I don’t feel inconvenienced except for those.
So, this patch will also be committed to main by "squash and merge".

On the one hand a single commit would not "pollute" the history with our "development process" on the other hand it might be useful to have some of the information contained in the process history. I have no experience with this and therefore I'm wondering what's best practice?

What is the best practice for merging depends on the nature of the project.
In the closed source project I am participating in, where we plan in advance within the team what should be committed to the codebase for each sprint, "rebase and merge" or "squash and merge" are rarely done but "creating a merge commit" is almost used.
However, in open source projects where what is committed to the codebase is often not determined by prior planning and is often determined by the priority of contributors, the change history as it is in the development process may reduce traceability.
I think "squash and merge" is often used in such projects.

Would the PR history still be visible if a squashed single commit would be merged into main?

Even if we "squash and merge"ed a PR, the commits we once pushed will not disappear from the PR page.

[geppi]

Thank you very much for the explanation.

So if I understood correctly there is nothing to do on my side for this "sqash and merge"?

The default commit message when "squash and merge" on GitHub is a multi-line text that combines the messages in the commits to be squashed, and it also includes the PR number, etc.

Wouldn't it be better to provide a refined commit message and just include the reference to the PR to keep track of the history?
When you say that this is the default commit message, would it be possible to amend the merge commit and change the commit message like for a "normal" commit in git?

[junkmd]

So if I understood correctly there is nothing to do on my side for this "sqash and merge"?

Yes, that’s correct. Just like when I first contributed to this project (#294) and wasn’t asked by the reviewer to do anything before merging, I don’t have anything I want you to do before merging this patch.

Wouldn't it be better to provide a refined commit message and just include the reference to the PR to keep track of the history?

I presume that you might be worried about losing information when squashing commits.
As for what the default commit message of a PR "squashed and merge"d on GitHub looks like, I think 876801f, which was merged in #473, can serve as a reference.
This includes the PR title, number, and co-authors.
I think this provides sufficient information.
Indeed, original commit hashes were not included, but as I mentioned earlier, the development history pushed to GitHub server does not disappear, so traceability is not lost.

When you say that this is the default commit message, would it be possible to amend the merge commit and change the commit message like for a "normal" commit in git?

“Technically it is possible”, but I do NOT plan to do it for the following reasons.

• --amend is one of the history-altering commands that completely changes the commit hash.
• The verified mark will be removed from the commit history.
• The commit hash when it was merged on the PR page and the actual commit history hash would differ. This could lead to more confusion.
• If I want to change the commit message from default when merging, I can edit it on the GitHub page before adding the commit.
  https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/

Furthermore, as mentioned above, I am satisfied with the granularity of the commit message that GitHub creates.
Rather than trying to refine the message and having contributors and reviewers exhaust themselves in discussion, delaying the merge of the crucial codebase, I would choose to use the default message.

[geppi]

OK, thank you again for the clarification.