Implement Cobertura coverage format

[joeskeen]

PR Summary

Cobertura is a commonly used code coverage format, and has been requested as a feature: https://github.com/pester/Pester/issues/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

• [x] PR has meaningful title
• [x] Summary describes changes
• [x] 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.
• [x] Tests are added/update (if required)
• [x] 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.

[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.

[joeskeen]

@fflaten I think I've worked out all of the kinks as I was able to successfully use my branch of Pester in Azure Pipelines, generate a Cobertura report, and merge it with my existing JavaScript Cobertura report. All the numbers in the report are correct.

You may proceed to review :)

[joeskeen]

It seems that XML ordering has once again caused me issues in the tests. I'll have to sort methods by name to fix the output.

https://nohwnd.visualstudio.com/Pester/_build/results?buildId=2248&view=logs&j=bb0d7604-d5f0-5b90-4645-1a85813b357e&t=4aa45a65-ea08-550f-841b-d6edd0a30f97&l=5062

[fflaten]

Need to review tomorrow (hopefully), but immediately noticed some mismatches with the DTD.

<coverage>:

• Missing:

  <!ATTLIST coverage complexity       CDATA #REQUIRED>
  <!ATTLIST coverage version          CDATA #REQUIRED>
  <!ATTLIST coverage timestamp        CDATA #REQUIRED>

<package>:

• Missing:

  <!ATTLIST coverage complexity       CDATA #REQUIRED>
  <!ATTLIST coverage version          CDATA #REQUIRED>
  <!ATTLIST coverage timestamp        CDATA #REQUIRED>

<class>:

• Missing:

 <!ATTLIST class complexity  CDATA #REQUIRED>

<method>:

• Shouldn't have hits-attribute
• Missing:

  <!ATTLIST method line-rate   CDATA #REQUIRED>
  <!ATTLIST method branch-rate CDATA #REQUIRED>
  <!ATTLIST method complexity  CDATA #REQUIRED>

[joeskeen]

@fflaten Yeah, I noticed those discrepancies as well. I will need to do further investigation as I mostly modeled this after the output of the jest-junit Cobertura report, which uses the same DTD.

[joeskeen]

I finally found the source for my JavaScript Cobertura reports, not in jest-junit, but in the istanbul implementation: https://github.com/istanbuljs/istanbuljs/blob/596f6ff1342ae4baa6688bf3ee7786c75d4df947/packages/istanbul-reports/lib/cobertura/index.js

It seems that they use the DTD coverage-04.dtd but do not adhere to it per the spec. There is an alternative DTD, however, coverage-loose.dtd, that seems to be what they use there (and thus what I implemented). https://github.com/cobertura/cobertura/blob/master/cobertura/src/site/htdocs/xml/coverage-loose.dtd

I'll switch the declaration to use that one once I figure out the correct reference URL for it.

[joeskeen]

I tried just changing coverage-04.dtd to coverage-loose.dtd but that is apparently an invalid link. I couldn't find any updated link for it, so I tried changing it to the raw GitHub link to the file: https://raw.githubusercontent.com/cobertura/cobertura/master/cobertura/src/site/htdocs/xml/coverage-loose.dtd. I ran Pester to generate a report with that DTD and then plugged it into ReportGenerator. I was worried that it may not detect it as a Cobertura report, but it did:

2023-02-14T15:12:03: Loading report 'pester-coverage-cobertura.xml' 1/2 in memory
2023-02-14T15:12:03: Preprocessing report
2023-02-14T15:12:03: Initiating parser for Cobertura
2023-02-14T15:12:03: Current Assembly: build
2023-02-14T15:12:03: Current Assembly: setup
2023-02-14T15:12:03: Current Assembly: setup/functions
2023-02-14T15:12:03: Finished parsing 'pester-coverage-cobertura.xml' 1/2

So I think I'll go that route, as the current output is almost compliant with the cobertura-loose.dtd. I'll push those changes shortly.

[fflaten]

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. 🙂

Scratch that. We'll need to optimize this once it works as intended.

# running a stopwatch around Get-JaCoCoReportXml/Get-CoberturaReportXml call in Main.ps1
$c = New-PesterConfiguration
$c.Run.Path = './tst/'
$c.Run.ExcludePath = '*/demo/*', '*/examples/*', '*/testProjects/*', '*/Pester.Tests.ps1'
$c.Output.Verbosity = 'None'
$c.CodeCoverage.Enabled = $true
$c.CodeCoverage.Path = './src/'
$c.CodeCoverage.UseBreakpoints = $false

$c.CodeCoverage.OutputFormat = 'JaCoCo'
$r = Invoke-Pester -Configuration $c

WARNING: CC took 00:00:13.0819358

$c.CodeCoverage.OutputFormat = 'Cobertura'
$r = Invoke-Pester -Configuration $c

WARNING: CC took 00:02:14.9018360

A profiler-run highlights line-filters. The durations are multiplied due to the profiler, but percent + hitcount is correct.

So these don't scale well.

    $methodLineFilter = { $_.File -eq $classGroup.Name -and $_.Function -eq $methodGroup.Name }

    $coveredLines = $CoverageReport.HitCommands `
    | & $SafeCommands["Where-Object"] $methodLineFilter `
    | & $SafeCommands["Group-Object"] -Property Line `
    | New-LineNode

    $lines = $allLines `
    | & $SafeCommands["Where-Object"] $methodLineFilter `
    | & $SafeCommands["Group-Object"] -Property Line `
    | New-LineNode

# and

    $classLineFilter = { $_.File -eq $classGroup.Name }

    $coveredLines = $CoverageReport.HitCommands `
    | & $SafeCommands["Where-Object"] $classLineFilter `
    | & $SafeCommands["Group-Object"] -Property Line `
    | New-LineNode

    $lines = $allLines `
    | & $SafeCommands["Where-Object"] $classLineFilter `
    | & $SafeCommands["Group-Object"] -Property Line `
    | New-LineNode

[joeskeen]

Thanks for looking into that. I'll rework those areas, perhaps by doing a single pass through the $CoverageReport.HitCommands and $CoverageReport.MissedCommands to create class and method groupings.

[joeskeen]

@fflaten would you be able to run the profiler again?

[fflaten]

Amazing improvements! The stopwatch test went down to 3s (!) 🏎️ Also, running Profiler shows none of the code from this PR high on the list.

[fflaten]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[fflaten]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[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]

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