Be strict about unintentional test coverage

[Xerkus]

Tests in zend framework are not using @covers annotations and create unintentional code coverage which allows some parts of code to remain untested.

I believe forcing covers annotation will make coverage metric more precise and reliable and allow us to further increase code quality.

[Xerkus]

There are two ways to declare coverage, strict:

namespace ZendTest\Router;

use PHPUnit\Framework\TestCase;
use Zend\Router\TreeRouteStack;

/**
 * We declare default class so that it could be omitted in @covers annotation
 * @coversDefaultClass \Zend\Router\TreeRouteStack
 * Tests will cover all non-public methods by default
 * @covers ::<!public>
 */
class TreeRouteStackTest extends TestCase
{
    /**
     * Note that we will record coverage for TreeRouteStack::addRoute here,
     * but not for parent SimpleRouteStack::addRoute
     * @covers ::addRoute
     * public method getRoute will not be recorded as covered by this test
     */
    public function testAddRouteAcceptsTraversable()
    {
        $stack = new TreeRouteStack();
        $stack->addRoute('foo', new \ArrayIterator([
            'type' => TestAsset\DummyPartialRoute::class
        ]));
        $this->assertNotNull($stack->getRoute('foo'));
    }
}

Or lax:

namespace ZendTest\Router;

use PHPUnit\Framework\TestCase;
use Zend\Router\TreeRouteStack;

/**
 * Coverage is for any method of tested class 
 * @covers \Zend\Router\TreeRouteStack
 */
class TreeRouteStackTest extends TestCase
{
    /**
     * addRoute will be covered without additional annotations, 
     * but again, not SimpleRouteStack::addRoute
     */
    public function testAddRouteAcceptsTraversable()
    {
        $stack = new TreeRouteStack();
        $stack->addRoute('foo', new \ArrayIterator([
            'type' => TestAsset\DummyPartialRoute::class
        ]));
        $this->assertNotNull($stack->getRoute('foo'));
    }
}

[Xerkus]

I think second approach will be good enough. Not too granular, no unrelated coverage.

[weierophinney]

I'm not 100% convinced we need this. Test coverage, while a valuable metric, is not one I want us to specifically target.

As an example, I'm working on test coverage for a new library currently. I'm less worried about things like value objects, but very worried about coverage of methods that are part of the primary domain of the package. My estimate, once I've completed coverage of these core considerations, is that coverage will be around 80-85%.

Adding @covers annotations likely will not affect this particular library, or could negatively affect coverage, as some of those value objects are implicitly covered currently due to being exercised by the primary API.

It may be worth writing up guidelines for how to work with coverage reports and properly report coverage, but I don't think they should be requirements.

[Xerkus]

Coverage % metric is not important. Record of the code being executed does not mean anything. It means only that code had no php errors, not that its logic is correct and it was asserted.
The lack of coverage is what actually important. Accidental coverage prevents us from discovering code that is not tested thoroughly or not tested at all. There might be a bug that is compensated somehow by the consumer code or its tests. You provide mock to return something you need and invalid result of another dependency just gets swallowed because you build test not for checking for correctness of that dependency.

You can cover more than one class, if it makes sense, but it have to be explicit. You declare that you actually had it in mind and take responsibility for it.

https://github.com/Xerkus/zend-router/blob/9481f2643cb81442d89697f126014f0d392c87fd/test/RouteResultTest.php#L16-L20

Upd: Wasn't it supposed to be inlined by github?

/**
 * @covers \Zend\Router\RouteResult
 * @covers \Zend\Router\RouteResultTrait
 */
class RouteResultTest extends TestCase
{