Branch Enhancements (take 2) #73

hunterjm · 2018-05-02T01:30:04Z

Description

This PR covers a lot of changes in order to better support branch coverage. The main purpose of this refactor is two-fold. First, to add improved support for branch coverage. Second, to improve the output of the 3 supported report formats by adding previously missing information.

At a high level, this PR covers the objectives by:

Resolving Enhanced Branch Coverage #67.
- Updated all report generators with new branch logic.
Changing Coverage Calculation from average of averages to a single average of the sums of covered/total points (for line, branch, and method).
- Utilized the new CoverageDetails result model from CoverageSummary in report generators in lieu of manually tracking and incrementing visits and counts.

Note

I re-forked and re-applied the changed from #69 because I managed to royally screw up the git history in that fork. What that means is I've effectively squashed all those commits into one, so you don't get to see my fun debugging journey, and for that - I'm sorry.

…l points instead of averages

tonerdo · 2018-05-02T08:59:52Z

No worries. Will take a look later today

tonerdo

Thanks for taking the time out to work on this in detail. Just a couple of changes and we are good to go. This is going into the 2.0 release because of the breaking changes in the json format

tonerdo · 2018-05-05T14:54:55Z

src/coverlet.core/CoverageSummary.cs

@@ -4,149 +4,232 @@

 namespace Coverlet.Core
 {
+    public class CoverageDetails


Let's move this to its own file CoverageDetails.cs

Also make Covered and Total internal set

tonerdo · 2018-05-05T14:57:19Z

src/coverlet.core/CoverageSummary.cs

+    {
+        public double Covered { get; set; }
+        public int Total { get; set; }
+        public double Percent { get; set; }


This should only be "gettable". Do the percent calculation in the get{} block

tonerdo · 2018-05-05T14:58:10Z

src/coverlet.core/CoverageSummary.cs

+            details.Covered = lines.Where(l => l.Value.Hits > 0).Count();
+            details.Total = lines.Count;
+            double coverage = details.Total == 0 ? details.Total : details.Covered / details.Total;
+            details.Percent = Math.Round(coverage, 3);


Move this an all other Percent calculations to the get block of this property

tonerdo · 2018-05-05T15:17:57Z

src/coverlet.core/Instrumentation/Instrumenter.cs

+                    index += 3;
+                }
+
+                if (targetedBranchPoints.Count() > 0)


There doesn't seem to be a reason to check the count here. If it's empty the next loop simply won't run

tonerdo · 2018-05-05T15:22:10Z

src/coverlet.core/Instrumentation/Instrumenter.cs

@@ -143,8 +171,8 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor
                    document.Lines.Add(new Line { Number = i, Class = method.DeclaringType.FullName, Method = method.FullName });
            }

-            string flag = IsBranchTarget(processor, instruction) ? "B" : "L";
-            string marker = $"{document.Path},{sequencePoint.StartLine},{sequencePoint.EndLine},{flag}";
+            // string flag = branchPoints.Count > 0 ? "B" : "L";


No need for this line

tonerdo · 2018-05-05T15:27:21Z

src/coverlet.core/Symbols/CecilSymbolHelper.cs

+    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);


Can you give an example of text that'll match this pattern

Nit: Empty line after this

OpenCover/opencover#310

tonerdo · 2018-05-05T15:32:14Z

src/coverlet.core/Symbols/CecilSymbolHelper.cs

+            }
+            catch (Exception ex)
+            {
+                throw new InvalidOperationException(


We should have a sane fallback instead of throwing an exception.

This is the only change I haven't yet made from this review. Do you have any suggestions? It's just catching any other exception that gets thrown and giving it a more descriptive message. This was pulled directly from OpenCover's CecilSymbolManager.

Perhaps put the try...catch inside the loop and simply continue to ignore the error. I don't see much point throwing an exception because the developer most likely won't know what/how to fix it. IMO better to have some missing coverage info than to have the entire thing fail with no directions on how to fix it. Also if this file was pulled from OpenCover I think it'll be right to attribute it to the project

The first 4 lines of this file include that attribution.

tonerdo · 2018-05-05T15:32:57Z

src/coverlet.core/Symbols/CecilSymbolHelper.cs

+                    var branchingInstructionLine = closestSeqPt.Maybe(x => x.StartLine, -1);
+                    var document = closestSeqPt.Maybe(x => x.Document.Url);
+
+                    if (null == instruction.Next)


Nit: instruction.Next == null

tonerdo · 2018-05-05T15:40:40Z

src/coverlet.core/Symbols/CecilSymbolHelper.cs

+            GetBranchPoints(methodDefinition, list);
+            return list;
+        }
+        private static void GetBranchPoints(MethodDefinition methodDefinition, List<BranchPoint> list)


Why not just make this method signature public static List<BranchPoint> GetBranchPoints(MethodDefinition methodDefinition) and initialize the BranchPoint list in it

It reduces the number of times we have to return the list if we need to short circuit the method. Other than that, it's the structure that existed in OpenCover: https://github.com/OpenCover/opencover/blob/master/main/OpenCover.Framework/Symbols/CecilSymbolManager.cs#L351

I can still change it if you'd like.

I'll appreciate if you change it, thanks

hunterjm · 2018-05-05T17:30:07Z

All requested changes completed.

…nhancements Branch Enhancements (take 2)

Bumps [coverlet.collector](https://github.com/coverlet-coverage/coverlet) from 1.3.0 to 3.0.1. #Release notes *Sourced from [coverlet.collector's releases](https://github.com/coverlet-coverage/coverlet/releases).* > ## v3.0.0 > * [#131](coverlet-coverage/coverlet#131) makes a slight change to the Coverlet JSON format > * 807f7b1bd5bea8158ffff343d5511cd16e0da9a0 uses a separate `coverlet.tracker` assembly to hold tracking code > * [#128](coverlet-coverage/coverlet#128) adds support for assemblies with `.exe` extension > * a1f18b4156374f3398d704e898ec58c7c6c64bf8 improves identifying compiler generated types > * [#134](coverlet-coverage/coverlet#134) adds considerable coverage tracking performance improvements > > ## v2.0.1 > * [#102](coverlet-coverage/coverlet#102) fixes issues with NUNIT3 Test adapter ([#101](coverlet-coverage/coverlet#101)) > * [#104](coverlet-coverage/coverlet#104) shows overall averages as part of final console output > * [#112](coverlet-coverage/coverlet#112) adds support for standard `ExcludeFromCodeCoverage` attribute to specify types and methods to exclude from code coverage. Deprecates `ExcludeFromCoverage` attribute > * coverlet-coverage/coverlet@7f190e4 prevents Opencover and Cobertura output generated at the same time from overwriting each other ([#111](coverlet-coverage/coverlet#111)) > * [#116](coverlet-coverage/coverlet#116) strongly signs the Coverlet assembly and aims to fix [#40](coverlet-coverage/coverlet#40) > > ## v2.0.0 > * [#78](coverlet-coverage/coverlet#78) adds support for generating multiple report formats in a single run > * [#73](coverlet-coverage/coverlet#73) improves branch coverage support and output formats* > * coverlet-coverage/coverlet@d2effb3 shows method coverage in summary output > * [#88](coverlet-coverage/coverlet#88) improves disk usage by using gzip compression > * [#93](coverlet-coverage/coverlet#93) adds `ThresholdType` property that allows you to specify the coverage type to apply the `Threshold` property to > * coverlet-coverage/coverlet@ebedd70 renames `Exclude` property to `ExcludeByFile`* > * coverlet-coverage/coverlet@9ed0864 supports using filter expressions to exclude assemblies, namespaces or types. Uses the `Exclude` property* > * [#99](coverlet-coverage/coverlet#99) adds improvements to evaluation of filter expressions > > `*` - Backwards incompatible change #Commits - See f...

add better branch support, change coverage calculation to be on actua…

89791e6

…l points instead of averages

This was referenced May 4, 2018

Coverlet Coverage does not match coverage reported from Report Generator reports #84

Closed

Complex branches are reported incorrectly #79

Closed

tonerdo requested changes May 5, 2018

View reviewed changes

hunterjm added 2 commits May 5, 2018 12:51

changes per feedback

85fa0f3

make requested changes to symbol helper

7b6e9d7

tonerdo approved these changes May 5, 2018

View reviewed changes

tonerdo merged commit 2597da9 into coverlet-coverage:master May 5, 2018

hunterjm deleted the feature/branchEnhancements branch May 5, 2018 18:01

tonerdo mentioned this pull request May 5, 2018

Coverage report which contains a completely excluded project with wrong percentage, affects the threshold requirements as well #74

Closed

NorekZ pushed a commit to NorekZ/coverlet that referenced this pull request Nov 8, 2018

Merge pull request coverlet-coverage#73 from hunterjm/feature/branchE…

ac529fb

…nhancements Branch Enhancements (take 2)

dependabot bot mentioned this pull request Mar 18, 2021

Bump coverlet.collector from 1.3.0 to 3.0.3 luojunyuan/Ve.DotNet#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branch Enhancements (take 2) #73

Branch Enhancements (take 2) #73

hunterjm commented May 2, 2018

tonerdo commented May 2, 2018

tonerdo left a comment •

edited

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

hunterjm May 5, 2018

tonerdo May 5, 2018

hunterjm May 5, 2018 •

edited

tonerdo May 5, 2018

hunterjm May 5, 2018

tonerdo May 5, 2018

tonerdo May 5, 2018

hunterjm May 5, 2018 •

edited

tonerdo May 5, 2018

hunterjm commented May 5, 2018

Branch Enhancements (take 2) #73

Branch Enhancements (take 2) #73

Conversation

hunterjm commented May 2, 2018

Description

Note

tonerdo commented May 2, 2018

tonerdo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunterjm May 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunterjm May 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunterjm commented May 5, 2018

tonerdo left a comment •

edited

hunterjm May 5, 2018 •

edited

hunterjm May 5, 2018 •

edited