Allow discovering test methods from base class in different assembly #147
Conversation
|
@ajryan, It will cover your contributions to all Microsoft-managed open source projects. |
|
@ajryan, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
| Assert.IsNotNull(tests); | ||
| Assert.AreEqual(methodCount, tests.Count); | ||
| Assert.IsTrue(tests.Any(t => t.TestMethod.Name == "BaseTestMethod")); |
There was a problem hiding this comment.
Assert.IsTrue(tests.Any(t => t.TestMethod.Name == "BaseTestMethod")); [](start = 12, length = 69)
Do add a message on assertion failure so it is easy to identify why this test failed.
There was a problem hiding this comment.
Will do for all test methods I've changed/added.
|
|
||
| var tests = typeEnumerator.Enumerate(out this.warnings); | ||
|
|
||
| Assert.IsTrue(tests.All(t => t.TestMethod.Name != "BaseTestMethod")); |
There was a problem hiding this comment.
t.TestMethod.Name != "BaseTestMethod" [](start = 41, length = 37)
add a contains check instead.
There was a problem hiding this comment.
How would that work? tests.Contains would expect an instance of UnitTestElement and that doesn't implement equality members, nor does TestMethod...
| @@ -97,8 +97,10 @@ internal Collection<UnitTestElement> GetTests(ICollection<string> warnings) | |||
| // Test class is already valid. Verify methods. | |||
| foreach (var method in this.type.GetRuntimeMethods()) | |||
| { | |||
| // Todo: Provide settings to allow users to pick up tests from other assemblies as well. | |||
| if (!method.DeclaringType.GetTypeInfo().Assembly.Equals(this.type.GetTypeInfo().Assembly)) | |||
| var isMethodDeclaredInTestTypeAssembly = method.DeclaringType.GetTypeInfo().Assembly.Equals(this.type.GetTypeInfo().Assembly); | |||
There was a problem hiding this comment.
method.DeclaringType.GetTypeInfo().Assembly.Equals(this.type.GetTypeInfo().Assembly) [](start = 57, length = 84)
This seems to be causing issues in testability. How about putting this in a separate API in reflecthelper and mocking that out in your tests?
There was a problem hiding this comment.
Great idea, will do.
| @@ -238,6 +245,16 @@ private static MSTestSettings ToSettings(XmlReader reader) | |||
| break; | |||
| } | |||
|
|
|||
| case "ENABLEBASECLASSTESTMETHODSFROMOTHERASSEMBLIES": | |||
There was a problem hiding this comment.
ENABLEBASECLASSTESTMETHODSFROMOTHERASSEMBLIES [](start = 30, length = 45)
can we add tests for this too please? you would find them under MSTest.CoreAdapter.Unit.Tests -> MSTestSettingsTests.cs
|
Thanks a bunch for getting this fixed quickly @ajryan. Had a few comments. Will push this in as soon as they are resolved. |
|
@pvlakshm should this be turned on by default(searching for test methods in a base class defined in another assembly) given that this is a common scenario? |
|
Thanks for the quick review! I'll have a revision soon. Please see my question above about the Contains thing too... |
|
This should be ready except for the comment about using Contains |
|
Awesome, thanks for reviewing and merging so quickly! 🎉 |
|
Pleasure. Thanks for the contribution. This was a long standing issue with around 96 votes on uservoice. Great impact 🎉 |
Fixes issue #23