Skip to content
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

WIP: Add Cobertura report format #734

Closed
wants to merge 4 commits into from

Conversation

rask
Copy link

@rask rask commented Feb 26, 2020

Attempting to add Cobertura support, addressing #6, in case this is something we want to have.

The report should be valid against http://cobertura.sourceforge.net/xml/coverage-04.dtd, and I need to know how to represent the branch-rate and related data. Is that something the coverage collector can provide? line-rate and related are simple enough.

The tests are work in progress as well, need to add the example files for the other types of coverage reports as well.

Other notes:

Cobertura, as originating in the Java world, has no concept of code outside classes. This means we probably can't display "raw" PHP file code coverage (e.g. stuff like functions.php, index.php and so on) in these reports unless we mangle the data to fit report structure in some way.

@rask
Copy link
Author

rask commented Feb 26, 2020

Here's a rather long example of a Cobertura report from some Java project: https://gist.github.com/apetro/fcfffb8c4cdab2c1061d

Now to refactor and think about some aspects of the report that may have
to change.
@rask
Copy link
Author

rask commented Feb 28, 2020

Now it passes tests, but is ugly as heck. Next up is refactoring, but before that happens I would like to know the following:

  1. Are we OK including coverage data only for classes and traits, as the format is made for just those types of things? See example in the test that tests against ignored blocks of code: the function code is not reported in any way.

  2. Should the package element be a single element that contains the whole test suite for a package, or should be separate things to packages determined by namespaces in some way? E.g. if we have a package vendor/foo, is that the package we want? Or do we want a package for each and every subnamespace that contains a class or a trait?

As in:

<package name="Vendor\Pkg">
...
</package>
<package name="Vendor\Pkg\NamespaceA">
...
</package>
<package name="Vendor\Pkg\NamespaceB">
...
</package>
<package name="Vendor\Pkg\NamespaceA\Subnamespace">
...
</package>

Not sure what package stands for in the Java world exactly. Is it a single library/package/project? Or some separation inside libraries? Something else?

Also still not sure what to do with branch-* and related data in the report. Right now they all are zero, which is kind of correct I think?

@sebastianbergmann
Copy link
Owner

I do not need support for Cobertura (but am curious to learn why you need it) and do not have the time to do the research required to answer these questions, sorry.

*/
class CoberturaTest extends TestCase
{
public function testCloverForBankAccountTest(): void
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be testCoberturaForBankAccountTest instead of testCloverForBankAccountTest.

@samuelnogueira
Copy link

A Cobertura report would allow integrating 'modern' code coverage Jenkins plugins (ex. https://jenkins.io/blog/2018/08/17/code-coverage-api-plugin-1/), for those using it as CI.

A tool (or XSLT file) that would convert one of the already existing formats in phpunit into Cobertura, would also do the trick, but I couldn't find any.

IMO, it's just a nice to have.

LeSuisse added a commit to Enalean/tuleap that referenced this pull request Mar 5, 2020
…pelines

The plugin is a source of issue [0] which is unlikely to get some
attention soon [1]. It also sometimes crash when analyzing the reports
which completely fail the pipeline even if all the tests have been passed
successfully.

The Javascript now uses the modern approach to do code coverage in
Jenkins [2].
For the PHP code coverage the situation is less pretty. PHPUnit cannot
output a report in a format understood by the Jenkins plugin which means
we loose the high level overview. The HTML report is still browsable
from the archive and it looks like there is currently some traction to
add a format that would be supported by the Jenkins plugin [3] (we can
also consider writting a small tool to do the format conversion).

[0] jenkinsci/clover-plugin#21
[1] jenkinsci/clover-plugin#21 (comment)
[2] https://jenkins.io/blog/2018/08/17/code-coverage-api-plugin-1/
[3] sebastianbergmann/php-code-coverage#734

Change-Id: Ic4915047616a7023515939dd305a23f12f2300cc
@rask
Copy link
Author

rask commented Mar 6, 2020

Yes, a nice to have from my part as well, as it seems many popular test frameworks and tools in popular languages have Cobertura support of some sort. Mostly my motivation is to be able to do merging and managing coverage reports a little easier between languages and test suites inside larger code bases.

@rask
Copy link
Author

rask commented Mar 10, 2020

An idea for "non-OOP" coverage reporting with Cobertura:

Treat each file that contains PHP code to be reported upon as a "class" with the class name being the file name instead. Maybe put this behavior behind a flag bool $includeNonClasslikes or something similar.

So a functions.php might look like the following:

<coverage ...>
    <sources>
        ...
    </sources>
    <packages>
        <package ...>
            <class name="functions.php" filename="/path/to/functions.php" line-rate="0.5" ...>
                <methods>
                    <!-- the "method" below is actually just a bare function inside functions.php -->
                    <method name="hello_world" signature="hello_world(string $val) : string">
                         ...
                    </method>
                </methods>
                <lines>
                    <!-- these in addition to the faux methods above will report non-function code in files -->
                    <line num="1" hit="1" />
                    <line num="2" hit="1" />
                    <line num="3" hit="1" />
                    <line num="4" hit="2" />
                    <line num="5" hit="2" />
                    <line num="6" hit="2" />
                    ...
                </lines>
            </class>
        </package>
    </packages>
</coverage>

Also from the looks of the example report I linked earlier, the packages construct seems to be used for the stuff where PHP uses namespaces, so we could probably go with having a package per namespace when creating this report. I need to add a test case for this first though.

@johnwc
Copy link

johnwc commented Apr 20, 2020

@rask I would like to test out your updates for Cobertura reporting. How do I get your changes integrated and usable with PHPUnit?

@sebastianbergmann sebastianbergmann force-pushed the master branch 3 times, most recently from 999abec to 67341a5 Compare May 23, 2020 07:57
@cschomburg
Copy link

I'm also interested in this feature since GitLab chose Cobertura for their Test Coverage Visualization feature. The code in this pull request already works perfectly for this use case with only minor changes, and I'm using it in CI/CD.

A few comments:

  1. GitLab only collects the filename and line numbers for their report, so package/method structure is not that important for this use case.
  2. A package element for each namespace makes sense imho.
  3. The XML currently contains the absolute filepath of a package, I think GitLab expects this to be relative to project root.
  4. Traits currently throw notices regarding className
  5. Regarding code blocks / functions outside classes: The GitLab examples simply generate dummy method names / filenames for the package elements.
  6. I think branch coverage data is currently being worked on in master? But I'd suggest leaving branch-rate = 0 and fixing it in a later PR.

@johnwc I found it easiest to use a custom PHPUnit ResultPrinter to generate the report: Gist

@johnwc
Copy link

johnwc commented May 25, 2020

@xconstruct Until this update gets put in place and released, I have had to use this other tool in our CI pipeline to convert php unit's clover code coverage report into Cobertura.
https://github.com/danielpalme/ReportGenerator

Prior to running the conversion, I have to run a simple PS script that I wrote that moves all the flat file php files into a "Unstructured" package in the clover report. So that it can be better organized in the report between code that is in real class vs waterfall php code.

@aufalm
Copy link

aufalm commented Jun 11, 2020

I need cobertura report to publish code coverage results to Azure Pipelines. it's only support cobertura and JaCoCo formats.
https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/test/publish-code-coverage-results?view=azure-devops

@johnwc
Copy link

johnwc commented Jun 11, 2020

@aufalm look at my previous comment, there is a task in Azure DevOps that will convert it for you.

@aufalm
Copy link

aufalm commented Jun 11, 2020

@johnwc Thank you

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it.

@martindilling
Copy link

Attempting to add Cobertura support, addressing #6, in case this is something we want to have.

The report should be valid against http://cobertura.sourceforge.net/xml/coverage-04.dtd, and I need to know how to represent the branch-rate and related data. Is that something the coverage collector can provide? line-rate and related are simple enough.

The tests are work in progress as well, need to add the example files for the other types of coverage reports as well.

Other notes:

Cobertura, as originating in the Java world, has no concept of code outside classes. This means we probably can't display "raw" PHP file code coverage (e.g. stuff like functions.php, index.php and so on) in these reports unless we mangle the data to fit report structure in some way.

@rask Are you by any chance making this into a package or something? :)
Would be great to be able to get coverage reporting working in Azure

@johnwc
Copy link

johnwc commented Aug 8, 2020

@martindilling please review the entire thread, there is already a working solution to get it working in Azure.

@Hornet-Wing
Copy link

I'd like to second the request from @martindilling for a package. I'd like to use it for coverage in Gitlab pipelines.

@johnwc
Copy link

johnwc commented Aug 8, 2020

I'd like to second the request from @martindilling for a package. I'd like to use it for coverage in Gitlab pipelines.

If you can run bash in gitlab, you can use the same utility that I pointed out to convert the report to cobertura.

@martindilling
Copy link

@martindilling please review the entire thread, there is already a working solution to get it working in Azure.

I have, and I have gotten it to work. But I would prefer to have it handled by phpunit.

@Hornet-Wing
Copy link

Hornet-Wing commented Aug 9, 2020

Just in case I can save someone else some time, here's what I ended up with after a lot of searching, trial and error:

I did as @johnwc suggested and installed ReportGenerator (specifically dotnet-reportgenerator-globaltool) on my test docker container. Used it to convert the clover xml to Cobertura.xml.

  • phpunit --coverage-clover coverage.xml
  • reportgenerator "-reports:coverage.xml" "-targetdir:./" "-sourcedirs:./app" -reporttypes:Cobertura

The tricky part was then matching the filename and source to what GitLab expects. After a bit of trial and error it looks like it wants relative paths whereas phpunit -> ReportGenerator produces absolute paths.

So for my Laravel app the repository directory structure is:

my-project
└── app
    ├── User.php
    └── Http
        └── Controllers
            └── HomeController.php

And my pipeline runner would output something like:

<coverage>
  <sources>
    <source>./app</source>
  </sources>
  <packages>
    <package name="App" line-rate="1" branch-rate="1" complexity="NaN">
      <classes>
        <class name="/builds/my-group/my-project/app/User.php" filename="/builds/my-group/my-project/app/User.php" line-rate="1" branch-rate="1" complexity="NaN">
          ...
        </class>
      </classes>
    </package>
    <package name="App\Http\Controllers" line-rate="1" branch-rate="1" complexity="NaN">
      <classes>
        <class name="/builds/my-group/my-project/app/Http/Controllers/HomeController.php" filename="/builds/my-group/my-project/app/Http/Controllers/HomeController.php" line-rate="1" branch-rate="1" complexity="NaN">
          ...
        </class>
      </classes>
    </package>
    ...
  </package>
</coverage>

I found a couple of commands that would help to translate the output to what GitLab expects:

  1. Turn the filenames into relative paths from project root, in my case removing everything before (and including) my-project:
    sed -i "s;filename=\".*my-project\/;filename=\";g" Cobertura.xml
  2. Remove all paths from the name, leaving just the filename (not sure if this is actually required):
    sed -i "s; name=\"\S*\/; name=\";g" Cobertura.xml

Examples after the above with updates:
<class name="User.php" filename="app/User.php" line-rate="1" branch-rate="1" complexity="NaN">
<class name="HomeController.php" filename="app/Http/Controllers/HomeController.php" line-rate="1" branch-rate="1" complexity="NaN">

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Sep 15, 2020

Shawn McCabe wrote on Twitter:

why the decision not to allow cobertura support?

I was not aware that more people than @rask, who created this pull request, would be interested in this. Furthermore, the WIP: in the title as well as work in progress and ugly as heck in the comments did not inspire my confidence to merge this. I should have communicated this and I am sorry that I did not.

There seems to be enough interest to warrant the inclusion of this report into this library as well as into PHPUnit (CLI option, XML configuration).

I need to know how to represent the branch-rate and related data. Is that something the coverage collector can provide?

Since version 9 (used by PHPUnit 9.3), this library has support for line, branch, and path coverage. The data you need is available.

Cobertura, as originating in the Java world, has no concept of code outside classes. This means we probably can't display "raw" PHP file code coverage (e.g. stuff like functions.php, index.php and so on) in these reports unless we mangle the data to fit report structure in some way.

This is a limitation I can live with. We just need to make sure to explain it, in the documentation, for instance.

TL;DR: I will accept this when it is based on current master, is not considered ugly as heck by its developer, supports branch-rate, is well-tested, and conforms to Cobertura's DTD (really?, a DTD?; Are you sure they do not offer an XSD?).

@rask
Copy link
Author

rask commented Sep 21, 2020

Hello. Been super busy and have not had time to improve upon this. Sorry!

Someone else can freely go ahead and fork my work on this in case they are in a rush, otherwise I might pick this up later on.

Thanks!

(Also GitHub notifications are not working for me for some reason, so I did not see the discussion here.)

@Daniel-Mendes
Copy link

Daniel-Mendes commented Sep 21, 2020

We should create another pull request in sebastianbergmann/phpunit to add CLI option, XML configuration.

This could be something like:

  • For the CLI option
phpunit --coverage-cobertura cobertura.xml
  • For the XML configuration
<report>
    <cobertura outputFile="cobertura.xml"/>
</report>

We need to change phpunit.xsd too.

I have another question to we generate a generic cobertura.xml that works for all Azure, BitBucket, GitLab and GitHub (don't know if there is already a GitHub Action), or do we add a flag to specify the format we want like: phpunit --coverage-cobertura cobertura.xml --gitlab, like PHPStan does with Output Format.

@sebastianbergmann
Copy link
Owner

We should create another pull request in sebastianbergmann/phpunit to add CLI option, XML configuration.

I will gladly implement the configuration on the PHPUnit side of things once the Cobertura report generator has been implemented.

@johnwc
Copy link

johnwc commented Sep 21, 2020

@Daniel-Mendes What do you mean, with your last statement about "versions" of cobertura? Each of those hosting companies do not have their own version, they merely read the standard formatted cobertura xml doc.

@Daniel-Mendes
Copy link

Daniel-Mendes commented Sep 21, 2020

@Daniel-Mendes What do you mean, with your last statement about "versions" of cobertura? Each of those hosting companies do not have their own version, they merely read the standard formatted cobertura xml doc.

@johnwc I was talking about this comment, I know all platforms use the same standardized format but with specificities:

I'm also interested in this feature since GitLab chose Cobertura for their Test Coverage Visualization feature. The code in this pull request already works perfectly for this use case with only minor changes, and I'm using it in CI/CD.

A few comments:

  1. GitLab only collects the filename and line numbers for their report, so package/method structure is not that important for this use case.
  2. A package element for each namespace makes sense imho.
  3. The XML currently contains the absolute filepath of a package, I think GitLab expects this to be relative to project root.
  4. Traits currently throw notices regarding className
  5. Regarding code blocks / functions outside classes: The GitLab examples simply generate dummy method names / filenames for the package elements.
  6. I think branch coverage data is currently being worked on in master? But I'd suggest leaving branch-rate = 0 and fixing it in a later PR.

@johnwc I found it easiest to use a custom PHPUnit ResultPrinter to generate the report: Gist

@smmccabe
Copy link
Contributor

I've got a PR ready already for phpunit and I'm working on an updated version of this PR, I'll post both later today, not quite ready yet. Just wanted to let people know so no one ended up duplicating work.

@smmccabe
Copy link
Contributor

New PR here: #812

@johnwc
Copy link

johnwc commented Sep 22, 2020

@Daniel-Mendes that's not the issue with the sites, it's an issue with cobertura. As cobertura was designed to be used with java, that had only classes and namespaces. We just have to come up with a way to mimic that for php files that are more script based than class based. And this gist you linked is a dead link.

@sebastianbergmann
Copy link
Owner

Superseded by #812.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants