Skip to content

Apply TestCategory from derived class on inherited test methods #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 9, 2019
Merged

Apply TestCategory from derived class on inherited test methods #513

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cf164f8
Apply TestCategory from derived class on inherited test methods
spanglerco Oct 11, 2018
d028c40
Merge branch 'master' into category-from-derived-class
spanglerco Nov 7, 2018
90892d4
Merge branch 'master' into category-from-derived-class
spanglerco Nov 27, 2018
160eb6f
Preserve DeclaringClassFullName from discovery to execution
spanglerco Nov 30, 2018
d587fa8
Restore GetCustomAttributeForAssembly taking in a MemberInfo, but ref…
spanglerco Nov 30, 2018
f3f9267
Merge branch 'master' into category-from-derived-class
singhsarab Dec 5, 2018
92459ad
Merge branch 'master' into category-from-derived-class
jayaranigarg Dec 10, 2018
e56e4d0
Merge branch 'master' into category-from-derived-class
spanglerco Dec 13, 2018
67658d1
Filter out duplicate test methods
spanglerco Dec 13, 2018
f77b7c9
Merge branch 'master' into category-from-derived-class
jayaranigarg Jan 10, 2019
a09b144
Fix compile after latest merge from master
spanglerco Jan 11, 2019
7f0f8d5
Merge branch 'master' into category-from-derived-class
spanglerco Jan 14, 2019
16854dc
Merge branch 'master' into category-from-derived-class
jayaranigarg Jan 25, 2019
0cca8df
Merge branch 'master' into category-from-derived-class
jayaranigarg Apr 8, 2019
30ef7fa
Merge branch 'master' into category-from-derived-class
jayaranigarg Apr 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Adapter/MSTest.CoreAdapter/Discovery/TypeEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,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);

Expand Down
26 changes: 14 additions & 12 deletions src/Adapter/MSTest.CoreAdapter/Helpers/ReflectHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,10 +313,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)
Expand Down Expand Up @@ -344,11 +345,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>
Expand All @@ -365,19 +367,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().Assembly, typeof(TestCategoryBaseAttribute))).ToArray();
Copy link
Contributor

@karanjitsingh karanjitsingh Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.microsoft.com/en-us/dotnet/api/system.reflection.memberinfo.declaringtype?view=netframework-4.7.2#System_Reflection_MemberInfo_DeclaringType

It seems the situation you were facing was only because the test method wasn't overridden. From the 4 scenarios in the link, one of the scenarios is this issues particular scenario. Just to be sure can you provide the behavior for the rest of the three scenarios.

I know this seems far fetched, but this could lead to a big breaking change so I just want to make sure :)

Copy link
Contributor Author

@spanglerco spanglerco Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I took the example class hierarchy from that link and added test attributes to get this: https://gist.github.com/spanglerco/fdb04b4a6bab1be2963f6063c937a587

It actually exposes an interesting corner case for classes that hide test methods (not that I can immediately think of a use case for that). Here's the result of using vstest.console to run the tests filtered by one category at a time with 1.4.0:

Category Test Class Per Execution Test Method Executed Total
A
  • A
  • B
  • B
  • C
  • D
  • A.Test
  • B.Test
  • B.Test
  • A.Test
  • D.Test
 5
B
  • B
  • B.Test
 1
C -- -- 0
D
  • D
  • D.Test
 1

The interesting thing is that test category A gets applied to B's new Test(), but instead of running both A.Test() with an instance of B and B.Test(), it runs B.Test() twice. But test category B only applies to B.Test(), so it runs once for that category. Test category C isn't applied to A.Test(), which is my original scenario.

Here's the results with this change:

Category Test Class Per Execution Test Method Executed Total
A
  • A
  • B
  • B
  • C
  • D
  • A.Test
  • B.Test
  • B.Test
  • A.Test
  • D.Test
 5
B
  • B
  • B
  • B.Test
  • B.Test
 2
C
  • C
  • A.Test
 1
D
  • D
  • D.Test
 1

A and D remain the same, but now that the test category of the subclass is applied to inherited methods, test category C includes A.Test() on an instance of C. A side-effect is that test category B then includes a second run of B.Test() like what happens in test category A.

The reason the duplicate execution happens is because the discoverer only sends the TestMethod.FullClassName and not the TestMethod.DeclaringClassFullName, so when the executor reacquires the MethodInfo, it does so on B instead of A, making it ambiguous. It simply chooses the first result, which is the B.Test() one. Assuming that it's considered backwards compatible to add new property values to the TestCase, it seems this could be fixed by sending the declaring class name as an optional property value and the executor can use that to find the correct MethodInfo.

Copy link
Contributor

@karanjitsingh karanjitsingh Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spanglerco, this is really good stuff. The changes look good but can we come up with a solution to avoid running tests multiple times for overloaded methods?

Since this is a case for overloading, for test category B the expectation would be that B.Test is run only once and since A.Test is being overloaded it should not run when running tests for category B.

Copy link
Contributor Author

@spanglerco spanglerco Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since A.Test is being overloaded it should not run when running tests for category B

@karanjitsingh I should have read this part more clearly to start with. I have a working implementation that resolves the duplicate execution, but I would have thought the desired behavior would be that test category B would run both B.Test and A.Test on an instance of B since B has both of those methods defined at the same time (B.Test is hiding A.Test, but A.Test is still there):

Category Test Class Per Execution Test Method Executed Total
A
  • A
  • B
  • B
  • C
  • D
  • A.Test
  • B.Test
  • A.Test
  • A.Test
  • D.Test
 5
B
  • B
  • B
  • B.Test
  • A.Test
 2
C
  • C
  • A.Test
 1
D
  • D
  • D.Test
 1

Copy link
Contributor

@karanjitsingh karanjitsingh Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spanglerco, in the case of this type of overload, the language specifies A.Test will be run if the object is of type A or the reference for object B is of type A, there is no right way here in this case but I would want to follow the convention that the language follows.

Copy link
Contributor

@karanjitsingh karanjitsingh Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spanglerco, any update on this?

Copy link
Contributor Author

@spanglerco spanglerco Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karanjitsingh, sorry for the delay, I haven't had a lot of time to look into this lately. I was able to tweak TypeEnumerator to remove the duplicate methods with these results:

Category Test Class Per Execution Test Method Executed Total
A
  • A
  • B
  • C
  • D
  • A.Test
  • B.Test
  • A.Test
  • D.Test
 4
B
  • B
  • B.Test
 1
C
  • C
  • A.Test
 1
D
  • D
  • D.Test
 1

}

if (categories != null)
Expand All @@ -389,18 +392,17 @@ internal IEnumerable<object> GetCustomAttributesRecursively(MemberInfo attribute
}

/// <summary>
/// Gets the custom attributes on the assembly of a member info
/// Gets the custom attributes on an assembly
/// NOTE: having it as separate virtual method, so that we can extend it for testing.
/// </summary>
/// <param name="memberInfo">The member to inspect.</param>
/// <param name="assembly">The assembly to inspect.</param>
/// <param name="type">The attribute type to find.</param>
/// <returns>Custom attributes defined.</returns>
internal virtual Attribute[] GetCustomAttributeForAssembly(MemberInfo memberInfo, Type type)
internal virtual Attribute[] GetCustomAttributeForAssembly(Assembly assembly, Type type)
{
return
PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(
memberInfo.DeclaringType.GetTypeInfo().Assembly,
type).OfType<Attribute>().ToArray();
assembly, type).OfType<Attribute>().ToArray();
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion test/UnitTests/MSTest.CoreAdapter.Unit.Tests/Discovery/TypeEnumeratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,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);

Expand Down
13 changes: 6 additions & 7 deletions test/UnitTests/MSTest.CoreAdapter.Unit.Tests/Helpers/ReflectHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public void IntializeTests()
this.reflectHelper = new TestableReflectHelper();
this.method = new Mock<MethodInfo>();
this.method.Setup(x => x.MemberType).Returns(MemberTypes.Method);
this.method.Setup(x => x.DeclaringType).Returns(typeof(ReflectHelperTests));

this.testablePlatformServiceProvider = new TestablePlatformServiceProvider();
this.testablePlatformServiceProvider.SetupMockReflectionOperations();
Expand All @@ -58,7 +57,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtClassLevel()
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel") }, MemberTypes.TypeInfo);

string[] expected = new[] { "ClassLevel" };
var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();

CollectionAssert.AreEqual(expected, actual);
}
Expand All @@ -73,7 +72,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtAllLevels()
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel") }, MemberTypes.TypeInfo);
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel") }, MemberTypes.Method);

var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();
string[] expected = new[] { "MethodLevel", "ClassLevel", "AsmLevel" };

CollectionAssert.AreEqual(expected, actual);
Expand All @@ -89,7 +88,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtAssemblyLevel()

string[] expected = new[] { "AsmLevel" };

var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();

CollectionAssert.AreEqual(expected, actual);
}
Expand All @@ -103,7 +102,7 @@ public void GetTestCategoryAttributeShouldIncludeMultipleTestCategoriesAtClassLe
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel"), new UTF.TestCategoryAttribute("ClassLevel1") }, MemberTypes.TypeInfo);

string[] expected = new[] { "ClassLevel", "ClassLevel1" };
var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();

CollectionAssert.AreEqual(expected, actual);
}
Expand All @@ -117,7 +116,7 @@ public void GetTestCategoryAttributeShouldIncludeMultipleTestCategoriesAtAssembl
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("AsmLevel"), new UTF.TestCategoryAttribute("AsmLevel1") }, MemberTypes.All);

string[] expected = new[] { "AsmLevel", "AsmLevel1" };
var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();
CollectionAssert.AreEqual(expected, actual);
}

Expand All @@ -130,7 +129,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtMethodLevel()
this.reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel") }, MemberTypes.Method);

string[] expected = new[] { "MethodLevel" };
var actual = this.reflectHelper.GetCategories(this.method.Object).ToArray();
var actual = this.reflectHelper.GetCategories(this.method.Object, typeof(ReflectHelperTests)).ToArray();

CollectionAssert.AreEqual(expected, actual);
}
Expand Down
2 changes: 1 addition & 1 deletion .../UnitTests/MSTest.CoreAdapter.Unit.Tests/TestableImplementations/TestableReflectHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes member
}
}

internal override Attribute[] GetCustomAttributeForAssembly(MemberInfo memberInfo, Type type)
internal override Attribute[] GetCustomAttributeForAssembly(Assembly assembly, Type type)
{
var hashcode = MemberTypes.All.GetHashCode() + type.FullName.GetHashCode();

Expand Down