-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Bring back more fine-grained code coverage targeting #5175
Comments
This is intentional and explained in #4502. |
Thanks for pointing me there! I've raised rectorphp/rector-phpunit#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. |
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 #5558 (comment) 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? |
This is a crazy change to remove being able to cover methods directly... Defeats the whole purpose of having Any recommendations on how to get targeted coverage results back? |
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 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 @sebastianbergmann Please, please, please reconsider this design choice, as |
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. |
@sebastianbergmann Looking at the tags and milestoning you've just done, I take it you are considering this. Should the ticket be re-opened ? |
@sebastianbergmann As a side-note: I was wondering how to deal with Covers attributes vs namespaced functions. The |
It should. :) And according to this test it does. |
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 |
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. |
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 🙏🏼 ) |
A big thank you @sebastianbergmann for this attribute port of the method annotations :) we're lucky to have you in the PHP community 🥇 |
@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 |
This will not be backported to PHPUnit 10, correct.
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.
Yes. |
@sebastianbergmann Thank you for confirming. |
@sebastianbergmann you asked:
Was there an intentional choice to only allow the #[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)] The workaround is fairly simple: having multiple |
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 methodsan 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)
)The text was updated successfully, but these errors were encountered: