Implement Cobertura coverage format

[joeskeen]

PR Summary

Cobertura is a commonly used code coverage format, and has been requested as a feature: #2203

Fix #2203

To implement this feature, I copy-pasted the JaCoCo code and modified it to produce the required XML structure for Cobertura.

I am not an expert on the Cobertura format, but I did reference their DTD and the Cobertura output from my TypeScript project that uses jest-junit. If anyone is able to review it for correctness, I would greatly appreciate it. A good place to analyze the output is the unit test I added, where you can compare the JaCoCo output directly above it to the new Cobertura output (I used the same inputs for both reports).

PR Checklist

• PR has meaningful title
• Summary describes changes
• PR is ready to be merged
  • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
• Tests are added/update (if required)
• Documentation is updated/added (if required)

[joeskeen]

@eizedev Since you were the OP for issue #2298, would you please review?

[joeskeen]

Also, it seems that the XML attributes don't always have consistent ordering when the XML is serialized. This is currently failing the tests. Does anyone know how we could get past this?

https://nohwnd.visualstudio.com/Pester/_build/results?buildId=2226&view=logs&j=83ea85d6-f9df-5810-618d-ec9b0f05919b&t=863f8651-66b5-5008-0935-1e501bd49882&l=4971

##[error]  Expected: '...eforge.net/xml/coverage-04.dtd"[]><coverage branch-rate=...'
##[error]  But was:  '...eforge.net/xml/coverage-04.dtd"[]><coverage lines-covere...'

[fflaten]

Thanks for looking into this so fast! :)
This is more @nohwnd's territory but I'll give a review later. Left a quick comment for now.

[eizedev]

@eizedev Since you were the OP for issue #2298, would you please review?

I can not make it today to look at it, but try to look at it the next few days and test! Many, many thanks

[joeskeen]

After fixing all the build failures (related to multiple PS versions and OSs), I re-ran this branch to produce a Cobertura report against my other project and compared it to the HTML report I previously generated from the JaCoCo option. Everything seems to match up (whew!).

[eizedev]

@joeskeen just checked it, looks good to me! Again, thanks for your help and work.
I need to (and will) test it in our gitlab environment at work in the next 2 weeks to verify it, if gitlab can handle the cobertura coverage format.
if anyone else here could also test it, I would appreciate it.

[fflaten]

Please see comments below. As mentioned, I'm not an expert on coverage and haven't tested this format, so I might be wrong.

Still, it feels like line-elements are unnecessarily duplicated in the report and should only be reported once per method or class. Though it might work fine, it would cause large files

[joeskeen]

@fflaten thanks for the thorough review. I did some more testing yesterday and found that there were a lot of problems with my initial implementation. So yesterday I started from scratch and I'm liking it much better. I'll push the changes shortly after I address a couple linting issues.

[joeskeen]

@fflaten (or others) - my last remaining linting error is Where-Object usage:

The built-in *-Object-cmdlets are slow compared to alternatives in .NET. To fix a violation of this rule, consider using an alterantive like foreach-keyword etc.

What is the suggested alternative for Where-Object to comply with the linting and coding standards in this repository?

Edit: maybe this isn't an issue, I just found that Get-CoverageMissedCommands in the same file uses & $SafeCommands['Where-Object'] { $_.Breakpoint.HitCount -eq 0 } which produces the same warning, but that seems to be acceptable?

[joeskeen]

Looks like using System.Xml.Linq isn't OK for compatibility reasons; I'll rewrite those parts to use System.Xml instead.

[fflaten]

@fflaten (or others) - my last remaining linting error is Where-Object usage:

The built-in *-Object-cmdlets are slow compared to alternatives in .NET. To fix a violation of this rule, consider using an alterantive like foreach-keyword etc.

What is the suggested alternative for Where-Object to comply with the linting and coding standards in this repository?

Edit: maybe this isn't an issue, I just found that Get-CoverageMissedCommands in the same file uses & $SafeCommands['Where-Object'] { $_.Breakpoint.HitCount -eq 0 } which produces the same warning, but that seems to be acceptable?

A replacement depends on each scenario, but often foreach/for with if-condition.

If you can avoid them, that's great, but it's not critical in this code as it's only executed once in post-process. The rule is there to highlight potential performance issues in the runtime for code executed 100s or 1000s of times. 🙂

Please let me know when it's ready for review again.

[fflaten]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[fflaten]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[nohwnd]

Looks great, few nitpicks.

[nohwnd]

I was going to fix this PR but you beat me to it @fflaten .

Makes sense to backport to v5?

[fflaten]

I was going to fix this PR but you beat me to it @fflaten .

Felt this was a "you break it, you buy it" scenario 😄

Started testing it with ReportGenerator and got same HTML report with JaCoCo and Cobertura which is good. Haven't tested multiple source paths yet as I got distracted by weird codecov results like no coverage for BeLessThan (tested tst/functions against src/functions) in both formats.

Not too familiar with CC so please help testdrive it.

Makes sense to backport to v5?

Yes

[nohwnd]

I will try to test drive it, but not today. :)

And sorry @joeskeen for taking so long to merge this.

[fflaten]

Source for epoch time format: https://github.com/cobertura/cobertura/blob/0ff963284cecaace30f409a977dccb07c41a5a8f/cobertura/src/main/java/net/sourceforge/cobertura/reporting/xml/XMLReport.java#L89

Backport version will need to use:

$nineteenSeventy = & $SafeCommands['New-Object'] 'System.DateTime' -ArgumentList @(1970, 1, 1, 0, 0, 0, [System.DateTimeKind]::Utc)
$now = [DateTime]::Now.ToUniversalTime()
[long] $endTime = [math]::Floor(($now - $nineteenSeventy).TotalMilliseconds)
[long] $startTime = [math]::Floor($endTime - $TotalMilliseconds)

[fflaten]

@nohwnd I included this bugfix for JaCoCo as well. [DateTime]"01/01/1970" is not proper epoch origin (missing UTC kind), so off by one hour in my timezone.

Source for epoch time format:
https://github.com/jacoco/jacoco/blob/77d2af5ec31faf04419d945e1a0c34da49f8a702/org.jacoco.core/src/org/jacoco/core/data/SessionInfo.java#L33-L38

Backport version will need to use:

$nineteenSeventy = & $SafeCommands['New-Object'] 'System.DateTime' -ArgumentList @(1970, 1, 1, 0, 0, 0, [System.DateTimeKind]::Utc)
$now = [DateTime]::Now.ToUniversalTime()
[long] $endTime = [math]::Floor(($now - $nineteenSeventy).TotalMilliseconds)
[long] $startTime = [math]::Floor($endTime - $TotalMilliseconds)

[fflaten]

TODO: Need to update the hardcoded report in Coverage.Tests.ps1 when #2553 gets merged. Conflict.

[nohwnd]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[nohwnd]

TODO: Need to update the hardcoded report in Coverage.Tests.ps1 when #2553 gets merged. Conflict.

You were right, it is all broken now 😅

[fflaten]

Hard-coded reports = The gift that keeps on giving 🙏 Fixed and ready for test & review.