-
Notifications
You must be signed in to change notification settings - Fork 353
Fix: DiaSession shows async methods to be external
#307
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.
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
Changes from 3 commits
bc3630c
b14aefe
be83f1a
f5b6872
0550ae1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,10 +97,10 @@ private static void GetMethodStartAndEndLineNumber( | |
| out int startLineNumber, | ||
| out int endLineNumber) | ||
| { | ||
| var startPoint = methodDebugDefinition.GetSequencePoints().OrderBy(s => s.StartLine).FirstOrDefault(); | ||
| var startPoint = methodDebugDefinition.GetSequencePoints().OrderBy(s => s.StartLine).FirstOrDefault(s => s.IsHidden == false); | ||
| startLineNumber = startPoint.StartLine; | ||
| var endPoint = | ||
| methodDebugDefinition.GetSequencePoints().OrderByDescending(s => s.StartLine).FirstOrDefault(); | ||
| methodDebugDefinition.GetSequencePoints().OrderByDescending(s => s.StartLine).FirstOrDefault(s => s.IsHidden == false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of calling same API twice, can we just call it once and then use FirstOrDefault and LastOrDefault method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, will change it. |
||
| endLineNumber = endPoint.StartLine; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,23 +100,22 @@ private void PopulateCacheForTypeAndMethodSymbols(string binaryPath) | |
| var typesDict = asm.GetTypes().ToDictionary(type => type.FullName); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of having a dictionary here? I think IEnumerable is what we need. |
||
| foreach (var typeEntry in typesDict) | ||
| { | ||
| // Get method infos for all types in assembly | ||
| var methodInfoDict = typeEntry.Value.GetMethods().ToDictionary(methodInfo => methodInfo.Name); | ||
| // Get declared method infos | ||
| var methodInfoList = ((TypeInfo)typeEntry.Value.GetTypeInfo()).DeclaredMethods; | ||
| var methodInfoDict = new Dictionary<string, MethodInfo>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need this dictionary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, list will be sufficient. |
||
| foreach (var methodInfo in methodInfoList) | ||
| { | ||
| methodInfoDict[methodInfo.Name] = methodInfo; | ||
| } | ||
| var methodsNavigationData = new Dictionary<string, DiaNavigationData>(); | ||
| this.methodsNavigationDataForType.Add(typeEntry.Key, methodsNavigationData); | ||
| this.methodsNavigationDataForType[typeEntry.Key] = methodsNavigationData; | ||
|
|
||
| foreach (var methodEntry in methodInfoDict) | ||
| { | ||
| if (string.CompareOrdinal(methodEntry.Value.Module.FullyQualifiedName, binaryPath) != 0) | ||
| { | ||
| // Ignore inherent methods | ||
| continue; | ||
| } | ||
|
|
||
| var diaNavigationData = pdbReader.GetDiaNavigationData(methodEntry.Value); | ||
| if (diaNavigationData != null) | ||
| { | ||
| methodsNavigationData.Add(methodEntry.Key, diaNavigationData); | ||
| methodsNavigationData[methodEntry.Key] = diaNavigationData; | ||
| } | ||
| else | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,18 @@ | ||
| <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <TestPlatformRoot Condition="$(TestPlatformRoot) == ''">$(MSBuildThisFileDirectory)../../</TestPlatformRoot> | ||
| <TestProject>true</TestProject> | ||
| </PropertyGroup> | ||
|
|
||
| <Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" /> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">Exe</OutputType> | ||
| <TargetFrameworks>netcoreapp1.0;net46</TargetFrameworks> | ||
| <AssemblyName>Microsoft.TestPlatform.Common.UnitTests</AssemblyName> | ||
| <PackageTargetFallback Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">$(PackageTargetFallback);dnxcore50;portable-net45+win8</PackageTargetFallback> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Compile Include="**\*.cs" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\Microsoft.TestPlatform.Common\Microsoft.TestPlatform.Common.csproj" /> | ||
| <ProjectReference Include="..\..\src\Microsoft.TestPlatform.ObjectModel\Microsoft.TestPlatform.ObjectModel.csproj"> | ||
|
|
@@ -31,26 +27,25 @@ | |
| </ProjectReference> | ||
| <ProjectReference Include="..\..\src\Microsoft.TestPlatform.CrossPlatEngine\Microsoft.TestPlatform.CrossPlatEngine.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' "> | ||
| <PackageReference Include="Microsoft.NETCore.App"> | ||
| <Version>1.0.0</Version> | ||
| </PackageReference> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition=" '$(TargetFramework)' == 'net46' "> | ||
| <Reference Include="System.Runtime" /> | ||
| <Reference Include="System" /> | ||
| <Reference Include="Microsoft.CSharp" /> | ||
| <Reference Include="System.Xml" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this service?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VS generates this automatically, to identify it as test project. Explanation here. |
||
| <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" /> | ||
| </ItemGroup> | ||
| <!-- Workaround for https://github.com/dotnet/sdk/issues/364 --> | ||
| <Target Name="WorkAroundPackageAndProjectReferenceConflict" BeforeTargets="ResolveLockFileReferences"> | ||
| <ItemGroup> | ||
| <ResolvedCompileFileDefinitions Remove="@(ResolvedCompileFileDefinitions)" Condition="'%(ResolvedCompileFileDefinitions.Filename)' == 'Microsoft.TestPlatform.CoreUtilities' Or '%(ResolvedCompileFileDefinitions.Filename)' == 'Microsoft.TestPlatform.Common'" /> | ||
| </ItemGroup> | ||
| </Target> | ||
|
|
||
| <Import Project="$(TestPlatformRoot)scripts\build\TestPlatform.targets" /> | ||
| </Project> | ||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,8 +40,44 @@ public void GetNavigationDataShouldReturnCorrectFileNameAndLineNumber() | |
| Assert.IsNotNull(diaNavigationData, "Failed to get navigation data"); | ||
| StringAssert.EndsWith(diaNavigationData.FileName, @"\SimpleTestProject\UnitTest1.cs"); | ||
|
|
||
| Assert.AreEqual(diaNavigationData.MinLineNumber, 23); | ||
| Assert.AreEqual(diaNavigationData.MaxLineNumber, 25); | ||
| Assert.AreEqual(23, diaNavigationData.MinLineNumber, "Incorrect min line number"); | ||
| Assert.AreEqual(25, diaNavigationData.MaxLineNumber, "Incorrect max line number"); | ||
|
|
||
| this.testEnvironment.TargetFramework = currentTargetFrameWork; | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void GetNavigationDataShouldReturnCorrectDataForAsyncMethod() | ||
| { | ||
| var currentTargetFrameWork = GetAndSetTargetFrameWork(this.testEnvironment); | ||
| var assemblyPath = this.GetAssetFullPath("SimpleTestProject3.dll"); | ||
|
|
||
| DiaSession diaSession = new DiaSession(assemblyPath); | ||
| DiaNavigationData diaNavigationData = diaSession.GetNavigationData("SampleUnitTestProject3.UnitTest1+<AsyncTestMethod>d__1", "MoveNext"); | ||
|
|
||
| Assert.IsNotNull(diaNavigationData, "Failed to get navigation data"); | ||
| StringAssert.EndsWith(diaNavigationData.FileName, @"\SimpleTestProject3\UnitTest1.cs"); | ||
|
|
||
| Assert.AreEqual(20, diaNavigationData.MinLineNumber, "Incorrect min line number"); | ||
| Assert.AreEqual(22, diaNavigationData.MaxLineNumber, "Incorrect max line number"); | ||
|
|
||
| this.testEnvironment.TargetFramework = currentTargetFrameWork; | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void GetNavigationDataShouldReturnCorrectDataForOverLoadedMethod() | ||
| { | ||
| var currentTargetFrameWork = GetAndSetTargetFrameWork(this.testEnvironment); | ||
| var assemblyPath = this.GetAssetFullPath("SimpleTestProject3.dll"); | ||
|
|
||
| DiaSession diaSession = new DiaSession(assemblyPath); | ||
| DiaNavigationData diaNavigationData = diaSession.GetNavigationData("SampleUnitTestProject3.Class1", "OverLoadededMethod"); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which method's navigation data should it return, there are two methods with the same name?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently it is returning the last method in source file for both full and portable pdb. |
||
| Assert.IsNotNull(diaNavigationData, "Failed to get navigation data"); | ||
| StringAssert.EndsWith(diaNavigationData.FileName, @"\SimpleTestProject3\UnitTest1.cs"); | ||
|
|
||
| Assert.AreEqual(32, diaNavigationData.MinLineNumber, "Incorrect min line number"); | ||
| Assert.AreEqual(33, diaNavigationData.MaxLineNumber, "Incorrect max line number"); | ||
|
|
||
| this.testEnvironment.TargetFramework = currentTargetFrameWork; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s => s.IsHidden == falseWhat's the purpose of adding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By following reference SequencePointList.cs, SequencePoint.cs, FullSymbolReader.cs IsHidden denotes special point which is not exists in source file. Should be ignored.