Add tests for S.Reflection types #11981

alexperovich · 2016-09-22T18:14:20Z

This change adds tests for the following System.Reflection types:

Module
Pointer
RuntimeReflectionExtensions
TypeDelegator
TypeInfo

Fixes #11766

This change adds tests for the following System.Reflection types: - Module - Pointer - RuntimeReflectionExtensions - TypeDelegator - TypeInfo Fixes #11766

alexperovich · 2016-09-22T18:18:42Z

@davidwrighton PTAL

davidwrighton · 2016-09-22T18:22:45Z

src/System.Runtime/tests/System/Reflection/PointerTests.cs

+            PropertyInfo property = typeof(PointerHolder).GetProperty("Property");
+            property.SetValue(obj, Pointer.Box((void*)value, typeof(char*)));
+            Assert.Equal(value, (int)obj.Property);
+        }


Please test that passing the parameter in as a boxed IntPtr does something. (I believe that's also supposed to work)

davidwrighton

I've reviewed the Pointer portions of these changes and they look good to me.

alexperovich · 2016-09-26T21:11:06Z

@weshaggard @atsushikan

ghost · 2016-09-26T21:21:22Z

corefx guideline: "var" can only be used in one of these:

var x = new Something()
var x = (Something)expr
var x = expr as Something

ghost · 2016-09-26T21:24:38Z

src/System.Private.Uri/src/System.Private.Uri.csproj

@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


Looks like VS bumped a bunch of .csproj - please remove these from the commit.

ghost · 2016-09-26T21:26:33Z

src/System.Runtime/tests/System/Reflection/TypeInfoTests.cs

+
+namespace System.Reflection.Tests
+{
+    public class TestType : Stream


Any particular reason why TestType derives from an api type?

I'm not sure where Stream stands in the api bringup effort but if we restore new members to that have to be overridden, you'll have to update this class and the various tests that depend on it.

I wanted something with a large number of methods. I will change this to a custom type.

ghost · 2016-09-26T21:28:11Z

src/System.Runtime/tests/System/Reflection/TypeInfoTests.cs

+        {
+            var fields = TypeInfo.DeclaredFields.ToList();
+            Assert.Equal(2, fields.Count);
+            Assert.Equal("StuffHappened", fields[0].Name);


You're taking a dependency on the order in DeclaredFields returns fields - there's no guarantee of order.

This applies to all the Type apis that enumerate members on types. It's probably better to do an order-agnostic set compare.

I will change this.

ghost · 2016-09-26T21:42:13Z

src/System.Runtime/tests/System/Reflection/ModuleTests.cs

+            {
+                Module.GetField(null);
+            });
+            Assert.Null(ex.InnerException);


I think this may be going overboard. Checking that it throws ArgumentNullException should be sufficient.

ghost · 2016-09-26T21:44:09Z

src/System.Runtime/tests/System/Reflection/ModuleTests.cs

+            List<CustomAttributeData> customAttributes = Module.CustomAttributes.ToList();
+            Assert.Equal(2, customAttributes.Count);
+            var fooAttribute = customAttributes[0];
+            Assert.Equal(typeof(FooAttribute), fooAttribute.AttributeType);


Nothing to preview

Another ordering dependency

ghost · 2016-09-27T19:37:57Z

src/System.Runtime/tests/System/Reflection/TypeInfoTests.cs

+
+    public class TypeInfoTests
+    {
+        public TypeInfo TypeInfo => typeof(TestType).GetTypeInfo();


Naming this property "TypeInfo" is confusing. Might be preferable just to inline so that people can more easily read the tests in isolation.

Ill rename it.

alexperovich · 2016-09-28T20:11:29Z

@dotnet-bot test Innerloop CentOS7.1 Debug Build and Test please (Jenkins Client issue)

ghost · 2016-09-28T20:20:38Z

src/System.Runtime/tests/System/Reflection/ModuleTests.cs

+        [Fact]
+        public void GetFields()
+        {
+            List<FieldInfo> fields = TestModule.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static).ToList();


Needs to be sorted by name for test to work reliably.

I'll add that

ghost · 2016-09-29T20:33:46Z

LGTM.

alexperovich · 2016-09-29T22:32:54Z

@dotnet-bot test Innerloop CentOS7.1 Release Build and Test please (Build Timeout)

* Add tests for S.Reflection types This change adds tests for the following System.Reflection types: - Module - Pointer - RuntimeReflectionExtensions - TypeDelegator - TypeInfo Fixes dotnet/corefx#11766 * Use checked in IL binary. * Add tests passing IntPtr to reflection apis * Ignore case in Module.Name check. * Ignore ordering in GetRuntimeFields() * Fixup Project Files * Remove resharper file that found its way in. * Remove some ordering dependencies * s/var/{type} * Rename TypeInfo property to TestTypeInfo * Sort fields by name for consistency Commit migrated from dotnet/corefx@f6f97c2

Add tests for S.Reflection types

a3edb6f

This change adds tests for the following System.Reflection types: - Module - Pointer - RuntimeReflectionExtensions - TypeDelegator - TypeInfo Fixes #11766

dnfclas added the cla-already-signed label Sep 22, 2016

davidwrighton reviewed Sep 22, 2016

View reviewed changes

davidwrighton approved these changes Sep 22, 2016

View reviewed changes

alexperovich added 3 commits September 23, 2016 14:09

Merge 'master'

9cbeefc

Use checked in IL binary.

000c0ac

Add tests passing IntPtr to reflection apis

c5a1a96

karelz added area-System.Reflection test enhancement Improvements of test source code labels Sep 26, 2016

alexperovich added 3 commits September 26, 2016 10:56

Merge branch 'master' into reflectionTests

5c2265b

Ignore case in Module.Name check.

613ee92

Ignore ordering in GetRuntimeFields()

f7ad0a7

ghost reviewed Sep 26, 2016

View reviewed changes

alexperovich added 5 commits September 26, 2016 17:45

Merge branch 'master' into reflectionTests

ed782ad

Fixup Project Files

0d672c8

Remove resharper file that found its way in.

418a89c

Remove some ordering dependencies

7cfa569

s/var/{type}

ba4cdb0

ghost reviewed Sep 27, 2016

View reviewed changes

Rename TypeInfo property to TestTypeInfo

32aaa77

ghost reviewed Sep 28, 2016

View reviewed changes

alexperovich added 2 commits September 29, 2016 13:09

Merge branch 'master' into reflectionTests

d28e2ad

Sort fields by name for consistency

cf1b7dc

alexperovich merged commit f6f97c2 into dotnet:master Sep 29, 2016

alexperovich deleted the reflectionTests branch September 29, 2016 23:26

karelz modified the milestone: 1.2.0 Dec 3, 2016

alexperovich mentioned this pull request Jan 31, 2020

The serialization constructor for System.Reflection.Pointer cannot be found. dotnet/runtime#18671

Closed

Add tests for S.Reflection types #11981

Add tests for S.Reflection types #11981

Uh oh!

Conversation

alexperovich commented Sep 22, 2016

Uh oh!

alexperovich commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Sep 26, 2016

Uh oh!

ghost commented Sep 26, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

alexperovich commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 29, 2016

Uh oh!

alexperovich commented Sep 29, 2016

Uh oh!

Uh oh!