-
-
Notifications
You must be signed in to change notification settings - Fork 467
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
Add Coverage to result object #1860
Conversation
Example of the output:
|
@@ -294,13 +294,14 @@ function Write-PesterReport { | |||
function Write-CoverageReport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to add comment-based help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's internal function, and it's a bit ugly that it writes screen and writes the report as well, but at least that makes they are the same. I would document only if the function is public.
@@ -614,6 +614,7 @@ function Get-CoverageReport { | |||
MissedCommands = $missedCommands | |||
HitCommands = $hitCommands | |||
AnalyzedFiles = $analyzedFiles | |||
CoveragePercent = if ($null -eq $CommandCoverage -or $CommandCoverage.Count -eq 0) { 0 } else { ($hitCommands.Count / $CommandCoverage.Count) * 100 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could string formatting be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid that because I don't want to force people to parse the value out if they need the raw value. They can add "%" easily, if they want.
@@ -1122,6 +1122,21 @@ function Invoke-Pester { | |||
$totalMilliseconds = $run.Duration.TotalMilliseconds | |||
$jaCoCoReport = Get-JaCoCoReportXml -CommandCoverage $breakpoints -TotalMilliseconds $totalMilliseconds -CoverageReport $coverageReport | |||
$jaCoCoReport | & $SafeCommands['Out-File'] $PesterPreference.CodeCoverage.OutputPath.Value -Encoding $PesterPreference.CodeCoverage.OutputEncoding.Value | |||
$reportText = Write-CoverageReport $coverageReport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this work be done outside of Pester.ps1, perhaps in Coverage? Invoke-Pester has a complexity metric of 348 according to Invoke-PsCodeHealth and could be split up? Might be another PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not happy about writing the screen and report at the same time. But for the moment it is imho okay.
|
||
public override string ToString() | ||
{ | ||
return CoveragePercent.ToString("N2 %"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would "P" work here instead of N2 %?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 1.000m in unit test with this and got N2 % back. Consider adding unit test to code with some cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -45,5 +47,10 @@ public static ErrorRecord CreateErrorRecord(string errorId, string message, stri | |||
var errorRecord = new ErrorRecord(exception, errorId, errorCategory, targetObject); | |||
return errorRecord; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, but I was considering using it. Will remove it later if I don't need it.
|
||
public decimal CoveragePercent { get; set; } | ||
|
||
public string CoverageReport { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be declared as nullable?
public string? CoverageReport { get; set; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project does not use nullability annotations.
Thanks for the comments. I merged the changes because I need them for my other fixes. How about the functionality? Looks to me like it works okay, apart from the percentage in the ToString? |
@nohwnd Late to the party, but have just one thought. Looks good, but just wonder what was you thought of having pre-formatted text in the property |
@johlju Not sure what you are asking. The report that is printed on the screen is echoed in the CoverageReport property. Plus, it always includes missing commands report, which I don't print to screen when verbosity is < Detailed. All the info that is used to construct that report is on the object as well. So your question is why I did it? |
Renamed the property on the result object from Coverage to CodeCoverage, in case someone is looking at this and have 5.2.0-alpha2 or newer. |
Added Coverage to the Result object, and printing of the report to screen. The same report is reported to the CoverageObject.
Fix #1625