Skip to content
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

Improve coverage of async/await state machine #698

Merged
merged 14 commits into from
Jan 22, 2020
Merged
2 changes: 1 addition & 1 deletion Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<!-- Do not upgrade this version or we won't support old SDK -->
<PackageReference Update="Newtonsoft.Json" Version="9.0.1" />
<PackageReference Update="NuGet.Packaging" Version="5.4.0" />
<PackageReference Update="ReportGenerator.Core" Version="4.4.4" />
<PackageReference Update="ReportGenerator.Core" Version="4.4.5" />
<!--
Do not change System.Reflection.Metadata version since we need to support VSTest DataCollectors. Goto https://www.nuget.org/packages/System.Reflection.Metadata to check versions.
We need to load assembly version 1.4.2.0 to properly work
Expand Down
7 changes: 7 additions & 0 deletions coverlet.sln
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "coverlet.integration.tests"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "coverlet.integration.template", "test\coverlet.integration.template\coverlet.integration.template.csproj", "{F6FE7678-C662-43D3-AC6A-64F6AC5A5935}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "coverlet.core.tests.samples.netstandard", "test\coverlet.core.tests.samples.netstandard\coverlet.core.tests.samples.netstandard.csproj", "{5FF404AD-7C0B-465A-A1E9-558CDC642B0C}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -104,6 +106,10 @@ Global
{F6FE7678-C662-43D3-AC6A-64F6AC5A5935}.Debug|Any CPU.Build.0 = Debug|Any CPU
{F6FE7678-C662-43D3-AC6A-64F6AC5A5935}.Release|Any CPU.ActiveCfg = Release|Any CPU
{F6FE7678-C662-43D3-AC6A-64F6AC5A5935}.Release|Any CPU.Build.0 = Release|Any CPU
{5FF404AD-7C0B-465A-A1E9-558CDC642B0C}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{5FF404AD-7C0B-465A-A1E9-558CDC642B0C}.Debug|Any CPU.Build.0 = Debug|Any CPU
{5FF404AD-7C0B-465A-A1E9-558CDC642B0C}.Release|Any CPU.ActiveCfg = Release|Any CPU
{5FF404AD-7C0B-465A-A1E9-558CDC642B0C}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand All @@ -122,6 +128,7 @@ Global
{D6B14F2F-9E7D-4D2C-BAC8-48834F853ED6} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134}
{99B4059C-B25C-4B82-8117-A0E9DC9B0949} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134}
{F6FE7678-C662-43D3-AC6A-64F6AC5A5935} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134}
{5FF404AD-7C0B-465A-A1E9-558CDC642B0C} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {9CA57C02-97B0-4C38-A027-EA61E8741F10}
Expand Down
2 changes: 1 addition & 1 deletion eng/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ steps:
displayName: Test
inputs:
command: test
arguments: -c $(BuildConfiguration) --no-build /p:CollectCoverage=true /p:CoverletOutputFormat=opencover /p:Include=[coverlet.*]* /p:Exclude=[coverlet.tests.remoteexecutor]*
arguments: -c $(BuildConfiguration) --no-build /p:CollectCoverage=true /p:CoverletOutputFormat=opencover /p:Include="[coverlet.collector]*%2c[coverlet.core]*%2c[coverlet.msbuild.tasks]*" /p:Exclude="[coverlet.core.tests.samples.netstandard]*%2c[coverlet.tests.remoteexecutor]*"
testRunTitle: $(Agent.JobName)
99 changes: 95 additions & 4 deletions src/coverlet.core/Symbols/CecilSymbolHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,25 @@ private static bool IsMoveNextInsideEnumerator(MethodDefinition methodDefinition
return false;
}

private static bool IsRecognizedMoveNextInsideAsyncStateMachineProlog(MethodDefinition methodDefinition)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alg update

{
/*
int num = <>1__state;
IL_0000: ldarg.0
IL_0001: ldfld ...::'<>1__state'
IL_0006: stloc.0
*/
return (methodDefinition.Body.Instructions[0].OpCode == OpCodes.Ldarg_0 ||
methodDefinition.Body.Instructions[0].OpCode == OpCodes.Ldarg) &&

methodDefinition.Body.Instructions[1].OpCode == OpCodes.Ldfld &&
methodDefinition.Body.Instructions[1].Operand is FieldDefinition fd && fd.Name == "<>1__state" &&

(methodDefinition.Body.Instructions[2].OpCode == OpCodes.Stloc &&
methodDefinition.Body.Instructions[2].Operand is VariableDefinition vd && vd.Index == 0) ||
methodDefinition.Body.Instructions[2].OpCode == OpCodes.Stloc_0;
}

public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition)
{
var list = new List<BranchPoint>();
Expand All @@ -69,9 +88,9 @@ public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinitio
UInt32 ordinal = 0;
var instructions = methodDefinition.Body.Instructions;

// if method is a generated MoveNext skip first branch (could be a switch or a branch)
bool isAsyncStateMachineMoveNext = IsMoveNextInsideAsyncStateMachine(methodDefinition);
bool skipFirstBranch = isAsyncStateMachineMoveNext || IsMoveNextInsideEnumerator(methodDefinition);
bool isRecognizedMoveNextInsideAsyncStateMachineProlog = isAsyncStateMachineMoveNext && IsRecognizedMoveNextInsideAsyncStateMachineProlog(methodDefinition);
bool skipFirstBranch = IsMoveNextInsideEnumerator(methodDefinition);

foreach (Instruction instruction in instructions.Where(instruction => instruction.OpCode.FlowControl == FlowControl.Cond_Branch))
{
Expand All @@ -83,15 +102,87 @@ public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinitio
continue;
}

/*
If method is a generated MoveNext we'll skip first branches (could be a switch or a series of branches)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alg update

that check state machine value to jump to correct state(for instance after a true async call)
Check if it's a Cond_Branch on state machine current value int num = <>1__state;
We are on branch OpCode so we need to go back by max 2 operation to reach ldloc.0 the load of "num"
Max 2 because we handle following patterns

Swich

// switch (num)
IL_0007: ldloc.0 2
// (no C# code)
IL_0008: switch (IL_0037, IL_003c, ... 1
...

Single branch

// if (num != 0)
IL_0007: ldloc.0 2
// (no C# code)
IL_0008: brfalse.s IL_000c 1
IL_000a: br.s IL_000e
IL_000c: br.s IL_0049
IL_000e: nop
...

More tha one branch

// if (num != 0)
IL_0007: ldloc.0
// (no C# code)
IL_0008: brfalse.s IL_0012
IL_000a: br.s IL_000c
// if (num == 1)
IL_000c: ldloc.0 3
IL_000d: ldc.i4.1 2
IL_000e: beq.s IL_0014 1
// (no C# code)
IL_0010: br.s IL_0019
IL_0012: br.s IL_0060
IL_0014: br IL_00e5
IL_0019: nop
...

so we know that current branch are checking that field and we're not interested in.
*/
if (isAsyncStateMachineMoveNext && isRecognizedMoveNextInsideAsyncStateMachineProlog)
{
bool skipInstruction = false;
Instruction current = instruction.Previous;
for (int instructionBefore = 2; instructionBefore > 0 && current.Previous != null; current = current.Previous, instructionBefore--)
{
if (
(current.OpCode == OpCodes.Ldloc && current.Operand is VariableDefinition vo && vo.Index == 0) ||
current.OpCode == OpCodes.Ldloc_0
)
{
skipInstruction = true;
break;
}
}
if (skipInstruction)
{
continue;
}
}

// Skip get_IsCompleted to avoid unuseful branch due to async/await state machine
if (isAsyncStateMachineMoveNext && instruction.Previous.Operand is MethodReference operand &&
if (
isAsyncStateMachineMoveNext && instruction.Previous.Operand is MethodReference operand &&
operand.Name == "get_IsCompleted" &&
(
operand.DeclaringType.FullName.StartsWith("System.Runtime.CompilerServices.TaskAwaiter") ||
operand.DeclaringType.FullName.StartsWith("System.Runtime.CompilerServices.ConfiguredTaskAwaitable")
)
&&
operand.DeclaringType.Scope.Name == "System.Runtime")
(
operand.DeclaringType.Scope.Name == "System.Runtime" ||
operand.DeclaringType.Scope.Name == "netstandard"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

include netstandard

)
)
{
continue;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Remember to use full name because adding new using directives change line numbers

namespace Coverlet.Core.Tests
{
public class Issue_669_2
{
private readonly System.Net.Http.HttpClient _httpClient = new System.Net.Http.HttpClient();

async public System.Threading.Tasks.ValueTask<System.Net.Http.HttpResponseMessage> SendRequest()
{
using (var requestMessage = new System.Net.Http.HttpRequestMessage(System.Net.Http.HttpMethod.Get, "https://www.google.it"))
{
return await _httpClient.SendAsync(requestMessage).ConfigureAwait(false);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<IsPackable>false</IsPackable>
<IsTestProject>false</IsTestProject>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.3" />
</ItemGroup>

</Project>
130 changes: 130 additions & 0 deletions test/coverlet.core.tests/Coverage/CoverageTests.AsyncAwait.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System.IO;
using System.Threading.Tasks;

using Coverlet.Core.Samples.Tests;
using Coverlet.Tests.RemoteExecutor;
using Xunit;

namespace Coverlet.Core.Tests
{
public partial class CoverageTests
{
[Fact]
public void AsyncAwait()
{
string path = Path.GetTempFileName();
try
{
RemoteExecutor.Invoke(async pathSerialize =>
{
CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<AsyncAwait>(instance =>
{
instance.SyncExecution();

int res = ((Task<int>)instance.AsyncExecution(true)).ConfigureAwait(false).GetAwaiter().GetResult();
res = ((Task<int>)instance.AsyncExecution(1)).ConfigureAwait(false).GetAwaiter().GetResult();
res = ((Task<int>)instance.AsyncExecution(2)).ConfigureAwait(false).GetAwaiter().GetResult();
res = ((Task<int>)instance.AsyncExecution(3)).ConfigureAwait(false).GetAwaiter().GetResult();
res = ((Task<int>)instance.ContinuationCalled()).ConfigureAwait(false).GetAwaiter().GetResult();
res = ((Task<int>)instance.ConfigureAwait()).ConfigureAwait(false).GetAwaiter().GetResult();

return Task.CompletedTask;
}, persistPrepareResultToFile: pathSerialize);
return 0;
}, path).Dispose();

CoverageResult result = TestInstrumentationHelper.GetCoverageResult(path);

result.Document("Instrumentation.AsyncAwait.cs")
.AssertLinesCovered(BuildConfiguration.Debug,
// AsyncExecution(bool)
(10, 1), (11, 1), (12, 1), (14, 1), (16, 1), (17, 0), (18, 0), (19, 0), (21, 1), (22, 1),
// Async
(25, 9), (26, 9), (27, 9), (28, 9),
// SyncExecution
(31, 1), (32, 1), (33, 1),
// Sync
(36, 1), (37, 1), (38, 1),
// AsyncExecution(int)
(41, 3), (42, 3), (43, 3), (46, 1), (47, 1), (48, 1), (51, 1),
(52, 1), (53, 1), (56, 1), (57, 1), (58, 1), (59, 1),
(62, 0), (63, 0), (64, 0), (65, 0), (68, 0), (70, 3), (71, 3),
// ContinuationNotCalled
(74, 0), (75, 0), (76, 0), (77, 0), (78, 0),
// ContinuationCalled -> line 83 should be 1 hit some issue with Continuation state machine
(81, 1), (82, 1), (83, 2), (84, 1), (85, 1),
// ConfigureAwait
(89, 1), (90, 1)
)
.AssertBranchesCovered(BuildConfiguration.Debug, (16, 0, 0), (16, 1, 1), (43, 0, 3), (43, 1, 1), (43, 2, 1), (43, 3, 1), (43, 4, 0))
// Real branch should be 2, we should try to remove compiler generated branch in method ContinuationNotCalled/ContinuationCalled
// for Continuation state machine
.ExpectedTotalNumberOfBranches(BuildConfiguration.Debug, 4);
}
finally
{
File.Delete(path);
}
}

[Fact]
public void AsyncAwait_Issue_669_1()
{
string path = Path.GetTempFileName();
try
{
RemoteExecutor.Invoke(async pathSerialize =>
{
CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<Issue_669_1>(instance =>
{
((Task)instance.Test()).ConfigureAwait(false).GetAwaiter().GetResult();
return Task.CompletedTask;
},
persistPrepareResultToFile: pathSerialize, disableRestoreModules: true);

return 0;
}, path, invokeInProcess: true).Dispose();

TestInstrumentationHelper.GetCoverageResult(path)
.Document("Instrumentation.AsyncAwait.cs")
.AssertLinesCovered(BuildConfiguration.Debug,
(97, 1), (98, 1), (99, 1), (101, 1), (102, 1), (103, 1),
(110, 1), (111, 1), (112, 1), (113, 1),
(116, 1), (117, 1), (118, 1), (119, 1));
}
finally
{
File.Delete(path);
}
}

[Fact]
public void AsyncAwait_Issue_669_2()
{
string path = Path.GetTempFileName();
try
{
RemoteExecutor.Invoke(async pathSerialize =>
{
CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run<Issue_669_2>(instance =>
{
((ValueTask<System.Net.Http.HttpResponseMessage>)instance.SendRequest()).ConfigureAwait(false).GetAwaiter().GetResult();
return Task.CompletedTask;
},
persistPrepareResultToFile: pathSerialize, disableRestoreModules: true);

return 0;
}, path, invokeInProcess: true).Dispose();

TestInstrumentationHelper.GetCoverageResult(path)
.Document("Instrumentation.AsyncAwait.cs")
.AssertLinesCovered(BuildConfiguration.Debug, (7, 1), (10, 1), (11, 1), (12, 1), (13, 1), (15, 1))
.ExpectedTotalNumberOfBranches(BuildConfiguration.Debug, 0);
}
finally
{
File.Delete(path);
}
}
}
}
Loading