Bring back more fine-grained code coverage targetting

[jnoordsij]

In PHPUnit 10.0, attributes were added to (optionally) replace code comment annotations (great feature btw!). However, these are currently limited only to apply to classes, where previously covers attributes were allowed to be on methods as well.

It would be fruitful for some usecases (as mentioned in various comments below) to allow for more fine-grained coverage measurement; in particular to allow targetting (class) methods by name using attributes.

~~Note: if this is an intentional change, and there is no plan to actively support this anymore, maybe this should be turned into two other issues:~~

• ~~add a deprecation message for covers attributes on methods~~
• ~~an issue on the Rector Rules repository to not automatically convert method-based covers annotations into attributes, as it will result into a broken tests suite (Attribute "PHPUnit\Framework\Attributes\CoversClass" cannot target method (allowed targets: class))~~

[sebastianbergmann]

This is intentional and explained in https://github.com/sebastianbergmann/phpunit/issues/4502.

[jnoordsij]

Thanks for pointing me there! I've raised https://github.com/rectorphp/rector-phpunit/issues/149 to highlight this issue there.

I think a separate deprecation for method-based annotations is probably not be needed, as this will be covered by deprecating all annotations in #4505. However it might be worth adding a somewhat more extended migration guide at that point, with some pointers on how to replace method-based annotations with class-based attributes.

[kylekatarnls]

This is intentional and explained in #4502.

I see it's intentional and noted "not recommended because too fine-grained"

But I'm searching with no luck where this design choice is explained. I've cases similar to https://github.com/sebastianbergmann/phpunit/issues/5558#issue-1976667171 where it's relevant to avoid a method to be unintentionally covered.

There I can't find any good reason why it would be bad to be too fine-grained in cover targeting.

Any reading you can recommend @sebastianbergmann to learn why we should not to that and then how we prevent accidental coverage of a method?

[mattvb91]

This is a crazy change to remove being able to cover methods directly... Defeats the whole purpose of having @covers in the first place...

Any recommendations on how to get targeted coverage results back?

[jrfnl]

I'm struggling with the same thing as everyone else in this thread.

Yes, of course, everyone should be using SOLID OO and classes should only have one responsibility and in an ideal world, using CoversClass would work brilliantly.

Except I do not live in such an ideal world. I live in a world of legacy code, where there are plenty of classes which do too much and where I want to know exactly what code (i.e. what method(s)) is covered by dedicated tests (vs incidentally covered) to know whether it is safe to refactor that code (preferably to more SOLID code).

Unfortunately, that will no longer be possible, which makes adding Covers* attributes at all, basically useless, as it no longer prevents unintentional code coverage from being recorded.

@sebastianbergmann Please, please, please reconsider this design choice, as CoversClass is next to useless for real world situations.

[sebastianbergmann]

Do you only want targetting methods back or do you also want to be able to use coverage target attributes on test methods in addition to test classes?

[jrfnl]

Do you only want targetting methods back or do you also want to be able to use coverage target attributes on test methods in addition to test classes?

@sebastianbergmann For me, the most important part is being able to target methods.

I don't have a problem with splitting up test classes which were targeting multiple different units of code to multiple test classes, each targeting just one unit of code.

Splitting test classes is not a breaking change, just busy-work while I have better things to do, but I can deal with that. Having to refactor large parts of the code base under test just to be able to see what code is covered by tests, that's a whole other matter due to the breaking changes in the public API that would involve.

[jrfnl]

@sebastianbergmann Looking at the tags and milestoning you've just done, I take it you are considering this. Should the ticket be re-opened ?

[jrfnl]

@sebastianbergmann As a side-note: I was wondering how to deal with Covers attributes vs namespaced functions.

The CoversFunction attribute is documented to only support global functions. I haven't tested it, but hope it supports namespaced functions too ?

[sebastianbergmann]

I haven't tested it, but hope it supports namespaced functions too?

It should. :) And according to this test it does.

[mattvb91]

Just a quick thank you to @sebastianbergmann for putting this change back in. Im in the exact same boat as @jrfnl where im working on a legacy codebase and the issue is exactly that its super chaotic in that there is a ton of global superclasses that hold a ton of functionality that I am in the process of splitting up and refactoring to individual classes so the @covers is essential in helping out what has already been tested and can be moved.

[mfn]

legacy codebase

I hate to "out myself", but I work on what I consider a more modern codebase and I find it still very useful to have.

[mfn]

HOLY 🐮

I gave up on this… this makes me so happy to see this back.

Thank you @sebastianbergmann , this is really really appreciated (also 🙇🏼 to those who argued in favor, thank you too 🙏🏼 )

[kylekatarnls]

A big thank you @sebastianbergmann for this attribute port of the method annotations :) we're lucky to have you in the PHP community 🥇

[jrfnl]

@sebastianbergmann Joining the others in saying: Thank you! I'm very happy to see that this will be brought back.

On a practical note: I presume this will only be introduced in PHPUnit 11.1 ?

For those of us who need to use a range of PHPUnit versions to runs the tests, am I correct in saying those projects should then require "phpunit/phpunit": "^8.0 || ^9.0 || ^11.1", effectively skipping PHPUnit 10 ? As 10.x will not support this attribute - and I presume will not read the @covers annotation if it finds other attributes on the class ?

[sebastianbergmann]

I presume this will only be introduced in PHPUnit 11.1?

This will not be backported to PHPUnit 10, correct.

I presume will not read the @covers annotation if it finds other attributes on the class?

PHPUnit 10 and PHPUnit 11 support metadata in annotations and attributes. If an attribute is found on a unit of code, annotations on that unit of code are ignored.

am I correct in saying those project should then require "phpunit/phpunit": "^8.0 || ^9.0 || ^11.1", effectively skipping PHPUnit 10?

Yes.

[jrfnl]

@sebastianbergmann Thank you for confirming.

[miken32]

@sebastianbergmann you asked:

Do you only want targeting methods back or do you also want to be able to use coverage target attributes on test methods in addition to test classes?

Was there an intentional choice to only allow the CoversMethod attribute to be applied to classes? I have always used the old @covers annotation on each test method. Is there a compelling reason not to have the attribute definition contain this?

#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]

The workaround is fairly simple: having multiple CoversMethod attributes on the class and indicating in comments or elsewhere which method the test method is covering. I'm just wondering why a workaround is needed at all, or if this was an oversight in the attribute definition.