Fixes navigation for base class methods declared in other assemblies #174
Conversation
|
@ajryan, |
| if (!method.DeclaringType.FullName.Equals(this.type.FullName)) | ||
| { | ||
| testMethod.DeclaringClassFullName = method.DeclaringType.FullName; | ||
| } | ||
|
|
||
| if (!this.reflectHelper.IsMethodDeclaredInSameAssemblyAsType(method, this.type)) |
There was a problem hiding this comment.
How about passing this in from the parent caller when it calls GetTestFromMethod? Wouldn't need to query for it again then.
There was a problem hiding this comment.
Good idea. I was thinking since it's internal the method could be used outside GetTests but it's only called by unit tests, so OK.
| @@ -80,15 +80,29 @@ internal UnitTestDiscoverer() | |||
|
|
|||
| internal void SendTestCases(string source, IEnumerable<UnitTestElement> testElements, ITestCaseDiscoverySink discoverySink) | |||
| { | |||
| object navigationSession = null; | |||
| Dictionary<string, object> navigationSessions = new Dictionary<string, object>(); | |||
There was a problem hiding this comment.
Am loving this. 👍 Performance is one of our key pillars. Although we haven't heard much in terms of performance issues in mstest v2, we learnt the hard way that this could hit us hard down the line.
| @@ -80,15 +80,29 @@ internal UnitTestDiscoverer() | |||
|
|
|||
| internal void SendTestCases(string source, IEnumerable<UnitTestElement> testElements, ITestCaseDiscoverySink discoverySink) | |||
| { | |||
| object navigationSession = null; | |||
| Dictionary<string, object> navigationSessions = new Dictionary<string, object>(); | |||
There was a problem hiding this comment.
nit: this can be a var instead.
| @@ -51,6 +56,25 @@ public TestMethod(string name, string fullClassName, string assemblyName, bool | |||
|
|
|||
| /// <summary> | |||
| /// Gets or sets the declaring class full name. This will be used while getting navigation data. | |||
| /// This will be null if AssemblyName is same as DeclaringAssemblyName. | |||
| /// Reason to set to null in the above case is to minimise the transfer of data across appdomains and not have a perf hit. | |||
| this.unitTestDiscoverer.SendTestCases(source, testElements, this.mockTestCaseDiscoverySink.Object); | ||
|
|
||
| // Assert | ||
| this.mockTestCaseDiscoverySink.Verify(ds => ds.SendTestCase(It.Is<TestCase>(tc => VerifyTestCase(tc))), Times.Once); |
There was a problem hiding this comment.
Strange. It looks like all methods in this class seem to be validating FullyQualifiedName instead of the navigationData(line number and fileName). Changing it to that assertion would fail this test though since the mock setup is done on DummyAssembly.dll and we query navigation session on DA instead.
Its perfectly fine if we just fix this one test and I'l take an AI to fix up the rest.
There was a problem hiding this comment.
Ah yeah I thought this was weird. Actually I didn't mean to leave that little VerifyTestCase method but had it for edit-and-continue convenience to find a better check. Will improve this test.
There was a problem hiding this comment.
Updated test is named SendTestCasesShouldUseNaigationSessionForDeclaredAssemblyName which I think is the only SendTestCases responsibility that needs to be tested at this layer.
| this.SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: false); | ||
| TypeEnumerator typeEnumerator = this.GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); | ||
| var methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); | ||
| var deploymentItems = new[] { new KeyValuePair<string, string>("C:\\temp", string.Empty) }; |
There was a problem hiding this comment.
nit: This does not seem to be used.
| /// <returns>Path to the .DLL of the assembly.</returns> | ||
| public string GetAssemblyPath(Assembly assembly) | ||
| { | ||
| #if NETSTANDARD1_5 |
There was a problem hiding this comment.
@jayaranigarg: Do let us know if you see any concerns with this.
|
@ajryan : Apologies this took some time. We've been heads down with a few release activities. |
|
Thanks for the thorough review! I'll take some time to address the issues soon. |
|
Last changes should resolve your comments. Sorry for the slow turnaround, and thanks again for welcoming my work! |
|
No problem. Thanks again. This should just make #164 a simple flip of a switch now. |
…onnection problem in test host and vstest which is very large time. (microsoft#174) Fix: Reduced it to one min
Fix for #163