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

Remove support for "ClassName<*>" as values for @covers and @uses annotations #3631

Closed
sebastianbergmann opened this issue Apr 26, 2019 · 5 comments
Assignees
Labels
feature/code-coverage Issues related to code coverage (but not php-code-coverage) type/backward-compatibility Something will be/is intentionally broken
Milestone

Comments

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Apr 26, 2019

PHPUnit 10 should not support the following annotations anymore:

  • @covers ClassName::<public>
  • @covers ClassName::<protected>
  • @covers ClassName::<private>
  • @covers ClassName::<!public>
  • @covers ClassName::<!protected>
  • @covers ClassName::<!private>
  • @covers ClassName<extended>
  • @uses ClassName::<public>
  • @uses ClassName::<protected>
  • @uses ClassName::<private>
  • @uses ClassName::<!public>
  • @uses ClassName::<!protected>
  • @uses ClassName::<!private>
  • @uses ClassName<extended>
@sebastianbergmann sebastianbergmann added feature-removal type/backward-compatibility Something will be/is intentionally broken labels Apr 26, 2019
@sebastianbergmann sebastianbergmann self-assigned this Apr 26, 2019
@sebastianbergmann sebastianbergmann added this to the PHPUnit 10.0 milestone Apr 26, 2019
@andrewnicols
Copy link
Contributor

I'm just wondering whether there will be any replacement for these and what the rationale is.

Generally I/we prefer to only @cover the public part of an API that we are actively testing, and to use @uses ClassName::<!public> to cover the private/protected methods. The rationale for this being that we are testing the public interface, but how the class actually achieves the result is its business. Technically I guess these could be considered to be functional tests, but the alternative is to add copious @covers tags for things that I'm not really testing but are covered, or to use Reflection to actively test business logic.

@Ocramius
Copy link
Contributor

Ocramius commented Sep 6, 2019

The rationale for this being that we are testing the public interface, but how the class actually achieves the result is its business.

The test is still supposed to figure out whether the internals of your system are sound and well covered or not. Having @covers Foo::<public> (for example) could lead to developers only writing public methods, or not having any coverage report at all on private API.

@uses can be limited to just the target class names: it's an external collaborator anyway, and public/private does not matter there either.

@sebastianbergmann sebastianbergmann added feature/code-coverage Issues related to code coverage (but not php-code-coverage) and removed feature-removal labels Feb 11, 2020
sebastianbergmann added a commit to sebastianbergmann/code-unit that referenced this issue Mar 1, 2020
…stead of a CodeUnit object (even if only a single CodeUnit object is to be returned)

The purpose of this component is to help with refactoring how information from @Covers and @uses annotations is passed from phpunit/phpunit to phpunit/php-code-coverage, see sebastianbergmann/php-code-coverage#519 for details.

I want this to happen sooner rather than later and do not want to wait for PHPUnit 10 and sebastianbergmann/phpunit#3631.

This means that, at least until PHPUnit 10, this component needs to support these strings for CodeUnit::fromString():

* ClassName::<public>
* ClassName::<protected>
* ClassName::<private>
* ClassName::<!public>
* ClassName::<!protected>
* ClassName::<!private>
@Firehed
Copy link
Contributor

Firehed commented Aug 21, 2020

Hi, somewhat late to the game here, but I agree with @andrewnicols's philosophy on coverage, and this is already blocking me from upgrading to 9.x due to (presumably) some change in the docblock parser that was probably related to plans for this. I'll assume for discussion purposes that breakage is a bug, but it would be just as relevant in the future if this happens for PHPUnit 10.

My tests all start off with the following:

/**
 * @coversDefaultClass Company\Project\My
 * @covers ::<protected>
 * @covers ::<private>
 */
class MyTest extends \PHPUnit\Framework\TestCase
{

Tests can and do figure out whether the internals of my code are sound, and the coverage reports confirm as much. Test cases interact with the public APIs, and exercise logic that's contained in the public and private APIs. The coverage reports for public APIs must be explicit, and anything internal is an implementation detail.

In my opinion, it's actively harmful for tests to be forced to know which private/protected methods any public API uses for the purpose of generating coverage reports, but as of the docs for 9.3, the only recommended way to annotate coverage is class-wide; even covering specific class methods is explicitly "not recommended".

So while my gut reaction is that this isn't entirely a good direction to go, I'm open to alternatives. Maybe the best practices in the documentation could be made clearer? My current approach is based on a test generator skeleton that probably dates back to PHPUnit 4 or so. It would be really nice to not introduce code churn on thousands of test files in an upgrade, but if there's a better way I'll deal with it.

To be clear: I have no issue with dropping <public> support, or equivalents. There's a lot of value in being explicit about which part(s) of the public API a unit test is covering. But being able to either implicitly or explicitly cover any part of the private API (whether via <!public> or <private> or something else) seems like a fundamental goal of proper encapsulation.

@andrewnicols
Copy link
Contributor

@sebastianbergmann it would be good to have some discussion on what the expected alternative is here.

sebastianbergmann added a commit that referenced this issue Dec 14, 2020
@markwatney2016
Copy link

markwatney2016 commented May 7, 2021

What is the alternative for the following now?

<?php
declare(strict_types=1);
namespace Tests;

use PHPUnit\Framework\TestCase;

/**
 * @coversDefaultClass Foo
 * @covers ::<!public>
 */
class FooTest extends TestCase
{
    /**
     * @test
     * @covers ::bar
     */
    public function loremIpsum(): void
    {
        ...
    }
}

Class Foo has a public bar() method and multiple private methods that are called by bar().

All private methods that are called by bar() should also become green in the code coverage report when the test for bar() passes. How can this be achived now that @covers ::<!public> is not supported any longer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/code-coverage Issues related to code coverage (but not php-code-coverage) type/backward-compatibility Something will be/is intentionally broken
Projects
None yet
Development

No branches or pull requests

5 participants