Coverage percentage output value rounded down with max 2 decimal precision #397

sasivishnu · 2019-05-04T18:52:54Z

Reports-After_Change.zip
Reports-Before_Change.zip

Related to issue #382

Change Details:

All percentage output are rounded down (Floor) with maximum of 2 decimal precision.
Arithmetic precision error fixed by rounding- ex: 0.1400D * 100 = 14.000000000000002
In Cobertura Report, "line-rate" and "branch-rate" value will have an extra decimal precision
Before: 0.143
After: 0.1432
In OpenCover report, "sequenceCoverage" and "branchCoverage" value will have an extra decimal precision
Before: 33.3
After: 33.33
In TeamCitry report, "CodeCoverageR" value will have an extra decimal precision
Before: 33.3
After: 33.33

I also updated and added new test cases to cover for the changes. Previously, we are rounding with max 3 decimal points before multiplying with 100. This yields value like 81.5%. I had to increase that to max 4 decimal points to yield like 81.52% when multiplied with 100%. In large projects, two decimal point reporting is useful and allows for incremental valuation without too much data loss after rounding down. Also I think, 2 decimal precision is commonly accepted standard.

Let me know If I missed something. Thanks

Edit: I have attached output for reports before my change (Coverlet.Console version 1.5.0) and after my change in zip.

Sample console output in version 1.5.0:

+-----------+-------+--------+--------+
| Module    | Line  | Branch | Method |
+-----------+-------+--------+--------+
| AppCenter | 13.8% | 25%    | 50%    |
+-----------+-------+--------+--------+

+---------+-------+--------+--------+
|         | Line  | Branch | Method |
+---------+-------+--------+--------+
| Total   | 13.8% | 25%    | 50%    |
+---------+-------+--------+--------+
| Average | 13.8% | 25%    | 50%    |
+---------+-------+--------+--------+

Sample console output after my change:

+-----------+--------+--------+--------+
| Module    | Line   | Branch | Method |
+-----------+--------+--------+--------+
| AppCenter | 13.79% | 25%    | 50%    |
+-----------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 13.79% | 25%    | 50%    |
+---------+--------+--------+--------+
| Average | 13.79% | 25%    | 50%    |
+---------+--------+--------+--------+

… precision.

codecov · 2019-05-04T19:00:38Z

Codecov Report

Merging #397 into master will increase coverage by 0.29%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   86.66%   86.96%   +0.29%     
==========================================
  Files          16       16              
  Lines        2130     2178      +48     
==========================================
+ Hits         1846     1894      +48     
  Misses        284      284

MarcoRossignoli · 2019-05-04T21:06:11Z

src/coverlet.core/CoverageDetails.cs

+            get => Math.Round(Total == 0 ? 1 : Covered / Total, 4);
+        }
+
+        public double GetCoveragePercentage()


Please use property CoveragePercentace { get }

Yeah, I can do that. Will convert method into prop CoveragePercentage { get }

MarcoRossignoli · 2019-05-04T21:07:52Z

src/coverlet.core/CoverageDetails.cs

+            return RoundDown(percentage);
+        }
+
+        private double RoundDown(double percentage)


Can you avoid RoundDown helper and use inline directly on CoveragePercentace prop, if I'm not mistaken this helper is private and used only in one method so it's not so useful extract it as helper.

Yes, its used only in one place as of now.

I forget to ask this earlier, Average Coverage Percentage value could go more than 2 decimal precision since its calculated inline.
coverageTable.AddRow("Average", $"{totalLinePercent / numModules}%", $"{totalBranchPercent / numModules}%", $"{totalMethodPercent / numModules}%");
Should I Floor the value with 2 decimal precision there also? Should I create MathHelper class under Helper folder, add this RoundDown method in the class & refer it in CoveragePercentage property and also during Average calculation? Is this fine?

@MarcoRossignoli I have done other changes and ready for check-in. Also before & after report formats output.

I am waiting to see your reply on what to do with "Average Percentage" calculation and then check-in code fully at once either way.

I did deeper review...and I think that we should change a bit the idea.
We should avoid to change CoverageDetail object only to "round data", I don't like the idea to have members with same meaning but only different "formatting" seems a duplication to me, I mean GetCoveragePercentage is Percent formatted.
I like the idea of double MathHelper.RoundPercentage(double) in a file MathHelper.cs under Helpers folder and apply this helper in every place we need, in this way we can also apply it on Average Coverage Percentage.
For instance

var totalLinePercent = MathHelper.RoundPercentage(summary.CalculateLineCoverage(result.Modules).Percent * 100); ... var method = MathHelper.RoundPercentage(summary.CalculateMethodCoverage(module.Value)); ... coverageTable.AddRow("Average", $"{MathHelper.RoundPercentage(totalLinePercent / numModules)}%",...

In this way we have also cetralized "percentage rounding" that we can apply in a uniform way in all codebase project.

could be cute to have an extension method

namespace Coverlet.Core.Helpers { public static MathHelper { public static double RoundPercentage(this double value) { ... } } ...

The idea of helper was for future possible use of rounding percentage everywhere in same way, btw I'm ok also with this with "fix" where we need like above.

I am planning to do both. Use Percent property only but add extension method in new MathHelper.cs class as public static double RoundPercentage(this double value).

So, it will be like Percent => { Total == 0? 1: ((Covered/Total)*100).RoundPercentage() }

@sasivishnu avoid helper at all, less code to review and maintain, if in future we'll need to reuse we'll move to helper, put all round alg in prop getter.

Thanks @sasivishnu, let me know when you'd done this so I can merge. The rest of the code seems good to me

@tonerdo Yep, I am done with my changes and pushed latest one.

MarcoRossignoli · 2019-05-04T21:11:45Z

@sasivishnu can you please run before/after coverage with all format and show result to PR(like comment or attaching files) we need to be sure that reports are ok.

sasivishnu · 2019-05-05T04:45:18Z

@sasivishnu can you please run before/after coverage with all format and show result to PR(like comment or attaching files) we need to be sure that reports are ok.

@MarcoRossignoli Yeah, I can do that. I will attach before and after reports for all five formats with console output too.

tonerdo · 2019-05-05T23:14:30Z

test/coverlet.core.tests/Reporters/OpenCoverReporterTests.cs

@@ -17,13 +17,13 @@ public void TestReport()
            result.Identifier = Guid.NewGuid().ToString();

            result.Modules = new Modules();
-            result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());
+            result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());            


Suggested change

result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());

result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());

tonerdo · 2019-05-05T23:20:19Z

src/coverlet.core/CoverageDetails.cs

@@ -8,7 +8,18 @@ public class CoverageDetails
        public int Total { get; internal set; }
        public double Percent
        {
-            get => Math.Round(Total == 0 ? 1 : Covered / Total, 3);
+            get => Math.Round(Total == 0 ? 1 : Covered / Total, 4);


All rounding down and decimal place code should go here

tonerdo · 2019-05-05T23:21:20Z

src/coverlet.console/Program.cs

-                var totalLinePercent = summary.CalculateLineCoverage(result.Modules).Percent * 100;
-                var totalBranchPercent = summary.CalculateBranchCoverage(result.Modules).Percent * 100;
-                var totalMethodPercent = summary.CalculateMethodCoverage(result.Modules).Percent * 100;
+                var totalLinePercent = summary.CalculateLineCoverage(result.Modules).GetCoveragePercentage();


This can just be var totalLinePercent = summary.CalculateLineCoverage(result.Modules).Percent if you update the Percent property to contain the necessary calculations. Applies everywhere this new method is used

… percentage value with max two decimal precision.

…Reporter

sasivishnu · 2019-05-06T15:26:14Z

@sasivishnu can you please run before/after coverage with all format and show result to PR(like comment or attaching files) we need to be sure that reports are ok.

@MarcoRossignoli I have attached before and after sample report in first comment

MarcoRossignoli · 2019-05-06T15:37:09Z

src/coverlet.core/CoverageDetails.cs

-        {
-            get => Math.Round(Total == 0 ? 1 : Covered / Total, 3);
-        }
+        public double Percent => Total == 0 ? 1D : Math.Floor((Covered / Total) * 10000) / 100;


should be 100D?

Yeah, good catch. Thanks. Will push the new commit with fix

MarcoRossignoli · 2019-05-06T15:50:16Z

Files compared and looks ok to me there is only expected extra decimal for cobertura line-rate="0.1379"

MarcoRossignoli

LGTM thank's a lot @sasivishnu!

tonerdo

LGTM!

Coverage percentage output value is rounded down with max two decimal…

2bc7f0a

… precision.

MarcoRossignoli requested a review from tonerdo May 4, 2019 20:41

MarcoRossignoli reviewed May 4, 2019

View reviewed changes

MarcoRossignoli requested a review from petli May 4, 2019 21:14

MarcoRossignoli mentioned this pull request May 5, 2019

Uniform helper namespace #399

Merged

tonerdo reviewed May 5, 2019

View reviewed changes

sasivishnu added 2 commits May 6, 2019 20:40

Modifying Percent property in coverage details to return rounded down…

9586b91

… percentage value with max two decimal precision.

Wrongly called Line coverage instead of branch coverage in Cobertura …

66dad3b

…Reporter

MarcoRossignoli reviewed May 6, 2019

View reviewed changes

When no line exists, Percent should return default percentage value 100.

2201898

MarcoRossignoli approved these changes May 6, 2019

View reviewed changes

tonerdo approved these changes May 7, 2019

View reviewed changes

tonerdo merged commit 7cfe07e into coverlet-coverage:master May 7, 2019

MarcoRossignoli mentioned this pull request May 8, 2019

0% branch coverage when code has no branches #261

Closed

MarcoRossignoli mentioned this pull request May 29, 2019

Issues with NUnit #449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coverage percentage output value rounded down with max 2 decimal precision #397

Coverage percentage output value rounded down with max 2 decimal precision #397

sasivishnu commented May 4, 2019 •

edited

Loading

codecov bot commented May 4, 2019 •

edited

Loading

MarcoRossignoli May 4, 2019

sasivishnu May 5, 2019 •

edited

Loading

MarcoRossignoli May 4, 2019 •

edited

Loading

sasivishnu May 5, 2019

sasivishnu May 5, 2019 •

edited

Loading

MarcoRossignoli May 5, 2019 •

edited

Loading

MarcoRossignoli May 5, 2019 •

edited

Loading

MarcoRossignoli May 6, 2019

sasivishnu May 6, 2019 •

edited

Loading

MarcoRossignoli May 6, 2019 •

edited

Loading

tonerdo May 6, 2019

sasivishnu May 6, 2019

MarcoRossignoli commented May 4, 2019 •

edited

Loading

sasivishnu commented May 5, 2019

tonerdo May 5, 2019

tonerdo May 5, 2019

tonerdo May 5, 2019

sasivishnu commented May 6, 2019

MarcoRossignoli May 6, 2019

sasivishnu May 6, 2019

MarcoRossignoli commented May 6, 2019 •

edited

Loading

MarcoRossignoli left a comment

tonerdo left a comment

	result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());
	result.Modules.Add("Coverlet.Core.Reporters.Tests", CreateFirstDocuments());

Coverage percentage output value rounded down with max 2 decimal precision #397

Coverage percentage output value rounded down with max 2 decimal precision #397

Conversation

sasivishnu commented May 4, 2019 • edited Loading

codecov bot commented May 4, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sasivishnu May 5, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasivishnu May 5, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli May 5, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli May 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasivishnu May 6, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 4, 2019 • edited Loading

sasivishnu commented May 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasivishnu commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 6, 2019 • edited Loading

MarcoRossignoli left a comment

Choose a reason for hiding this comment

tonerdo left a comment

Choose a reason for hiding this comment

sasivishnu commented May 4, 2019 •

edited

Loading

codecov bot commented May 4, 2019 •

edited

Loading

sasivishnu May 5, 2019 •

edited

Loading

MarcoRossignoli May 4, 2019 •

edited

Loading

sasivishnu May 5, 2019 •

edited

Loading

MarcoRossignoli May 5, 2019 •

edited

Loading

MarcoRossignoli May 5, 2019 •

edited

Loading

sasivishnu May 6, 2019 •

edited

Loading

MarcoRossignoli May 6, 2019 •

edited

Loading

MarcoRossignoli commented May 4, 2019 •

edited

Loading

MarcoRossignoli commented May 6, 2019 •

edited

Loading