forceCoversAnnotations not respected in Util\Test::requiresCodeCoverageDataCollection()

[andrewnicols]

Q	A
PHPUnit version	master
PHP version	7.3.5
Installation Method	Git

If you have specified the forceCoversAnnotation setting in your configuration, I would expect this to influence the result of requiresCodeCoverageDataCollection() such that the Coverage Driver is only started if any coverage is required.

It appears that the configuration value is only used at the end of the run when generating the coverage report, and is ignored when starting the driver.

Related to #3697/#3699.

[codecov]

Codecov Report

Merging #3701 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3701      +/-   ##
============================================
- Coverage     82.79%   82.73%   -0.07%     
- Complexity     3755     3759       +4     
============================================
  Files           147      147              
  Lines          9911     9920       +9     
============================================
+ Hits           8206     8207       +1     
- Misses         1705     1713       +8

Impacted Files	Coverage Δ	Complexity Δ
src/Util/Test.php	86.61% <100%> (-0.13%)	222 <0> (+1)
src/Framework/TestCase.php	75.4% <100%> (-0.23%)	334 <2> (+2)
src/Framework/TestSuite.php	80.3% <100%> (+0.23%)	112 <1> (+1)	⬆️
src/Framework/ExceptionWrapper.php	88.23% <0%> (-11.77%)	16% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41a5260...50fb1bc. Read the comment docs.

[codecov]

Codecov Report

Merging #3701 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #3701      +/-   ##
============================================
- Coverage     82.79%   82.74%   -0.06%     
- Complexity     3755     3759       +4     
============================================
  Files           147      147              
  Lines          9911     9920       +9     
============================================
+ Hits           8206     8208       +2     
- Misses         1705     1712       +7

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/TestCase.php	75.4% <100%> (-0.23%)	334 <2> (+2)
src/Util/Test.php	86.79% <100%> (+0.04%)	222 <0> (+1)	⬆️
src/Framework/TestSuite.php	80.3% <100%> (+0.23%)	112 <1> (+1)	⬆️
src/Framework/ExceptionWrapper.php	88.23% <0%> (-11.77%)	16% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41a5260...872bfef. Read the comment docs.

[andrewnicols]

I feel that this change makes sense from a coverage performance perspective. When forceCoversAnnotation is set, coverage reports should require the @covers annotation, and therefore it only makes sense to start the coverage driver in this situation.

I'm not sure whether I have pulled the forceCoversAnnotation configuration correctly into the TestCase, but this did seem to be the most correct way of doing so from my understanding of the surrounding code.

Similar to the change in 3697/3699, I have modified the existing testCanSkipCoverage test. That test was creating a new instance of the testcase without specifying the name of the test, and therefore was missing the method annotations. As a result, it was only testing whether the @covers and @coversNothing was applied to the class, and not checking overrides on individual test methods.

I've updated those tests to check the testSomething test in each of those examples, and corrected the outcome.
Each of the existing tests did explicitly specify a @covers annotation on either the class, or the test method, so I have also added a new test which does not specify any annotations to test this properly.

Note: This does not affect the getLinesToBeCovered function or associated tests because that is only called after coverage has been generated, and is only passed the test class and name, and only inspects the annotations. It is not aware of the TestRunner configuration at all.

I suspect that it will be easier to merge this change before 3697/3699 and have that issue build upon the changes here.

[sebastianbergmann]

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

[andrewnicols]

Hi @sebastianbergmann ,

Thanks for your time to review this. It'd be good to understand your rationale for future reference, and to see if there is something else we can do instead. The @coversNothing change in #3697/#3699 should really help with our issues, but this would probably have been a more future-proof fix.

Thanks again for your time and dedication to this project.

[sebastianbergmann]

The @forceCoversAnnotations should not have anything to with whether or not code coverage data is collected.

[andrewnicols]

Thanks for the quick explanation.

The documentation for forceCoversAnnotation states:

Code Coverage will only be recorded for tests that use the @covers annotation documented in @covers.

Therefore I'd argue that it the presence of the configuration setting is entirely relevant to whether code coverage data should be collected.

With the setting enabled, coverage will not be generated regardless of whether or not it is collected, so what sense does it make to collect it?

I see in php-code-coverage that it is possible to ignore the forceCodeCoverage configuration when calling stop() on the coverage tool, but we don't ever pass this argument from phpunit when calling stop. If this were to change in future, then it would make sense to not ignore the coverage generation at the point that such a change were made.

Thanks again for your time.

[andrewnicols]

Hi @sebastianbergmann,

Don't mean to bug you but I realised you may not have notifications turned on and therefore may not have seen my previous comment as I forgot to cc you.

[andrewnicols]

Hi @sebastianbergmann ,

As I say, it would be extremely useful to have rationale as to this decision. I feel that I have justified why it makes sense to make this change, citing project documentation which states that this is meant to be the case, and providing rationale as to why it would make sense.

It is pointless to generate coverage data only to discard it completely unused.