Apply TestCategory from derived class on inherited test methods (#513)

* Apply TestCategory from derived class on inherited test methods

Enables sharing test methods across a class hierarchy while categorizing
the inherited tests based on the derived class that's actually running
them.

* Preserve DeclaringClassFullName from discovery to execution

* Restore GetCustomAttributeForAssembly taking in a MemberInfo, but reference the member's Module instead of DeclaringType

* Filter out duplicate test methods

Selects the one declared closest to the test class among all valid test
methods with the same name.

* Fix compile after latest merge from master

Author: spanglerco

src/Adapter/MSTest.CoreAdapter/Constants.cs

@@ -40,6 +40,8 @@ internal static class Constants
 
         internal static readonly TestProperty TestClassNameProperty = TestProperty.Register("MSTestDiscoverer.TestClassName", TestClassNameLabel, typeof(string), TestPropertyAttributes.Hidden, typeof(TestCase));
 
+        internal static readonly TestProperty DeclaringClassNameProperty = TestProperty.Register("MSTestDiscoverer.DeclaringClassName", DeclaringClassNameLabel, typeof(string), TestPropertyAttributes.Hidden, typeof(TestCase));
+
         internal static readonly TestProperty AsyncTestProperty = TestProperty.Register("MSTestDiscoverer.IsAsync", IsAsyncLabel, typeof(bool), TestPropertyAttributes.Hidden, typeof(TestCase));
 
 #pragma warning disable CS0618 // Type or member is obsolete
@@ -98,6 +100,7 @@ internal static class Constants
         /// These Property names should not be localized.
         /// </summary>
         private const string TestClassNameLabel = "ClassName";
+        private const string DeclaringClassNameLabel = "DeclaringClassName";
         private const string IsAsyncLabel = "IsAsync";
         private const string TestCategoryLabel = "TestCategory";
         private const string PriorityLabel = "Priority";

src/Adapter/MSTest.CoreAdapter/Discovery/TypeEnumerator.cs

@@ -70,6 +70,8 @@ internal virtual ICollection<UnitTestElement> Enumerate(out ICollection<string>
         /// <returns> List of Valid Tests. </returns>
         internal Collection<UnitTestElement> GetTests(ICollection<string> warnings)
         {
+            bool foundDuplicateTests = false;
+            var foundTests = new HashSet<string>();
             var tests = new Collection<UnitTestElement>();
 
             // Test class is already valid. Verify methods.
@@ -85,11 +87,35 @@ internal Collection<UnitTestElement> GetTests(ICollection<string> warnings)
 
                 if (this.testMethodValidator.IsValidTestMethod(method, this.type, warnings))
                 {
+                    foundDuplicateTests = foundDuplicateTests || !foundTests.Add(method.Name);
                     tests.Add(this.GetTestFromMethod(method, isMethodDeclaredInTestTypeAssembly, warnings));
                 }
             }
 
-            return tests;
+            if (!foundDuplicateTests)
+            {
+                return tests;
+            }
+
+            // Remove duplicate test methods by taking the first one of each name
+            // that is declared closest to the test class in the hierarchy.
+            var inheritanceDepths = new Dictionary<string, int>();
+            var currentType = this.type;
+            int currentDepth = 0;
+
+            while (currentType != null)
+            {
+                inheritanceDepths[currentType.FullName] = currentDepth;
+                ++currentDepth;
+                currentType = currentType.GetTypeInfo().BaseType;
+            }
+
+            return new Collection<UnitTestElement>(
+                tests.GroupBy(
+                    t => t.TestMethod.Name,
+                    (_, elements) =>
+                        elements.OrderBy(t => inheritanceDepths[t.TestMethod.DeclaringClassFullName ?? t.TestMethod.FullClassName]).First())
+                    .ToList());
         }
 
         /// <summary>
@@ -126,9 +152,9 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT
             var asyncTypeName = method.GetAsyncTypeName();
             testElement.AsyncTypeName = asyncTypeName;
 
-            testElement.TestCategory = this.reflectHelper.GetCategories(method);
+            testElement.TestCategory = this.reflectHelper.GetCategories(method, this.type);
 
-            testElement.DoNotParallelize = this.reflectHelper.IsDoNotParallelizeSet(method);
+            testElement.DoNotParallelize = this.reflectHelper.IsDoNotParallelizeSet(method, this.type);
 
             var traits = this.reflectHelper.GetTestPropertiesAsTraits(method);
 

src/Adapter/MSTest.CoreAdapter/Execution/TypeCache.cs

@@ -606,10 +606,26 @@ private TestMethodAttribute GetTestMethodAttribute(MethodInfo methodInfo, TestCl
         private MethodInfo GetMethodInfoForTestMethod(TestMethod testMethod, TestClassInfo testClassInfo)
         {
             var methodsInClass = testClassInfo.ClassType.GetRuntimeMethods().ToArray();
+            MethodInfo testMethodInfo;
 
-            var testMethodInfo =
-                methodsInClass.Where(method => method.Name.Equals(testMethod.Name))
-                    .FirstOrDefault(method => method.HasCorrectTestMethodSignature(true));
+            if (testMethod.DeclaringClassFullName != null)
+            {
+                // Only find methods that match the given declaring name.
+                testMethodInfo =
+                    methodsInClass.Where(method => method.Name.Equals(testMethod.Name)
+                                                && method.DeclaringType.FullName.Equals(testMethod.DeclaringClassFullName)
+                                                && method.HasCorrectTestMethodSignature(true)).FirstOrDefault();
+            }
+            else
+            {
+                // Either the declaring class is the same as the test class, or
+                // the declaring class information wasn't passed in the test case.
+                // Prioritize the former while maintaining previous behavior for the latter.
+                var className = testClassInfo.ClassType.FullName;
+                testMethodInfo =
+                    methodsInClass.Where(method => method.Name.Equals(testMethod.Name) && method.HasCorrectTestMethodSignature(true))
+                        .OrderByDescending(method => method.DeclaringType.FullName.Equals(className)).FirstOrDefault();
+            }
 
             // if correct method is not found, throw appropriate
             // exception about what is wrong.

src/Adapter/MSTest.CoreAdapter/Extensions/TestCaseExtensions.cs

@@ -22,9 +22,15 @@ internal static UnitTestElement ToUnitTestElement(this TestCase testCase, string
         {
             var isAsync = (testCase.GetPropertyValue(Constants.AsyncTestProperty) as bool?) ?? false;
             var testClassName = testCase.GetPropertyValue(Constants.TestClassNameProperty) as string;
+            var declaringClassName = testCase.GetPropertyValue(Constants.DeclaringClassNameProperty) as string;
 
             TestMethod testMethod = new TestMethod(testCase.DisplayName, testClassName, source, isAsync);
 
+            if (declaringClassName != null && declaringClassName != testClassName)
+            {
+                testMethod.DeclaringClassFullName = declaringClassName;
+            }
+
             UnitTestElement testElement = new UnitTestElement(testMethod)
                                         {
                                             IsAsync = isAsync,

src/Adapter/MSTest.CoreAdapter/Helpers/ReflectHelper.cs

@@ -315,10 +315,11 @@ internal virtual bool IsMethodDeclaredInSameAssemblyAsType(MethodInfo method, Ty
         /// Get categories applied to the test method
         /// </summary>
         /// <param name="categoryAttributeProvider">The member to inspect.</param>
+        /// <param name="owningType">The reflected type that owns <paramref name="categoryAttributeProvider"/>.</param>
         /// <returns>Categories defined.</returns>
-        internal virtual string[] GetCategories(MemberInfo categoryAttributeProvider)
+        internal virtual string[] GetCategories(MemberInfo categoryAttributeProvider, Type owningType)
         {
-            var categories = this.GetCustomAttributesRecursively(categoryAttributeProvider, typeof(TestCategoryBaseAttribute));
+            var categories = this.GetCustomAttributesRecursively(categoryAttributeProvider, owningType, typeof(TestCategoryBaseAttribute));
             List<string> testCategories = new List<string>();
 
             if (categories != null)
@@ -346,11 +347,12 @@ internal ParallelizeAttribute GetParallelizeAttribute(Assembly assembly)
         /// Get the parallelization behavior for a test method.
         /// </summary>
         /// <param name="testMethod">Test method.</param>
+        /// <param name="owningType">The type that owns <paramref name="testMethod"/>.</param>
         /// <returns>True if test method should not run in parallel.</returns>
-        internal bool IsDoNotParallelizeSet(MemberInfo testMethod)
+        internal bool IsDoNotParallelizeSet(MemberInfo testMethod, Type owningType)
         {
             return this.GetCustomAttributes(testMethod, typeof(DoNotParallelizeAttribute)).Any()
-                   || this.GetCustomAttributes(testMethod.DeclaringType.GetTypeInfo(), typeof(DoNotParallelizeAttribute)).Any();
+                   || this.GetCustomAttributes(owningType.GetTypeInfo(), typeof(DoNotParallelizeAttribute)).Any();
         }
 
         /// <summary>
@@ -367,19 +369,20 @@ internal bool IsDoNotParallelizeSet(Assembly assembly)
         /// Gets custom attributes at the class and assembly for a method.
         /// </summary>
         /// <param name="attributeProvider">Method Info or Member Info or a Type</param>
+        /// <param name="owningType">The type that owns <paramref name="attributeProvider"/>.</param>
         /// <param name="type"> What type of CustomAttribute you need. For instance: TestCategory, Owner etc.,</param>
         /// <returns>The categories of the specified type on the method. </returns>
-        internal IEnumerable<object> GetCustomAttributesRecursively(MemberInfo attributeProvider, Type type)
+        internal IEnumerable<object> GetCustomAttributesRecursively(MemberInfo attributeProvider, Type owningType, Type type)
         {
             var categories = this.GetCustomAttributes(attributeProvider, typeof(TestCategoryBaseAttribute));
             if (categories != null)
             {
-                categories = categories.Concat(this.GetCustomAttributes(attributeProvider.DeclaringType.GetTypeInfo(), typeof(TestCategoryBaseAttribute))).ToArray();
+                categories = categories.Concat(this.GetCustomAttributes(owningType.GetTypeInfo(), typeof(TestCategoryBaseAttribute))).ToArray();
             }
 
             if (categories != null)
             {
-                categories = categories.Concat(this.GetCustomAttributeForAssembly(attributeProvider, typeof(TestCategoryBaseAttribute))).ToArray();
+                categories = categories.Concat(this.GetCustomAttributeForAssembly(owningType.GetTypeInfo(), typeof(TestCategoryBaseAttribute))).ToArray();
             }
 
             if (categories != null)
@@ -401,8 +404,7 @@ internal virtual Attribute[] GetCustomAttributeForAssembly(MemberInfo memberInfo
         {
             return
                 PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(
-                    memberInfo.DeclaringType.GetTypeInfo().Assembly,
-                    type).OfType<Attribute>().ToArray();
+                    memberInfo.Module.Assembly, type).OfType<Attribute>().ToArray();
         }
 
         /// <summary>

src/Adapter/MSTest.CoreAdapter/ObjectModel/TestMethod.cs

@@ -74,7 +74,8 @@ public string DeclaringAssemblyName
         }
 
         /// <summary>
-        /// Gets or sets the declaring class full name. This will be used while getting navigation data.
+        /// Gets or sets the declaring class full name.
+        /// This will be used to resolve overloads and while getting navigation data.
         /// This will be null if FullClassName is same as DeclaringClassFullName.
         /// Reason to set to null in the above case is to minimise the transfer of data across appdomains and not have a perf hit.
         /// </summary>

src/Adapter/MSTest.CoreAdapter/ObjectModel/UnitTestElement.cs

@@ -114,6 +114,12 @@ internal TestCase ToTestCase()
 
             testCase.SetPropertyValue(TestAdapter.Constants.TestClassNameProperty, this.TestMethod.FullClassName);
 
+            // Set declaring type if present so the correct method info can be retrieved
+            if (this.TestMethod.DeclaringClassFullName != null)
+            {
+                testCase.SetPropertyValue(TestAdapter.Constants.DeclaringClassNameProperty, this.TestMethod.DeclaringClassFullName);
+            }
+
             // Many of the tests will not be async, so there is no point in sending extra data
             if (this.IsAsync)
             {

src/Adapter/PlatformServices.Interface/ObjectModel/ITestMethod.cs

@@ -19,7 +19,8 @@ public interface ITestMethod
         string FullClassName { get; }
 
         /// <summary>
-        /// Gets the declaring class full name. This will be used while getting navigation data.
+        /// Gets the declaring class full name.
+        /// This will be used for resolving overloads and while getting navigation data.
         /// </summary>
         string DeclaringClassFullName { get; }
 

test/UnitTests/MSTest.CoreAdapter.Unit.Tests/Discovery/TypeEnumeratorTests.cs

@@ -183,6 +183,82 @@ public void GetTestsShouldNotReturnBaseTestMethodsFromAnotherAssemblyByConfigura
                 "DummyDerivedFromRemoteTestClass inherits DummyRemoteBaseTestClass from different assembly. BestTestMethod from DummyRemoteBaseTestClass should not be discovered when RunSettings MSTestV2 specifies EnableBaseClassTestMethodsFromOtherAssemblies = false.");
         }
 
+        [TestMethod]
+        public void GetTestsShouldNotReturnHiddenTestMethods()
+        {
+            this.SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true);
+            TypeEnumerator typeEnumerator = this.GetTypeEnumeratorInstance(typeof(DummyHidingTestClass), Assembly.GetExecutingAssembly().FullName);
+
+            var tests = typeEnumerator.Enumerate(out this.warnings);
+
+            Assert.IsNotNull(tests);
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "BaseTestMethod"),
+                "DummyHidingTestClass declares BaseTestMethod directly so it should always be discovered.");
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "DerivedTestMethod"),
+                "DummyHidingTestClass declares BaseTestMethod directly so it should always be discovered.");
+            Assert.IsFalse(
+                tests.Any(t => t.TestMethod.DeclaringClassFullName == typeof(DummyBaseTestClass).FullName),
+                "DummyHidingTestClass hides BaseTestMethod so declaring class should not be the base class");
+        }
+
+        [TestMethod]
+        public void GetTestsShouldReturnOverriddenTestMethods()
+        {
+            this.SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true);
+            TypeEnumerator typeEnumerator = this.GetTypeEnumeratorInstance(typeof(DummyOverridingTestClass), Assembly.GetExecutingAssembly().FullName);
+
+            var tests = typeEnumerator.Enumerate(out this.warnings);
+
+            Assert.IsNotNull(tests);
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "BaseTestMethod"),
+                "DummyOverridingTestClass inherits BaseTestMethod so it should be discovered.");
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "DerivedTestMethod"),
+                "DummyOverridingTestClass overrides DerivedTestMethod directly so it should always be discovered.");
+            Assert.AreEqual(
+                typeof(DummyHidingTestClass).FullName,
+                tests.Single(t => t.TestMethod.Name == "BaseTestMethod").TestMethod.DeclaringClassFullName,
+                "DummyOverridingTestClass inherits BaseTestMethod from DummyHidingTestClass specifically.");
+            Assert.IsNull(
+                tests.Single(t => t.TestMethod.Name == "DerivedTestMethod").TestMethod.DeclaringClassFullName,
+                "DummyOverridingTestClass overrides DerivedTestMethod so is the declaring class.");
+        }
+
+        [TestMethod]
+        public void GetTestsShouldNotReturnHiddenTestMethodsFromAnyLevel()
+        {
+            this.SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true);
+            TypeEnumerator typeEnumerator = this.GetTypeEnumeratorInstance(typeof(DummySecondHidingTestClass), Assembly.GetExecutingAssembly().FullName);
+
+            var tests = typeEnumerator.Enumerate(out this.warnings);
+
+            Assert.IsNotNull(tests);
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "BaseTestMethod"),
+                "DummySecondHidingTestClass hides BaseTestMethod so it should be discovered.");
+            Assert.AreEqual(
+                1,
+                tests.Count(t => t.TestMethod.Name == "DerivedTestMethod"),
+                "DummySecondHidingTestClass hides DerivedTestMethod so it should be discovered.");
+            Assert.IsFalse(
+                tests.Any(t => t.TestMethod.DeclaringClassFullName == typeof(DummyBaseTestClass).FullName),
+                "DummySecondHidingTestClass hides all base test methods so declaring class should not be any base class");
+            Assert.IsFalse(
+                tests.Any(t => t.TestMethod.DeclaringClassFullName == typeof(DummyHidingTestClass).FullName),
+                "DummySecondHidingTestClass hides all base test methods so declaring class should not be any base class");
+            Assert.IsFalse(
+                tests.Any(t => t.TestMethod.DeclaringClassFullName == typeof(DummyOverridingTestClass).FullName),
+                "DummySecondHidingTestClass hides all base test methods so declaring class should not be any base class");
+        }
+
         #endregion
 
         #region GetTestFromMethod tests
@@ -247,7 +323,7 @@ public void GetTestFromMethodShouldSetTestCategory()
             var testCategories = new string[] { "foo", "bar" };
 
             // Setup mocks
-            this.mockReflectHelper.Setup(rh => rh.GetCategories(methodInfo)).Returns(testCategories);
+            this.mockReflectHelper.Setup(rh => rh.GetCategories(methodInfo, typeof(DummyTestClass))).Returns(testCategories);
 
             var testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, this.warnings);
 
@@ -496,5 +572,34 @@ public void DerivedTestMethod()
         }
     }
 
+    public class DummyHidingTestClass : DummyBaseTestClass
+    {
+        public new virtual void BaseTestMethod()
+        {
+        }
+
+        public virtual void DerivedTestMethod()
+        {
+        }
+    }
+
+    public class DummyOverridingTestClass : DummyHidingTestClass
+    {
+        public override void DerivedTestMethod()
+        {
+        }
+    }
+
+    public class DummySecondHidingTestClass : DummyOverridingTestClass
+    {
+        public new void BaseTestMethod()
+        {
+        }
+
+        public new void DerivedTestMethod()
+        {
+        }
+    }
+
     #endregion
 }