Apply TestCategory from derived class on inherited test methods #513
Conversation
Enables sharing test methods across a class hierarchy while categorizing the inherited tests based on the derived class that's actually running them.
karanjitsingh
requested review from
jayaranigarg,
singhsarab and
mayankbansal018
| } | ||
|
|
||
| if (categories != null) | ||
| { | ||
| categories = categories.Concat(this.GetCustomAttributeForAssembly(attributeProvider, typeof(TestCategoryBaseAttribute))).ToArray(); | ||
| categories = categories.Concat(this.GetCustomAttributeForAssembly(owningType.GetTypeInfo().Assembly, typeof(TestCategoryBaseAttribute))).ToArray(); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 |
|
|
5 |
| B |
|
|
1 |
| C | -- | -- | 0 |
| D |
|
|
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 |
|
|
5 |
| B |
|
|
2 |
| C |
|
|
1 |
| D |
|
|
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 |
|
|
5 |
| B |
|
|
2 |
| C |
|
|
1 |
| D |
|
|
1 |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@spanglerco, any update on this?
There was a problem hiding this comment.
@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 |
|
|
4 |
| B |
|
|
1 |
| C |
|
|
1 |
| D |
|
|
1 |
|
@spanglerco, sorry for the late reply. I'll be continuously monitoring this PR from now on until its conclusion. |
…erence the member's Module instead of DeclaringType
Selects the one declared closest to the test class among all valid test methods with the same name.
|
@spanglerco, some tests are failing. Can you fix? |
|
@karanjitsingh, should be fixed now. |
|
@karanjitsingh, it looks like the RunTestsForTestShouldPreferParallelSettingsFromRunSettingsOverAssemblyLevelAttributes test failed in my PR release build, which I'm guessing is intermittent. Is there a way to trigger it to try again? Also, I'm wondering if that test should use something like a ConcurrentDictionary instead of HashSet (I'm just guessing and don't know the history of why the sleeps are there). |
|
@dotnet-bot test Windows / Release Build please |
|
@dotnet-bot test Windows / Release Build please |
|
Thank you for getting around to this one @jayaranigarg |
Enables sharing test methods across a class hierarchy while categorizing the inherited tests based on the derived class that's actually running them. It does this by passing the class type from the
TypeEnumeratorthrough to theReflectHelperrather than relying onMemberInfo.DeclaringType.Fixes #498. Previous behavior was that inherited methods would only receive test categories starting at the class defining the method. This changes it so inherited methods receive test categories starting at the test class being discovered.
I didn't add any new tests because the updated unit tests for ReflectHelper verify the correct class type argument is passed to ReflectionUtility, and the existing tests for ReflectionUtility verify correct test categories are returned based on the class hierarchy passed in. I have manually verified expected behavior specified by @karanjitsingh in #498 (comment) which is my original use case.