Abstract LibGit2Sharp away in testing context - The Sequel

[nulltoken]

Similarly to #185, make remaining exposed types mockable (protected ctor and virtual methods).

[nulltoken]

In order to make sure not to forget anything, it might be worth adding a meta test to the suite. This test would inspect every types in the LibGit2Sharp namespace, make sure they bear a protected constructor and that their public/protected members are overriddable.

/cc @yorah

[yorah]

Here is a first draft of the meta test.

[nulltoken]

I like this. Very much!.

Below some tiny proposals for fixes

diff --git a/LibGit2Sharp.Tests/MetaFixture.cs b/LibGit2Sharp.Tests/MetaFixture.cs
index 6a2b234..000688b 100644
--- a/LibGit2Sharp.Tests/MetaFixture.cs
+++ b/LibGit2Sharp.Tests/MetaFixture.cs
@@ -21,10 +21,11 @@ public void TypesInLibGit2SharpMustBeExtensibleInATestingContext()

             foreach (Type type in libGit2SharpTypes)
             {
-                if (type.IsInterface)
+                if (type.IsInterface || type.IsEnum || IsStatic(type))
                     continue;

-                ConstructorInfo[] publicConstructor = type.GetConstructors(BindingFlags.Public);
+                ConstructorInfo[] publicConstructor = type.GetConstructors(BindingFlags.Public | BindingFlags.Instance);
+
                 if (publicConstructor.Any())
                 {
                     continue;
@@ -43,15 +44,13 @@ public void TypesInLibGit2SharpMustBeExtensibleInATestingContext()
                 }
             }

-            if (!nonTestableTypes.Any())
+            if (nonTestableTypes.Any())
             {
-                Assert.True(true);
+                Assert.True(false, Environment.NewLine + BuildNonTestableTypesMessage(nonTestableTypes));
             }
-
-            Assert.True(false, Environment.NewLine + BuildNonTestableTypesMessage(nonTestableTypes));
         }

-        private string BuildNonTestableTypesMessage(Dictionary<Type, IEnumerable<string>> nonTestableTypes)
+        private static string BuildNonTestableTypesMessage(Dictionary<Type, IEnumerable<string>> nonTestableTypes)
         {
             var sb = new StringBuilder();

@@ -69,11 +68,11 @@ private string BuildNonTestableTypesMessage(Dictionary<Type, IEnumerable<string>
             return sb.ToString();
         }

-        private IEnumerable<string> GetNonVirtualPublicMethodsNames(Type type)
+        private static IEnumerable<string> GetNonVirtualPublicMethodsNames(Type type)
         {
             var publicMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly);

-            return from mi in publicMethods where !mi.IsVirtual select mi.Name;
+            return from mi in publicMethods where !mi.IsVirtual && !mi.IsStatic select mi.Name;
         }

         private static bool HasEmptyProtectedConstructor(Type type)
@@ -82,5 +81,10 @@ private static bool HasEmptyProtectedConstructor(Type type)

             return nonPublicConstructors.Any(ci => !ci.IsPrivate && !ci.IsAssembly && !ci.IsFinal && !ci.GetParameters().Any());
         }
+
+        public bool IsStatic(Type type)
+        {
+            return type.IsAbstract && type.IsSealed;
+        }
     }
 }

[yorah]

Updated the MetaTest with your fixes!

Question => the first lines are:

'LibGit2Sharp.TreeEntryDefinition' cannot be easily abstracted in a testing context. Please make sure it either has a public constructor, or an empty protected constructor.
    - Method 'Boolean Equals(LibGit2Sharp.TreeEntryDefinition)' must be virtual

My fix for this one would be to simply refactor the way Equalsis implemented on this class, as you can see on b5c1e1029e60a9edf3fc5b75308f9461a5a9bfaf. Do you agree with this?

[nulltoken]

From what I read TreeEntryDefinition would rather lack a IEquatable<TreeEntryDefinition> declaration.
After a quick search, it appears that Note would also benefit from this declaration.

Could you add those in a separate "Fix missing IEquatable declarations" commit?

[nulltoken]

Fixed by #187.

@yorah ❤️