sebastianbergmann / phpunit

The PHP Unit Testing framework.
https://phpunit.de/
BSD 3-Clause "New" or "Revised" License
19.69k stars 2.2k forks source link

Deprecate `includeUncoveredFiles` configuration option #5951

Closed nostadt closed 1 month ago

nostadt commented 1 month ago

Hi Sebastian,

before I start, I want to give my appreciation for the time and effort you put into this project.

In the code coverage section (v11.3) of the documentation it is mentioned that it is highly recommended to keep the default (true) for includeUncoveredFiles. Otherwise the report is not complete and honest. That made me wonder, why can I disable it at all? Perhaps it can be removed, with the goal to reduce complexity.

Looking forward to hearing from you,

Alexander

sebastianbergmann commented 1 month ago

That made me wonder, why can I disable it at all? Perhaps it can be removed, with the goal to reduce complexity.

Good question and excellent point. Thank you for raising it!

Before php-code-coverage used static analysis (using PHP-Parser) to (also) analyse uncovered files, uncovered files were actually loaded using require. This resulted in any code outside of code units such as classes or functions to be executed. This in turn could cause problems and was the reason why it was made optional whether uncovered files should be processed or not.

So, yes, the includeUncoveredFiles option could have been removed a while ago.

nostadt commented 1 month ago

You're welcome. Thank you for the response and happy to hear that it can be removed nowadays.

mgleska commented 2 weeks ago

There is one important reason to leave includeUncoveredFiles as it was before - daily work performance.

In my daily work I often run coverage test for only one test file. To check if my recent update of test file covers all lines and branches in tested class. With includeUncoveredFiles=false, on coverage result index page I get only tested class and a few related files. With includeUncoveredFiles=true, on coverage result index page I get coverage result for whole system - mostly with negative coverage score.

For very simple system it looks like this: simple vs long

For real (big) system second list will be much longer. It is easy to see, that on longer list we get some (a lot of) unwanted information, which is useless in this context and disturb quick perception of coverage result of tested class.

@sebastianbergmann Please consider restoring includeUncoveredFiles in config file or adding a CLI option for to enable this functionality.

sebastianbergmann commented 2 weeks ago

@theseer, @localheinz, @staabm, @Schrank, @sebastianheuer, @Tesla91, and I discussed this issue during the PHPUnit Code Sprint in Munich today. We would like to understand why you look at the overview page when you seem to only be interested in the details of a single source file (or a subdirectory of the project root directory).

mgleska commented 2 weeks ago

@sebastianbergmann Because I am starting browsing of coverage results from index.html file.

Here is the content of coverage directory from my example (generated without includeUncoveredFiles=false):

/app/coverage # ls -p
Admin/                  CommonInfrastructure/   Kernel.php_branch.html  Printer/                _js/
Api/                    Customer/               Kernel.php_path.html    _css/                   dashboard.html
Auth/                   Kernel.php.html         Order/                  _icons/                 index.html

index.html is a natural starting point.

Now I see, that I can go to CommonInfrastructure directory and open index.html from this location. Before this discussion I never thought to do so.

OK. I can start browsing in specific directory. But there is another issue - performance. From my small demo project I get following execution time:

includeUncoveredFiles="false" includeUncoveredFiles="true"
[00:00.070] [00:02.256]

The src directory contains less then 100 files.

Is there any other option to not waste my time, CPU power and energy? How to not waste resources? I want to save the planet 😃