Skip to content

Fix false negative with "/p:MergeWith" async/await branches #277

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

Merged
merged 8 commits into from
Jan 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions src/coverlet.core/Coverage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public CoverageResult GetCoverageResult()
InstrumentationHelper.RestoreOriginalModule(result.ModulePath, _identifier);
}

var coverageResult = new CoverageResult { Identifier = _identifier, Modules = modules };
var coverageResult = new CoverageResult { Identifier = _identifier, Modules = modules, InstrumentedResults = _results };

if (!string.IsNullOrEmpty(_mergeWith) && !string.IsNullOrWhiteSpace(_mergeWith) && File.Exists(_mergeWith))
{
Expand Down Expand Up @@ -257,14 +257,14 @@ private void CalculateCoverage()
}

// for MoveNext() compiler autogenerated method we need to patch false positive (IAsyncStateMachine for instance)
// we'll remove all MoveNext() not covered branch
// we'll remove all MoveNext() not covered branch
foreach (var document in result.Documents)
{
List<KeyValuePair<(int, int), Branch>> branchesToRemove = new List<KeyValuePair<(int, int), Branch>>();
foreach (var branch in document.Value.Branches)
{
//if one branch is covered we search the other one only if it's not covered
if (CecilSymbolHelper.IsMoveNext(branch.Value.Method) && branch.Value.Hits > 0)
if (IsAsyncStateMachineMethod(branch.Value.Method) && branch.Value.Hits > 0)
{
foreach (var moveNextBranch in document.Value.Branches)
{
Expand All @@ -286,6 +286,23 @@ private void CalculateCoverage()
}
}

private bool IsAsyncStateMachineMethod(string method)
{
if (!method.EndsWith("::MoveNext()"))
{
return false;
}

foreach (var instrumentationResult in _results)
{
if (instrumentationResult.AsyncMachineStateMethod.Contains(method))
{
return true;
}
}
return false;
}

private string GetSourceLinkUrl(Dictionary<string, string> sourceLinkDocuments, string document)
{
if (sourceLinkDocuments.TryGetValue(document, out string url))
Expand Down
54 changes: 54 additions & 0 deletions src/coverlet.core/CoverageResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
using System.IO;
using System.Linq;
using Coverlet.Core.Enums;
using Coverlet.Core.Instrumentation;
using Coverlet.Core.Symbols;

namespace Coverlet.Core
{
Expand Down Expand Up @@ -39,6 +41,7 @@ public class CoverageResult
{
public string Identifier;
public Modules Modules;
internal List<InstrumenterResult> InstrumentedResults;

internal CoverageResult() { }

Expand Down Expand Up @@ -105,6 +108,57 @@ internal void Merge(Modules modules)
}
}
}

// for MoveNext() compiler autogenerated method we need to patch false positive (IAsyncStateMachine for instance)
// we'll remove all MoveNext() not covered branch
List<BranchInfo> branchesToRemove = new List<BranchInfo>();
foreach (var module in this.Modules)
{
foreach (var document in module.Value)
{
foreach (var @class in document.Value)
{
foreach (var method in @class.Value)
{
foreach (var branch in method.Value.Branches)
{
//if one branch is covered we search the other one only if it's not covered
if (IsAsyncStateMachineMethod(method.Key) && branch.Hits > 0)
{
foreach (var moveNextBranch in method.Value.Branches)
{
if (moveNextBranch != branch && moveNextBranch.Hits == 0)
{
branchesToRemove.Add(moveNextBranch);
}
}
}
}
foreach (var branchToRemove in branchesToRemove)
{
method.Value.Branches.Remove(branchToRemove);
}
}
}
}
}
}

private bool IsAsyncStateMachineMethod(string method)
{
if (!method.EndsWith("::MoveNext()"))
{
return false;
}

foreach (var instrumentedResult in InstrumentedResults)
{
if (instrumentedResult.AsyncMachineStateMethod.Contains(method))
{
return true;
}
}
return false;
}

public ThresholdTypeFlags GetThresholdTypesBelowThreshold(CoverageSummary summary, double threshold, ThresholdTypeFlags thresholdTypes, ThresholdStatistic thresholdStat)
Expand Down
35 changes: 35 additions & 0 deletions src/coverlet.core/Instrumentation/Instrumenter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ internal class Instrumenter
private TypeDefinition _customTrackerTypeDef;
private MethodReference _customTrackerRegisterUnloadEventsMethod;
private MethodReference _customTrackerRecordHitMethod;
private List<string> _asyncMachineStateMethod;

public Instrumenter(string module, string identifier, string[] excludeFilters, string[] includeFilters, string[] excludedFiles, string[] excludedAttributes)
{
Expand Down Expand Up @@ -61,6 +62,8 @@ public InstrumenterResult Instrument()

InstrumentModule();

_result.AsyncMachineStateMethod = _asyncMachineStateMethod == null ? Array.Empty<string>() : _asyncMachineStateMethod.ToArray();

return _result;
}

Expand Down Expand Up @@ -372,6 +375,7 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor

var key = (branchPoint.StartLine, (int)branchPoint.Ordinal);
if (!document.Branches.ContainsKey(key))
{
document.Branches.Add(key,
new Branch
{
Expand All @@ -385,12 +389,43 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor
}
);

if (IsAsyncStateMachineBranch(method.DeclaringType, method))
{
if (_asyncMachineStateMethod == null)
{
_asyncMachineStateMethod = new List<string>();
}

if (!_asyncMachineStateMethod.Contains(method.FullName))
{
_asyncMachineStateMethod.Add(method.FullName);
}
}
}

var entry = (true, document.Index, branchPoint.StartLine, (int)branchPoint.Ordinal);
_result.HitCandidates.Add(entry);

return AddInstrumentationInstructions(method, processor, instruction, _result.HitCandidates.Count - 1);
}

private bool IsAsyncStateMachineBranch(TypeDefinition typeDef, MethodDefinition method)
{
if (!method.FullName.EndsWith("::MoveNext()"))
{
return false;
}

foreach (InterfaceImplementation implementedInterface in typeDef.Interfaces)
{
if (implementedInterface.InterfaceType.FullName == "System.Runtime.CompilerServices.IAsyncStateMachine")
{
return true;
}
}
return false;
}

private Instruction AddInstrumentationInstructions(MethodDefinition method, ILProcessor processor, Instruction instruction, int hitEntryIndex)
{
if (_customTrackerRecordHitMethod == null)
Expand Down
1 change: 1 addition & 0 deletions src/coverlet.core/Instrumentation/InstrumenterResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public InstrumenterResult()
}

public string Module;
public string[] AsyncMachineStateMethod;
public string HitsFilePath;
public string HitsResultGuid;
public string ModulePath;
Expand Down
5 changes: 0 additions & 5 deletions src/coverlet.core/Symbols/CecilSymbolHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ public static class CecilSymbolHelper
private const int StepOverLineCode = 0xFEEFEE;
private static readonly Regex IsMovenext = new Regex(@"\<[^\s>]+\>\w__\w(\w)?::MoveNext\(\)$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);

public static bool IsMoveNext(string fullName)
{
return IsMovenext.IsMatch(fullName);
}

public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition)
{
var list = new List<BranchPoint>();
Expand Down