diff --git a/Documentation/Changelog.md b/Documentation/Changelog.md index 142e0be6a..abc061d9a 100644 --- a/Documentation/Changelog.md +++ b/Documentation/Changelog.md @@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 -Attribute exclusion does not work if attribute name does not end with "Attribute" [#884](https://github.com/coverlet-coverage/coverlet/pull/884) by https://github.com/bddckr -Fix deterministic build+source link bug [#895](https://github.com/coverlet-coverage/coverlet/pull/895) -Fix anonymous delegate compiler generate bug [#896](https://github.com/coverlet-coverage/coverlet/pull/896) --Fix incorrect branch coverage with await ValueTask [#949](https://github.com/coverlet-coverage/coverlet/pull/949) by https://github.com/alexthornton1 +-Fix incorrect branch coverage with await ValueTask [#949](https://github.com/coverlet-coverage/coverlet/pull/949) by https://github.com/alexthornton1 +-Fix switch pattern coverage [#1006](https://github.com/coverlet-coverage/coverlet/pull/1006) ### Added -Skip autoprops feature [#912](https://github.com/coverlet-coverage/coverlet/pull/912) diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs index 92d84273e..45a5a5f9d 100644 --- a/src/coverlet.core/Coverage.cs +++ b/src/coverlet.core/Coverage.cs @@ -392,7 +392,33 @@ private void CalculateCoverage() } } - List<(int docIndex, int line)> zeroHitsLines = new List<(int docIndex, int line)>(); + // Calculate lines to skip for every hits start/end candidate + // Nested ranges win on outermost one + foreach (HitCandidate hitCandidate in result.HitCandidates) + { + if (hitCandidate.isBranch || hitCandidate.end == hitCandidate.start) + { + continue; + } + + foreach (HitCandidate hitCandidateToCompare in result.HitCandidates) + { + if (hitCandidate != hitCandidateToCompare && !hitCandidateToCompare.isBranch) + { + if (hitCandidateToCompare.start >= hitCandidate.start && + hitCandidateToCompare.end <= hitCandidate.end) + { + for (int i = hitCandidateToCompare.start; + i <= (hitCandidateToCompare.end == 0 ? hitCandidateToCompare.start : hitCandidateToCompare.end); + i++) + { + (hitCandidate.AccountedByNestedInstrumentation ??= new HashSet()).Add(i); + } + } + } + } + } + var documentsList = result.Documents.Values.ToList(); using (var fs = _fileSystem.NewFileStream(result.HitsFilePath, FileMode.Open)) using (var br = new BinaryReader(fs)) @@ -416,27 +442,14 @@ private void CalculateCoverage() { for (int j = hitLocation.start; j <= hitLocation.end; j++) { - var line = document.Lines[j]; - line.Hits += hits; - - // We register 0 hit lines for later cleanup false positive of nested lambda closures - if (hits == 0) + if (hitLocation.AccountedByNestedInstrumentation?.Contains(j) == true) { - zeroHitsLines.Add((hitLocation.docIndex, line.Number)); + continue; } - } - } - } - } - // Cleanup nested state machine false positive hits - foreach (var (docIndex, line) in zeroHitsLines) - { - foreach (var lineToCheck in documentsList[docIndex].Lines) - { - if (lineToCheck.Key == line) - { - lineToCheck.Value.Hits = 0; + var line = document.Lines[j]; + line.Hits += hits; + } } } } diff --git a/src/coverlet.core/Instrumentation/InstrumenterResult.cs b/src/coverlet.core/Instrumentation/InstrumenterResult.cs index 481e7410c..a575f7739 100644 --- a/src/coverlet.core/Instrumentation/InstrumenterResult.cs +++ b/src/coverlet.core/Instrumentation/InstrumenterResult.cs @@ -88,6 +88,7 @@ internal class HitCandidate public int start { get; set; } [DataMember] public int end { get; set; } + public HashSet AccountedByNestedInstrumentation { get; set; } } [DataContract] diff --git a/src/coverlet.core/Symbols/CecilSymbolHelper.cs b/src/coverlet.core/Symbols/CecilSymbolHelper.cs index 1baee3e38..409a351cc 100644 --- a/src/coverlet.core/Symbols/CecilSymbolHelper.cs +++ b/src/coverlet.core/Symbols/CecilSymbolHelper.cs @@ -21,6 +21,7 @@ internal class CecilSymbolHelper : ICecilSymbolHelper private const int StepOverLineCode = 0xFEEFEE; // Create single instance, we cannot collide because we use full method name as key private readonly ConcurrentDictionary _compilerGeneratedBranchesToExclude = new ConcurrentDictionary(); + private readonly ConcurrentDictionary> _sequencePointOffsetToSkip = new ConcurrentDictionary>(); // In case of nested compiler generated classes, only the root one presents the CompilerGenerated attribute. // So let's search up to the outermost declaring type to find the attribute @@ -395,6 +396,12 @@ private bool SkipGeneratedBranchesForExceptionHandlers(MethodDefinition methodDe return _compilerGeneratedBranchesToExclude[methodDefinition.FullName].Contains(instruction.Offset); } + // https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Expression%20Breakpoints.md + private bool SkipExpressionBreakpointsBranches(Instruction instruction) => instruction.Previous is not null && instruction.Previous.OpCode == OpCodes.Ldc_I4 && + instruction.Previous.Operand is int operandValue && operandValue == 1 && + instruction.Next is not null && instruction.Next.OpCode == OpCodes.Nop && + instruction.Operand == instruction.Next?.Next; + public IReadOnlyList GetBranchPoints(MethodDefinition methodDefinition) { var list = new List(); @@ -441,6 +448,11 @@ public IReadOnlyList GetBranchPoints(MethodDefinition methodDefinit } } + if (SkipExpressionBreakpointsBranches(instruction)) + { + continue; + } + if (SkipLambdaCachedField(instruction)) { continue; @@ -620,6 +632,10 @@ private static uint BuildPointsForSwitchCases(List list, BranchPoin return ordinal; } + public bool SkipNotCoverableInstruction(MethodDefinition methodDefinition, Instruction instruction) => + SkipNotCoverableInstructionAfterExceptionRethrowInsiceCatchBlock(methodDefinition, instruction) || + SkipExpressionBreakpointsSequences(methodDefinition, instruction); + /* Need to skip instrumentation after exception re-throw inside catch block (only for async state machine MoveNext()) es: @@ -660,7 +676,7 @@ await ... IL_00eb: br.s IL_00ed ... */ - public bool SkipNotCoverableInstruction(MethodDefinition methodDefinition, Instruction instruction) + public bool SkipNotCoverableInstructionAfterExceptionRethrowInsiceCatchBlock(MethodDefinition methodDefinition, Instruction instruction) { if (!IsMoveNextInsideAsyncStateMachine(methodDefinition)) { @@ -714,6 +730,44 @@ static Instruction GetPreviousNoNopInstruction(Instruction i) } } + private bool SkipExpressionBreakpointsSequences(MethodDefinition methodDefinition, Instruction instruction) + { + if (_sequencePointOffsetToSkip.ContainsKey(methodDefinition.FullName) && _sequencePointOffsetToSkip[methodDefinition.FullName].Contains(instruction.Offset) && instruction.OpCode == OpCodes.Nop) + { + return true; + } + /* + Sequence to skip https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Expression%20Breakpoints.md + // if (1 == 0) + // sequence point: (line 33, col 9) to (line 40, col 10) in C:\git\coverletfork\test\coverlet.core.tests\Samples\Instrumentation.SelectionStatements.cs + IL_0000: ldc.i4.1 + IL_0001: brtrue.s IL_0004 + // if (value is int) + // sequence point: (line 34, col 9) to (line 40, col 10) in C:\git\coverletfork\test\coverlet.core.tests\Samples\Instrumentation.SelectionStatements.cs + IL_0003: nop + // sequence point: hidden + ... + */ + if ( + instruction.OpCode == OpCodes.Ldc_I4 && instruction.Operand is int operandValue && operandValue == 1 && + instruction.Next?.OpCode == OpCodes.Brtrue && + instruction.Next?.Next?.OpCode == OpCodes.Nop && + instruction.Next?.Operand == instruction.Next?.Next?.Next && + methodDefinition.DebugInformation.GetSequencePoint(instruction.Next?.Next) is not null + ) + { + if (!_sequencePointOffsetToSkip.ContainsKey(methodDefinition.FullName)) + { + _sequencePointOffsetToSkip.TryAdd(methodDefinition.FullName, new List()); + } + _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Offset); + _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Next.Offset); + _sequencePointOffsetToSkip[methodDefinition.FullName].Add(instruction.Next.Next.Offset); + } + + return false; + } + private static bool SkipBranchGeneratedExceptionFilter(Instruction branchInstruction, MethodDefinition methodDefinition) { if (!methodDefinition.Body.HasExceptionHandlers) diff --git a/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs b/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs index 3824c2e15..367b9b592 100644 --- a/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs +++ b/test/coverlet.core.tests/Coverage/CoverageTests.SelectionStatements.cs @@ -85,5 +85,69 @@ public void SelectionStatements_Switch() File.Delete(path); } } + + [Fact] + public void SelectionStatements_Switch_CSharp8_OneBranch() + { + string path = Path.GetTempFileName(); + try + { + FunctionExecutor.Run(async (string[] pathSerialize) => + { + CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run(instance => + { + instance.SwitchCsharp8(int.MaxValue); + return Task.CompletedTask; + }, persistPrepareResultToFile: pathSerialize[0]); + return 0; + }, new string[] { path }); + + TestInstrumentationHelper.GetCoverageResult(path) + .Document("Instrumentation.SelectionStatements.cs") + .AssertLinesCovered(BuildConfiguration.Debug, 33, 34, 35, 36, 40) + .AssertLinesNotCovered(BuildConfiguration.Debug, 37, 38, 39) + .AssertBranchesCovered(BuildConfiguration.Debug, (34, 0, 1), (34, 1, 0), (34, 2, 0), (34, 3, 0), (34, 4, 0), (34, 5, 0)) + .ExpectedTotalNumberOfBranches(3); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public void SelectionStatements_Switch_CSharp8_AllBranches() + { + string path = Path.GetTempFileName(); + try + { + FunctionExecutor.Run(async (string[] pathSerialize) => + { + CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run(instance => + { + instance.SwitchCsharp8(int.MaxValue); + instance.SwitchCsharp8(uint.MaxValue); + instance.SwitchCsharp8(short.MaxValue); + try + { + instance.SwitchCsharp8(""); + } + catch { } + return Task.CompletedTask; + }, persistPrepareResultToFile: pathSerialize[0]); + return 0; + }, new string[] { path }); + + TestInstrumentationHelper.GetCoverageResult(path) + .Document("Instrumentation.SelectionStatements.cs") + .AssertLinesCovered(BuildConfiguration.Debug, 33, 34, 35, 36, 37, 38, 39, 40) + .AssertBranchesCovered(BuildConfiguration.Debug, (34, 0, 1), (34, 1, 3), (34, 2, 1), (34, 3, 2), (34, 4, 1), (34, 5, 1)) + .ExpectedTotalNumberOfBranches(3); + } + finally + { + File.Delete(path); + } + } } } \ No newline at end of file diff --git a/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs b/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs index e05c782b1..92da3b47e 100644 --- a/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs +++ b/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs @@ -320,6 +320,57 @@ public static Document AssertLinesCovered(this Document document, BuildConfigura return document; } + public static Document AssertLinesCovered(this Document document, BuildConfiguration configuration, params int[] lines) + { + return AssertLinesCoveredInternal(document, configuration, true, lines); + } + + public static Document AssertLinesNotCovered(this Document document, BuildConfiguration configuration, params int[] lines) + { + return AssertLinesCoveredInternal(document, configuration, false, lines); + } + + private static Document AssertLinesCoveredInternal(this Document document, BuildConfiguration configuration, bool covered, params int[] lines) + { + if (document is null) + { + throw new ArgumentNullException(nameof(document)); + } + + BuildConfiguration buildConfiguration = GetAssemblyBuildConfiguration(); + + if ((buildConfiguration & configuration) != buildConfiguration) + { + return document; + } + + List linesToCover = new List(lines); + foreach (KeyValuePair line in document.Lines) + { + foreach (int lineToCheck in lines) + { + if (line.Value.Number == lineToCheck) + { + if (covered && line.Value.Hits > 0) + { + linesToCover.Remove(line.Value.Number); + } + if (!covered && line.Value.Hits == 0) + { + linesToCover.Remove(line.Value.Number); + } + } + } + } + + if (linesToCover.Count != 0) + { + throw new XunitException($"Not all requested line found, {linesToCover.Select(l => l.ToString()).Aggregate((a, b) => $"{a}, {b}")}"); + } + + return document; + } + public static Document AssertNonInstrumentedLines(this Document document, BuildConfiguration configuration, int from, int to) { if (document is null) diff --git a/test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs b/test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs index 5f0fc3391..e182fffb9 100644 --- a/test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs +++ b/test/coverlet.core.tests/Samples/Instrumentation.SelectionStatements.cs @@ -28,5 +28,15 @@ public int Switch(int caseSwitch) return 0; } } + + public string SwitchCsharp8(object value) => + value + switch + { + int i => i.ToString(System.Globalization.CultureInfo.InvariantCulture), + uint ui => ui.ToString(System.Globalization.CultureInfo.InvariantCulture), + short s => s.ToString(System.Globalization.CultureInfo.InvariantCulture), + _ => throw new System.NotSupportedException() + }; } }