Merge pull request #908 from khellang/clarify-failing-convention-tests

Clarify why convention tests are failing

Octokit.Tests.Conventions/Exception/InvalidDebuggerDisplayAttributeValueException.cs

@@ -0,0 +1,18 @@
+using System;
+
+namespace Octokit.Tests.Conventions
+{
+    public class InvalidDebuggerDisplayAttributeValueException : Exception
+    {
+        public InvalidDebuggerDisplayAttributeValueException(Type modelType, string value)
+            : base (CreateMessage(modelType, value)) { }
+
+        static string CreateMessage(Type modelType, string value)
+        {
+            return string.Format(
+                "Model type '{0}' has invalid DebuggerDisplayAttribute value '{1}'. Expected '{{DebuggerDisplay, nq}}'",
+                modelType.FullName,
+                value);
+        }
+    }
+}

Octokit.Tests.Conventions/Exception/InvalidDebuggerDisplayReturnType.cs

@@ -0,0 +1,18 @@
+using System;
+
+namespace Octokit.Tests.Conventions
+{
+    public class InvalidDebuggerDisplayReturnType : Exception
+    {
+        public InvalidDebuggerDisplayReturnType(Type modelType, Type propertyType)
+            : base (CreateMessage(modelType, propertyType)) { }
+
+        static string CreateMessage(Type modelType, Type propertyType)
+        {
+            return string.Format(
+                "Model type '{0}' has invalid DebuggerDisplay return type '{1}'. Expected 'string'.",
+                modelType.FullName,
+                propertyType.Name);
+        }
+    }
+}

Octokit.Tests.Conventions/Exception/MissingDebuggerDisplayAttributeException.cs

@@ -0,0 +1,10 @@
+using System;
+
+namespace Octokit.Tests.Conventions
+{
+    public class MissingDebuggerDisplayAttributeException : Exception
+    {
+        public MissingDebuggerDisplayAttributeException(Type modelType)
+            : base (string.Format("Model type '{0}' is missing the DebuggerDisplayAttribute.", modelType.FullName)) { }
+    }
+}

Octokit.Tests.Conventions/Exception/MissingDebuggerDisplayPropertyException.cs

@@ -0,0 +1,10 @@
+using System;
+
+namespace Octokit.Tests.Conventions
+{
+    public class MissingDebuggerDisplayPropertyException : Exception
+    {
+        public MissingDebuggerDisplayPropertyException(Type modelType)
+            : base (string.Format("Model type '{0}' is missing the DebuggerDisplay property.", modelType.FullName)) { }
+    }
+}

Octokit.Tests.Conventions/Exception/MutableModelPropertiesException.cs

@@ -0,0 +1,21 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+
+namespace Octokit.Tests.Conventions
+{
+    public class MutableModelPropertiesException : Exception
+    {
+        public MutableModelPropertiesException(Type modelType, IEnumerable<PropertyInfo> mutableProperties)
+            : base (CreateMessage(modelType, mutableProperties)) { }
+
+        static string CreateMessage(Type modelType, IEnumerable<PropertyInfo> mutableProperties)
+        {
+            return string.Format("Model type '{0}' contains the following mutable properties: {1}{2}",
+                modelType.FullName,
+                Environment.NewLine,
+                string.Join(Environment.NewLine, mutableProperties.Select(x => x.Name)));
+        }
+    }
+}

Octokit.Tests.Conventions/ModelTests.cs

@@ -2,7 +2,6 @@
 using System.Collections;
 using System.Diagnostics;
 using System.Linq;
-using Octokit.Tests.Helpers;
 using Xunit;
 using System.Collections.Generic;
 using System.Reflection;
@@ -15,25 +14,50 @@ namespace Octokit.Tests.Conventions
         [MemberData("ModelTypes")]
         public void AllModelsHaveDebuggerDisplayAttribute(Type modelType)
         {
-            var attribute = AssertEx.HasAttribute<DebuggerDisplayAttribute>(modelType);
+            var attribute = modelType.GetCustomAttribute<DebuggerDisplayAttribute>(inherit: false);
+            if (attribute == null)
+            {
+                throw new MissingDebuggerDisplayAttributeException(modelType);
+            }
 
-            Assert.Equal("{DebuggerDisplay,nq}", attribute.Value);
+            if (attribute.Value != "{DebuggerDisplay,nq}")
+            {
+                throw new InvalidDebuggerDisplayAttributeValueException(modelType, attribute.Value);
+            }
 
             var property = modelType.GetProperty("DebuggerDisplay", BindingFlags.Instance | BindingFlags.NonPublic);
+            if (property == null)
+            {
+                throw new MissingDebuggerDisplayPropertyException(modelType);
+            }
 
-            Assert.NotNull(property);
-            Assert.Equal(typeof(string), property.PropertyType);
+            if (property.PropertyType != typeof(string))
+            {
+                throw new InvalidDebuggerDisplayReturnType(modelType, property.PropertyType);
+            }
         }
 
         [Theory]
         [MemberData("ResponseModelTypes")]
         public void ResponseModelsHaveGetterOnlyProperties(Type modelType)
         {
+            var mutableProperties = new List<PropertyInfo>();
+
             foreach (var property in modelType.GetProperties())
             {
                 var setter = property.GetSetMethod(nonPublic: true);
 
-                Assert.True(setter == null || !setter.IsPublic);
+                if (setter == null || !setter.IsPublic)
+                {
+                    continue;
+                }
+
+                mutableProperties.Add(property);
+            }
+
+            if (mutableProperties.Any())
+            {
+                throw new MutableModelPropertiesException(modelType, mutableProperties);
             }
         }
 
@@ -41,6 +65,8 @@ namespace Octokit.Tests.Conventions
         [MemberData("ResponseModelTypes")]
         public void ResponseModelsHaveReadOnlyCollections(Type modelType)
         {
+            var mutableCollectionProperties = new List<PropertyInfo>();
+
             foreach (var property in modelType.GetProperties())
             {
                 var propertyType = property.PropertyType;
@@ -54,9 +80,19 @@ namespace Octokit.Tests.Conventions
                         continue;
                     }
 
-                    AssertEx.IsReadOnlyCollection(propertyType);
+                    if (propertyType.IsReadOnlyCollection())
+                    {
+                        continue;
+                    }
+
+                    mutableCollectionProperties.Add(property);
                 }
             }
+
+            if (mutableCollectionProperties.Any())
+            {
+                throw new MutableModelPropertiesException(modelType, mutableCollectionProperties);
+            }
         }
 
         //TODO: This should (probably) be moved to the PaginationTests class that is being introduced in PR #760

Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj

@@ -58,6 +58,11 @@
     </Reference>
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="Exception\InvalidDebuggerDisplayAttributeValueException.cs" />
+    <Compile Include="Exception\InvalidDebuggerDisplayReturnType.cs" />
+    <Compile Include="Exception\MissingDebuggerDisplayAttributeException.cs" />
+    <Compile Include="Exception\MissingDebuggerDisplayPropertyException.cs" />
+    <Compile Include="Exception\MutableModelPropertiesException.cs" />
     <Compile Include="Exception\PaginationGetAllMethodNameMismatchException.cs" />
     <Compile Include="ModelTests.cs" />
     <Compile Include="Exception\InterfaceHasAdditionalMethodsException.cs" />

Octokit.Tests.Conventions/TypeExtensions.cs

@@ -99,6 +99,20 @@ namespace Octokit.Tests.Conventions
         {
             return type.GetGenericArguments()[0];
         }
+
+        public static bool IsReadOnlyCollection(this Type type)
+        {
+            var isReadOnlyList = type.HasGenericTypeDefinition(typeof(IReadOnlyList<>));
+
+            var isReadOnlyDictionary = type.HasGenericTypeDefinition(typeof(IReadOnlyDictionary<,>));
+
+            return isReadOnlyList || isReadOnlyDictionary;
+        }
+
+        private static bool HasGenericTypeDefinition(this Type type, Type genericTypeDefinition)
+        {
+            return type.IsGenericType && type.GetGenericTypeDefinition() == genericTypeDefinition;
+        }
     }
 
     public enum TypeCategory { Other, Task, GenericTask, ReadOnlyList, ClientInterface }

Octokit.Tests/Helpers/AssertEx.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Reflection;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -14,15 +13,6 @@ namespace Octokit.Tests.Helpers
             assert();
         }
 
-        public static TAttribute HasAttribute<TAttribute>(MemberInfo memberInfo, bool inherit = false) where TAttribute : Attribute
-        {
-            var attribute = memberInfo.GetCustomAttribute<TAttribute>(inherit);
-
-            Assert.NotNull(attribute);
-
-            return attribute;
-        }
-
         static readonly string[] whitespaceArguments = { " ", "\t", "\n", "\n\r", "  " };
 
         public static async Task ThrowsWhenGivenWhitespaceArgument(Func<string, Task> action)
@@ -39,14 +29,5 @@ namespace Octokit.Tests.Helpers
             // The collection == null case is for .NET 4.0
             Assert.True(instance is IReadOnlyList<T> && (collection == null || collection.IsReadOnly));
         }
-
-        public static void IsReadOnlyCollection(Type type)
-        {
-            var isReadOnlyList = type.IsGenericType && type.GetGenericTypeDefinition() == typeof(IReadOnlyList<>);
-
-            var isReadOnlyDictionary = type.IsGenericType && type.GetGenericTypeDefinition() == typeof(IReadOnlyDictionary<,>);
-
-            Assert.True(isReadOnlyList || isReadOnlyDictionary);
-        }
     }
 }