Changed to calculate based on the average coverage of the modules #479

dlplenin · 2019-06-27T21:54:22Z

Fixes #346

Minor changes to calculate Average (console's table) based on average from all projects. Im not sure if it is ok only to modify CalculateLineCoverage(Modules modules) functions.
Also a change in CoverageResult.cs just to keep same logic, because its switch-case (ThresholdStatistic.Average) was working well (average based on average from all projects).

…dules. (coverlet-coverage#346)

…dules.2 (tonerdo#346)

MarcoRossignoli · 2019-06-28T07:15:23Z

Diego can you add some tests here https://github.com/tonerdo/coverlet/blob/master/test/coverlet.core.tests/CoverageSummaryTests.cs?

sasivishnu · 2019-06-28T16:14:31Z

Just a suggestion... could you use modules.Count instead of modules.Count()? since property already available to give the value. Linq Count() method use property Count anyway when type is of ICollection I believe.

sasivishnu · 2019-06-28T16:22:58Z

src/coverlet.console/Program.cs

                foreach (var _module in result.Modules)
                {
                    var linePercent = summary.CalculateLineCoverage(_module.Value).Percent;
                    var branchPercent = summary.CalculateBranchCoverage(_module.Value).Percent;
                    var methodPercent = summary.CalculateMethodCoverage(_module.Value).Percent;

-                    coverageTable.AddRow(Path.GetFileNameWithoutExtension(_module.Key), $"{linePercent}%", $"{branchPercent}%", $"{methodPercent}%");
+                    coverageTable.AddRow(Path.GetFileNameWithoutExtension(_module.Key), $"{linePercent.ToString("0.00")}%", $"{branchPercent.ToString("0.00")}%", $"{methodPercent.ToString("0.00")}%");


We don't need do ToString("0.00"). Percent property already takes care of rounding down to 2 decimal precision.

sasivishnu · 2019-06-28T16:32:18Z

src/coverlet.console/Program.cs

@@ -187,8 +191,8 @@ static int Main(string[] args)
                coverageTable.Rows.Clear();

                coverageTable.AddColumn(new[] { "", "Line", "Branch", "Method" });
-                coverageTable.AddRow("Total", $"{totalLinePercent}%", $"{totalBranchPercent}%", $"{totalMethodPercent}%");
-                coverageTable.AddRow("Average", $"{totalLinePercent / numModules}%", $"{totalBranchPercent / numModules}%", $"{totalMethodPercent / numModules}%");
+                coverageTable.AddRow("Total", $"{totalLinePercent.ToString("0.00")}%", $"{totalBranchPercent.ToString("0.00")}%", $"{totalMethodPercent.ToString("0.00")}%");


Same here... Percent property of CoverageDetails class takes care of rounding down.

sasivishnu · 2019-06-28T16:41:23Z

src/coverlet.core/CoverageDetails.cs

@@ -6,6 +6,7 @@ public class CoverageDetails
    {
        public double Covered { get; internal set; }
        public int Total { get; internal set; }
+        public double AverageModulePercent { get; internal set; }


Just a thought... can we do rounding down to 2 decimal precision here itself? In getter, we can return value after rounding down to 2 decimal precision at max. So to keep rounding decision at one place.

By this way, we can avoid doing .ToString("0.00") for displaying average at different place.

Upto @MarcoRossignoli @tonerdo for final thoughts though.

I agree with u, at first try I wrote with this approach, just more verbose than ToString("0.00"), but as u said we have different outputs if only use string format:

Ex: threshold value is "85.25" and lets say line value is "85.2463", then if condition will evaluate to true and we output coverage failed for average threshold. But while displaying, we print "85.25" because we do ToString("0.00") which will do normal rounding.

I realized about it, but I needed another oint of view, tks! I'll work on it

src/coverlet.core/CoverageResult.cs

sasivishnu · 2019-06-28T17:04:18Z

src/coverlet.core/CoverageSummary.cs

@@ -53,12 +53,15 @@ public CoverageDetails CalculateLineCoverage(Documents documents)
        public CoverageDetails CalculateLineCoverage(Modules modules)
        {
            var details = new CoverageDetails();
+            var accumPercent = 0.0;


Just to make it more obvious, can you do var accumPercent = 0.0D

sasivishnu · 2019-06-28T17:09:06Z

src/coverlet.console/Program.cs

@@ -172,13 +172,17 @@ static int Main(string[] args)
                var totalBranchPercent = summary.CalculateBranchCoverage(result.Modules).Percent;
                var totalMethodPercent = summary.CalculateMethodCoverage(result.Modules).Percent;

+                var averageLinePercent = summary.CalculateLineCoverage(result.Modules).AverageModulePercent;
+                var averageBranchPercent = summary.CalculateBranchCoverage(result.Modules).AverageModulePercent;
+                var averageMethodPercent = summary.CalculateMethodCoverage(result.Modules).AverageModulePercent;


We can reuse objects from line# 171, 172 and 173, then get total percent and average percent from those CoverageDetails object for each type line, branch and module respectively. This way we avoid doing recalculation and get some perf benefit. What you think?

yep, it need a refactoring

sasivishnu · 2019-06-28T17:17:09Z

@MarcoRossignoli I did review since I worked on this section last time. Let me know if you want to remove my comments. I am doing review first time in Github so I am not sure whether I did it right or messed up something.

MarcoRossignoli · 2019-06-28T18:19:41Z

Let me know if you want to remove my comments.

@sasivishnu absolutely NOT!
This project is OSS every idea review discussion on code are welcome!
Feel free to comment everything!
We won't remove any comments if constructive!
I think that @tonerdo agrees with me 😄

…rdo#346)

dlplenin · 2019-06-28T22:08:43Z

@MarcoRossignoli Tests added.
@sasivishnu I refactored according your suggestions, also I change the aligment for numbers (just visual thing), let me knot if it is ok or if it add complexity in vane.

Maybe I shouldnt 😬 coz:

///////////////////////////////////////////////////////////////////////////////////////////////////////////////
// ⚠ Do not modify this file. It will be replaced by a package reference once ConsoleTables has a strong name.
///////////////////////////////////////////////////////////////////////////////////////////////////////////////

but I saw a couple of commits...

MarcoRossignoli · 2019-06-29T07:13:34Z

test/coverlet.core.tests/CoverageSummaryTests.cs

@@ -75,8 +75,52 @@ private void SetupData()
            _modules.Add("module", documents);
        }

+        private void SetupAditionalData()


nit: rename SetupDataForAverageCalculation and call inside constructor CoverageSummaryTests one time togheter with others.

Rename is done,.... but if SetupDataForAverageCalculation is called into the constructor, previous tests will fail because initial data will have two modules.
I want to have two scnearios to test, one with one module(TestCalculateBranchCoverage_OneModule), and other with multiple modules (TestCalculateBranchCoverage_Modules).... what do u think?

I prefer custom private fields with custom setup for tests for instance

private Modules _averageCalculationSingleModule; private Modules _averageCalculationMultiModule;

and load in constructor.

Changed... pls take a look at line 123 (LMK if its ok) => I want to reuse data from SetupDataSingleModule

I see it's ok, some nits and LGTM

MarcoRossignoli · 2019-06-29T07:19:14Z

also I change the aligment for numbers (just visual thing), let me knot if it is ok or if it add complexity in vane.

Were there issues here?I don't think we should touch that code in this PR, better keep the focus on issue topic.
BTW can you attach screenshot with before/after to understand the difference?

MarcoRossignoli · 2019-06-29T07:21:29Z

src/coverlet.core/CoverageDetails.cs

+        private double averageModulePercent;
+        public double AverageModulePercent
+        {
+            get { return Math.Round(averageModulePercent, 2); ; }


nit: double token ;

Also, can you do return Math.Floor(averageModulePercent * 100)/100;

To keep consistency with the way Coverlet handles rounding. We round down using floor. You can refer Percent property below this line.

sasivishnu · 2019-06-29T10:11:19Z

also I change the aligment for numbers (just visual thing), let me knot if it is ok or if it add complexity in vane.

Were there issues here?I don't think we should touch that code in this PR, better keep the focus on issue topic.
BTW can you attach screenshot with before/after to understand the difference?

Yep, agree with @MarcoRossignoli here. No need to change any alignment as part of this PR. Please open new issue to discuss regarding your alignment proposal.

dlplenin · 2019-06-29T15:37:19Z

also I change the aligment for numbers (just visual thing), let me knot if it is ok or if it add complexity in vane.

Were there issues here?I don't think we should touch that code in this PR, better keep the focus on issue topic.
BTW can you attach screenshot with before/after to understand the difference?

There isnt a issue at all, it is just a visual aligment for number. It is not really important so I reverted the code.
Just: "My client asked me why number are aligned to left in CI report, ...I like that my clients value the use of tests"

sasivishnu · 2019-06-30T05:36:43Z

LGTM

…or (tonerdo#346)

MarcoRossignoli · 2019-07-02T08:10:56Z

src/coverlet.core/CoverageDetails.cs

@@ -6,6 +6,13 @@ public class CoverageDetails
    {
        public double Covered { get; internal set; }
        public int Total { get; internal set; }
+        private double averageModulePercent;


nit: move this field on top of class and change in _averageModulePercent

public class CoverageDetails { private double _averageModulePercent; public double Covered { get; internal set; } public int Total { get; internal set; } public double AverageModulePercent { get { return Math.Floor(_averageModulePercent * 100) / 100; } internal set { _averageModulePercent = value; } } public double Percent => Total == 0 ? 100D : Math.Floor((Covered / Total) * 10000) / 100; }

MarcoRossignoli · 2019-07-02T08:34:53Z

src/coverlet.core/CoverageResult.cs

-                        double line = 0;
-                        double branch = 0;
-                        double method = 0;
+                        var line = summary.CalculateLineCoverage(Modules).AverageModulePercent;


nit: double

Done... just one thing: lines 173 to 175 & 225 to 227 are using var, LMK if var o double is better, I can do a quick push if needed

yes please push fast update to keep style.

The rule I follow is "if the type is clear from assignment it's ok var but if it's not clear better make clean".

MarcoRossignoli · 2019-07-02T08:36:22Z

test/coverlet.core.tests/CoverageSummaryTests.cs

+
+            Branches branches = new Branches
+            {
+                new BranchInfo { Line = 1, Hits = 1, Offset = 1, Path = 0, Ordinal = 1 },//covered


nit: add space before comments } // covered everywhere

MarcoRossignoli

LGTM thank's Diego!
thank's also to @sasivishnu for help on review!

MarcoRossignoli · 2019-07-27T08:14:06Z

@tonerdo I merged this stale bug fix.
Feel free to revert if not in agreement.

Diego Pardo and others added 2 commits June 26, 2019 22:55

[Bug fix]Changed to calculate based on the average coverage of the mo…

c7f0179

…dules. (coverlet-coverage#346)

[Bug fix]Changed to calculate based on the average coverage of the mo…

4c1b6a0

…dules.2 (tonerdo#346)

MarcoRossignoli added the bug Something isn't working label Jun 28, 2019

MarcoRossignoli requested a review from tonerdo June 28, 2019 07:14

MarcoRossignoli added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) and removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Jun 28, 2019

sasivishnu reviewed Jun 28, 2019

View reviewed changes

[Bug fix] Added tests, refactoring and changed number alignment (tone…

2744f8e

…rdo#346)

MarcoRossignoli reviewed Jun 29, 2019

View reviewed changes

[Bug fix] Clean code and reverted ConsoleTable (aligment), (tonerdo#346)

492ecf8

[Bug fix] Refactoring: configuration data called inside the construct…

91e5a3c

…or (tonerdo#346)

MarcoRossignoli reviewed Jul 2, 2019

View reviewed changes

[Bug fix] Format code (tonerdo#346)

6522779

MarcoRossignoli approved these changes Jul 2, 2019

View reviewed changes

[Bug fix] Format code (tonerdo#346)

be3a72b

sasivishnu approved these changes Jul 2, 2019

View reviewed changes

MarcoRossignoli changed the title ~~[Bug fix]Changed to calculate based on the average coverage of the modules (tonerdo#346)~~ [Bug fix]Changed to calculate based on the average coverage of the modules Jul 3, 2019

MarcoRossignoli changed the title ~~[Bug fix]Changed to calculate based on the average coverage of the modules~~ Changed to calculate based on the average coverage of the modules Jul 3, 2019

MarcoRossignoli merged commit 4d61d29 into coverlet-coverage:master Jul 27, 2019

dlplenin mentioned this pull request Aug 2, 2019

[Bug fix] Put the same code that is in covelet.console to show the same result in 'average' (tonerdo#346) #509

Merged

MarcoRossignoli mentioned this pull request Sep 23, 2019

v5.2.0 was released without a ChangeLog #562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed to calculate based on the average coverage of the modules #479

Changed to calculate based on the average coverage of the modules #479

dlplenin commented Jun 27, 2019 •

edited by MarcoRossignoli

MarcoRossignoli commented Jun 28, 2019

sasivishnu commented Jun 28, 2019 •

edited

sasivishnu Jun 28, 2019 •

edited

sasivishnu Jun 28, 2019

sasivishnu Jun 28, 2019

dlplenin Jun 28, 2019

sasivishnu Jun 28, 2019

sasivishnu Jun 28, 2019

dlplenin Jun 28, 2019

sasivishnu commented Jun 28, 2019

MarcoRossignoli commented Jun 28, 2019 •

edited

dlplenin commented Jun 28, 2019 •

edited

MarcoRossignoli Jun 29, 2019 •

edited

dlplenin Jun 29, 2019

MarcoRossignoli Jul 1, 2019

dlplenin Jul 1, 2019

MarcoRossignoli Jul 2, 2019

MarcoRossignoli commented Jun 29, 2019

MarcoRossignoli Jun 29, 2019

sasivishnu Jun 29, 2019

dlplenin Jun 29, 2019

sasivishnu commented Jun 29, 2019

dlplenin commented Jun 29, 2019

sasivishnu commented Jun 30, 2019

MarcoRossignoli Jul 2, 2019

dlplenin Jul 2, 2019

MarcoRossignoli Jul 2, 2019

dlplenin Jul 2, 2019

MarcoRossignoli Jul 2, 2019

MarcoRossignoli Jul 2, 2019

dlplenin Jul 2, 2019

MarcoRossignoli Jul 2, 2019 •

edited

MarcoRossignoli left a comment

MarcoRossignoli commented Jul 27, 2019

Changed to calculate based on the average coverage of the modules #479

Changed to calculate based on the average coverage of the modules #479

Conversation

dlplenin commented Jun 27, 2019 • edited by MarcoRossignoli

MarcoRossignoli commented Jun 28, 2019

sasivishnu commented Jun 28, 2019 • edited

sasivishnu Jun 28, 2019 • 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

sasivishnu commented Jun 28, 2019

MarcoRossignoli commented Jun 28, 2019 • edited

dlplenin commented Jun 28, 2019 • edited

MarcoRossignoli Jun 29, 2019 • 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

MarcoRossignoli commented Jun 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasivishnu commented Jun 29, 2019

dlplenin commented Jun 29, 2019

sasivishnu commented Jun 30, 2019

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

MarcoRossignoli Jul 2, 2019 • edited

Choose a reason for hiding this comment

MarcoRossignoli left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented Jul 27, 2019

dlplenin commented Jun 27, 2019 •

edited by MarcoRossignoli

sasivishnu commented Jun 28, 2019 •

edited

sasivishnu Jun 28, 2019 •

edited

MarcoRossignoli commented Jun 28, 2019 •

edited

dlplenin commented Jun 28, 2019 •

edited

MarcoRossignoli Jun 29, 2019 •

edited

MarcoRossignoli Jul 2, 2019 •

edited