Add basic SafeArray marshalling tests #20866

jkoritzinsky · 2018-11-07T22:35:24Z

Port over basic tests from .NET Framework for marshalling an array as a SAFEARRAY*.

jkoritzinsky · 2018-11-08T21:02:31Z

@dotnet-bot test this please.

jkoritzinsky · 2018-11-08T23:37:14Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x64 Release CoreFX Tests

AaronRobinsonMSFT · 2018-11-09T01:32:04Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+//	Start of SafeArray calls
+//--------------------------------
+
+extern "C" DLL_EXPORT BOOL STDMETHODCALLTYPE SafeArray_In(SAFEARRAY* psa)


These tests are a bit much. Instead, I would look at the COM tests for arrays and see the basic validation that is needed.

I'd actually prefer to keep the validation of all of the different components of the safe array since they're not something we have tests for anywhere else and they add a lot of complexity on top of traditional arrays.

I am fine with the validation - although in this case, you are testing windows not the CLR, but the validation is fine. My criticism is that it is in every case. That is definitely not needed and should be limited to a single case.

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

AaronRobinsonMSFT · 2018-11-10T19:05:36Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+	int* pInt;
+	HRESULT hr;
+	StructWithSA* ps = (StructWithSA*)CoreClrAlloc(sizeof(StructWithSA));
+	(*ps).i32 = 77;


Where is this value being validated?

AaronRobinsonMSFT · 2018-11-10T19:06:35Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+		printf("\t\tSafeArrayCreateVector call failed!\n");
+		exit(1);
+	}
+	else


This else isn't needed.

AaronRobinsonMSFT · 2018-11-10T19:17:00Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+	}
+
+	//check element size
+	if(SafeArrayGetElemsize(psa) != 4) //size of each element should be 4 bytes


Why? This is a perfect example of why I dislike how these tests are written. The base invariant here is that the passed in SafeArray is of INT4, but that isn't defined anywhere in this code nor validated. Additionally this could be for a float for that matter. Basically I am fine with this kind of validation, but my question to you and something we should always consider when writing tests is the following list:

What are we validating?
Why are we validating it?
If this test fails what does it indicate?
If this test fails is it obvious what other scenarios are impacted?
Is this test validating undocumented or unsupported scenarios?
Can a new person look at this test and easily determine the test purpose and value?

AaronRobinsonMSFT · 2018-11-10T19:20:09Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+{
+	SAFEARRAY * psa;
+
+	const int rank = 2;


Again, what rank is wrong? This is setting the rank to 2? Is that wrong? A nice indication of this is defining the variable as const int InvalidRank = 2; that would make it clear 2 isn't the valid value.

AaronRobinsonMSFT

Let's chat in person when I get back.

…#33413) dotnet/coreclr#20866 extends the jit to incorporate the type of readonly ref class typed static fields into jitted code when jitting happens after class initialization. For instance, the jit may optimize type tests or devirtualize calls. With the advent of type based jit optimizations we are also blocking reflection based updates to all readonly static fields after class initialization.

…ommented out.

…R or LPWSTR.

AaronRobinsonMSFT · 2018-11-17T00:37:02Z

tests/src/Interop/ArrayMarshalling/SafeArray/RecordNative.cpp

+        return --refCount;
+    }
+
+    HRESULT QueryInterface(const IID& riid, void** ppvObject)


Boo. Technically this doesn't matter, but these kind of things tend to get copy/pasted. QueryInterface() always calls AddRef() on success. Just make the call prior to returning S_OK.

Same for the one below.

AaronRobinsonMSFT · 2018-11-17T00:39:23Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+#include <xplatform.h>
+#include <oleauto.h>
+#include <algorithm>
+#include "platformdefines.h"


Is platformdefines.h in this directory? I think this should be <...>? Please fix this where ever you see it.

AaronRobinsonMSFT · 2018-11-17T00:52:45Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp

+#include "platformdefines.h"
+#include "Helpers.h"
+
+extern "C" DLL_EXPORT BOOL STDMETHODCALLTYPE XorBoolArray(SAFEARRAY* d, BOOL* result)


We are getting to the point where this model of testing is more burden than benefit.

Instead of returning a BOOL, just return the HRESULT. Tests should almost always just throw an exception or return an error code. The BOOL concept is a relic that should be purged from the codebase. HRESULTs will be properly converted to an Exception and in most cases translated. This will also allow you to follow the standard pattern of RETURN_IF_FAILED().

AaronRobinsonMSFT · 2018-11-17T00:54:17Z

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayTest.cs

+    }
+
+    [DllImport(nameof(SafeArrayNative))]
+    public static extern bool XorBoolArray(


All of the associated out params can now just be natural return values.

…dotnet#33413) dotnet/coreclr#20866 extends the jit to incorporate the type of readonly ref class typed static fields into jitted code when jitting happens after class initialization. For instance, the jit may optimize type tests or devirtualize calls. With the advent of type based jit optimizations we are also blocking reflection based updates to all readonly static fields after class initialization.

jkoritzinsky added 3 commits November 7, 2018 14:14

Port simple SafeArray tests over from .NET Framework's test suite.

be8d12d

Clean up native calling conventions.

19f8e09

Add basic field array -> field SAFEARRAY* tests.

be5a31e

jkoritzinsky requested a review from AaronRobinsonMSFT November 7, 2018 22:35

Fix some hiding stdcall calling convention decorators.

0b7c624

jkoritzinsky added area-Interop test enhancement Improvements of test source code and removed area-Interop labels Nov 7, 2018

AaronRobinsonMSFT reviewed Nov 9, 2018

View reviewed changes

tests/src/Interop/ArrayMarshalling/SafeArray/SafeArrayNative.cpp Outdated Show resolved Hide resolved

PR Feedback.

84c6fd0

AaronRobinsonMSFT reviewed Nov 10, 2018

View reviewed changes

AaronRobinsonMSFT suggested changes Nov 10, 2018

View reviewed changes

AndyAyersMS mentioned this pull request Nov 12, 2018

Remove test that sets static readonly field after type initialization dotnet/corefx#33413

Merged

jkoritzinsky added 5 commits November 15, 2018 17:49

Rewrite native part of SafeArray tests

44cc3c4

Wrote the managed side of the tests. Non-passing ones are currently c…

5fe16c0

…ommented out.

Fix up tests and enable marshalling a string[] as a SAFEARRAY of LPST…

53ce196

…R or LPWSTR.

Add blittable records test with a return parameter.

590dc13

Add returning non-blittable record safearray test.

4c41295

jkoritzinsky closed this Nov 16, 2018

AaronRobinsonMSFT reviewed Nov 17, 2018

View reviewed changes

Add basic SafeArray marshalling tests #20866

Add basic SafeArray marshalling tests #20866

Uh oh!

Conversation

jkoritzinsky commented Nov 7, 2018

Uh oh!

jkoritzinsky commented Nov 8, 2018

Uh oh!

jkoritzinsky commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants