Add Marshal.GetObjectForNativeVariant tests#31461
Add Marshal.GetObjectForNativeVariant tests#31461luqunl merged 2 commits intodotnet:masterfrom hughbe:marshal-variant-primitive-tests
Conversation
| } | ||
|
|
||
| #pragma warning disable 618 | ||
| public const ushort VT_EMPTY = 0; |
There was a problem hiding this comment.
Please do me a favor and mention the Windows header file for reference. // Taken from wtypes.h <- I think that is right?
| // VT_DATE => DateTime. | ||
| yield return new object[] | ||
| { | ||
| CreateVariant(VT_DATE, new UnionTypes { _date = 2958465.99999999 }), |
There was a problem hiding this comment.
Any way you can make a local and have the raw value computed instead of hard coding 2958465...
|
|
||
| [Guid("0000002F-0000-0000-C000-000000000046"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] | ||
| [ComImport] | ||
| public interface IRecordInfo |
There was a problem hiding this comment.
This is something we need to chat about. The entire ITypeLib interface family (IRecordInfo included) is in a weird state of support in CoreCLR. Can you please add a test ensures we fail on non-Windows platforms if APIs that use this type are called? Thanks.
cc @luqunl
There was a problem hiding this comment.
Sorry i'm slightly confused by what you mean. We have the following test already as non-Windows APIs do not implement GetObjectForNativeVariant (even though they probably could as this doesn't have any COM dependencies and the structure of ole variants is well documented, but the use case is minimal)
[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix)]
public void GetObjectForNativeVariant_Unix_ThrowsPlatformNotSupportedException()
{
Assert.Throws<PlatformNotSupportedException>(() => Marshal.GetObjectForNativeVariant(IntPtr.Zero));
Assert.Throws<PlatformNotSupportedException>(() => Marshal.GetObjectForNativeVariant<int>(IntPtr.Zero));
}I don't see any other examples of IRecordInfo being used or any APIs that take ITypeLib being exposed in .NET Core
There was a problem hiding this comment.
Hmm, looks like the tests don't pass against .NET Core - any ideas?
Unhandled Exception of Type System.ArgumentException
Message :
System.ArgumentException : The specified record cannot be mapped to a managed value class.
Stack Trace :
at System.Runtime.InteropServices.Marshal.GetObjectForNativeVariant(IntPtr pSrcNativeVariant)
at System.Runtime.InteropServices.Tests.GetObjectForNativeVariantTests.GetObjectForNativeVariant(Variant variant) in D:\j\workspace\windows-TGrou---74aa877a\src\System.Runtime.InteropServices\tests\System\Runtime\InteropServices\Marshal\GetObjectForNativeVariantTests.cs:line 994
at System.Runtime.InteropServices.Tests.GetObjectForNativeVariantTests.GetObjectForNativeVariant_Record_ReturnsExpected() in D:\j\workspace\windows-TGrou---74aa877a\src\System.Runtime.InteropServices\tests\System\Runtime\InteropServices\Marshal\GetObjectForNativeVariantTests.cs:line 699
There was a problem hiding this comment.
@hughbe It absolutely has COM dependencies since the ITypeLib concept is based entirely around Automation/OLE - which is built on top of the COM system. My general concern was defining this interface which is an Automation interface that requires ITypeLib support because that is how the record is generated - from a TLB.
There was a problem hiding this comment.
Understood - sorry if i'm misunderstanding but is there anything I should action on this?
There was a problem hiding this comment.
No, you are fine. This is all good stuff. I was more concern about testing for things that we just don't want to bring in at this point. As you pointed out, there is a test for non supported platforms so at this point I am good if you are?
There was a problem hiding this comment.
yeah there is one for non supported platforms to make sure this throws PNSE
|
@luqunl any other comments? |
|
@luqunl ping? |
| // VT_BOOL => bool. | ||
| yield return new object[] | ||
| { | ||
| CreateVariant(VT_BOOL, new UnionTypes { _i1 = 1 }), |
There was a problem hiding this comment.
For VT_BOOL, VARIANT_TRUE == -1, VARIANT_FALSE == 0. maybe We don't need "new UnionTypes { _i1 = 1 }" scenario
There was a problem hiding this comment.
I added this test for that specific reason - this is an unknown value so I was checking to see how the CLR would react to it. Makes sense to keep it IMO
| }; | ||
|
|
||
| // VT_RECORD. | ||
| yield return new object[] |
There was a problem hiding this comment.
dup? line 600 and line 607?
| Assert.Equal(d, GetObjectForNativeVariant(variant)); | ||
| } | ||
|
|
||
| public static IEnumerable<object[]> GetObjectForNativeVariant_Array_TestData() |
There was a problem hiding this comment.
Will do in a future PR - is that OK?
There was a problem hiding this comment.
Will do in a future PR - is that OK?
of course.
|
@dotnet-bot test Linux x64 Release Build |
|
@dotnet-bot test OSX x64 Debug Build |
|
@dotnet-bot test Packaging All Configurations x64 Debug Build |
|
@dotnet-bot test OSX x64 Debug Build |
|
@dotnet-bot test this please |
luqunl
left a comment
luqunl
left a comment
There was a problem hiding this comment.
Thanks a lot for adding these scenario.
7 participants
Tests all the various variant types apart from VT_ARRAY