Implement Cobertura coverage format #2298

joeskeen · 2023-02-10T18:14:24Z

PR Summary

Cobertura is a commonly used code coverage format, and has been requested as a feature: #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 · 2023-02-10T18:17:08Z

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

joeskeen · 2023-02-10T18:33:07Z

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

src/functions/Coverage.ps1

fflaten · 2023-02-10T18:49:36Z

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 · 2023-02-10T19:00:06Z

@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 · 2023-02-10T21:07:37Z

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 · 2023-02-11T14:09:26Z

@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

src/Main.ps1

src/functions/Coverage.ps1

tst/functions/Coverage.Tests.ps1

src/functions/Coverage.ps1

joeskeen · 2023-02-14T15:10:02Z

@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 · 2023-02-14T16:22:11Z

@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 · 2023-02-14T16:44:45Z

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

src/functions/Coverage.ps1

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


src/functions/Coverage.ps1

+            $classLineFilter = { $_.File -eq $classGroup.Name -and -not $_.Function }
+
+            $coveredLines = $CoverageReport.HitCommands `
+            | & $SafeCommands["Where-Object"] $classLineFilter `


src/functions/Coverage.ps1

fflaten · 2023-02-14T16:58:22Z

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

src/functions/Coverage.ps1

Co-authored-by: Frode Flaten <3436158+fflaten@users.noreply.github.com>

joeskeen · 2023-02-14T18:18:27Z

@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 · 2023-02-14T18:56:59Z

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 · 2023-02-14T20:15:41Z

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 · 2023-02-14T20:19:47Z

@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 · 2023-02-14T22:01:18Z

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 · 2023-02-14T22:16:34Z

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 · 2023-02-14T22:25:40Z

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 · 2023-02-15T00:49:10Z

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 · 2023-02-15T12:32:43Z

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

fflaten · 2023-02-15T20:33:27Z

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

Awesome job! 🎉

I haven't tested this in a report generator or CI, so if someone's able to try it out with Gitlab that would be great.

Would also like @nohwnd to take a quick look. Be aware that this won't be merged until he finds some time, so might take a little while.

fflaten · 2023-02-15T20:41:18Z

src/functions/Coverage.ps1

+        }
+    }
+
+    $xmlDeclaration = '<?xml version="1.0" ?>'


Include encoding="UTF-8" like JaCoCo? Though I just noticed both hardcode this, while we actually make this configurable in CodeCoverage.OutputEncoding 😬

interesting... so what should we do? Hard-code or try to get it from the settings?

I'd hardcode for now to make sure it's compatible with Cobertura-tools. At least we'd be consistent and support unicode-characters properly.

Then we'll fix both later in a separate issue as it's more a xml-thing.

fflaten · 2023-02-15T20:46:47Z

src/functions/Coverage.ps1

+    }
+
+    $xmlDeclaration = '<?xml version="1.0" ?>'
+    $docType = '<!DOCTYPE coverage SYSTEM "https://raw.githubusercontent.com/cobertura/cobertura/master/cobertura/src/site/htdocs/xml/coverage-loose.dtd">'


@nohwnd: Should we point to this external URL or just coverage-loose.dtd and include the DTD-file in our build like the other output formats? I'm not familiar with the history of that decision.

I'm happy to do it whichever way is the preferred way. Would you like me to make that change or do we need to consult other maintainers?

I'm just a contributor, so lets wait on Jakub :)

fflaten · 2023-02-15T20:54:14Z

src/functions/Coverage.ps1

+    $xmlDeclaration = '<?xml version="1.0" ?>'
+    $docType = '<!DOCTYPE coverage SYSTEM "https://raw.githubusercontent.com/cobertura/cobertura/master/cobertura/src/site/htdocs/xml/coverage-loose.dtd">'
+    $coverageXml = ConvertTo-XmlElement -Node $coverage
+    $document = "$xmlDeclaration`n$docType`n$(([System.Xml.XmlElement]$coverageXml).OuterXml)"


Nit: Is the cast necessary?

Hmm... maybe not. I'll try without it.

joeskeen added 2 commits February 10, 2023 10:51

wip: Implement Cobertura coverage format

932d9b4

fix unit test

36af738

joeskeen mentioned this pull request Feb 10, 2023

Would anyone from the Cobertura community be available for a quick code review? cobertura/cobertura#431

Open

joeskeen added 2 commits February 10, 2023 11:21

revert editor settings change

14e36af

remove ?? operator

658b358

fflaten reviewed Feb 10, 2023

View reviewed changes

src/functions/Coverage.ps1 Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Feb 10, 2023

View reviewed changes

src/functions/Coverage.ps1 Fixed Show fixed Hide fixed

joeskeen added 4 commits February 10, 2023 12:07

fix attribute ordering

1f6525b

make coverage report test work on all platforms

a2e5eb0

Fix windows paths

61eefc1

fix unit test for Windows paths

6c3db56

kick the build

3b6e06b

fflaten requested changes Feb 13, 2023

View reviewed changes

re-implement Cobertura coverage report generation

14e8ef7

github-advanced-security bot found potential problems Feb 14, 2023

View reviewed changes

fix compatibility issues

ab0fa57

fflaten reviewed Feb 14, 2023

View reviewed changes

src/functions/Coverage.ps1 Outdated Show resolved Hide resolved

joeskeen added 3 commits February 14, 2023 10:36

fix tests

70ed47a

removing Cobertura from v4 parameter options

386cd3c

fix compatibility with ReportGenerator

f0ac943

Update src/functions/Coverage.ps1

a272120

Co-authored-by: Frode Flaten <3436158+fflaten@users.noreply.github.com>

joeskeen added 4 commits February 14, 2023 11:21

fix whitespace

f9ed4a0

Merge branch 'Cobertura' of github.com:joeskeen/Pester into Cobertura

c20f260

fix output

6a1ddc3

fix windows paths

9d78186

order packages,classes,methods by name

0bd5244

joeskeen mentioned this pull request Feb 14, 2023

Cobertura reporter does not respect the provided DTD istanbuljs/istanbuljs#603

Open

change Cobertura DTD to loose

719047a

Tune coverage report for performance

9116f06

joeskeen requested a review from fflaten February 15, 2023 12:47

fflaten reviewed Feb 15, 2023

View reviewed changes

fflaten added the Feature label Mar 30, 2023

fflaten added this to the 6.0.0 milestone May 13, 2024

fflaten mentioned this pull request May 13, 2024

CodeCoverage.OutputEncoding ignored in xml encoding-attribute #2450

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Cobertura coverage format #2298

Implement Cobertura coverage format #2298

joeskeen commented Feb 10, 2023 •

edited

joeskeen commented Feb 10, 2023

joeskeen commented Feb 10, 2023

fflaten commented Feb 10, 2023 •

edited

eizedev commented Feb 10, 2023

joeskeen commented Feb 10, 2023 •

edited

eizedev commented Feb 11, 2023 •

edited

fflaten left a comment •

edited

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023 •

edited

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023 •

edited

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023 •

edited

joeskeen commented Feb 15, 2023

joeskeen commented Feb 15, 2023

fflaten commented Feb 15, 2023 •

edited

fflaten left a comment

fflaten Feb 15, 2023

joeskeen Feb 15, 2023

fflaten Feb 18, 2023

fflaten Feb 15, 2023

joeskeen Feb 15, 2023

fflaten Feb 15, 2023

fflaten Feb 15, 2023

joeskeen Feb 15, 2023

Implement Cobertura coverage format #2298

Are you sure you want to change the base?

Implement Cobertura coverage format #2298

Conversation

joeskeen commented Feb 10, 2023 • edited

PR Summary

PR Checklist

joeskeen commented Feb 10, 2023

joeskeen commented Feb 10, 2023

fflaten commented Feb 10, 2023 • edited

eizedev commented Feb 10, 2023

joeskeen commented Feb 10, 2023 • edited

eizedev commented Feb 11, 2023 • edited

fflaten left a comment • edited

Choose a reason for hiding this comment

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023 • edited

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023 • edited

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

joeskeen commented Feb 14, 2023

fflaten commented Feb 14, 2023 • edited

joeskeen commented Feb 15, 2023

joeskeen commented Feb 15, 2023

fflaten commented Feb 15, 2023 • edited

fflaten left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeskeen commented Feb 10, 2023 •

edited

fflaten commented Feb 10, 2023 •

edited

joeskeen commented Feb 10, 2023 •

edited

eizedev commented Feb 11, 2023 •

edited

fflaten left a comment •

edited

joeskeen commented Feb 14, 2023 •

edited

fflaten commented Feb 14, 2023 •

edited

fflaten commented Feb 14, 2023 •

edited

fflaten commented Feb 15, 2023 •

edited