phpstan-phpunit icon indicating copy to clipboard operation
phpstan-phpunit copied to clipboard

Add rule that checks for invalid and unrecognized annotations

Open PrinsFrank opened this issue 2 years ago • 3 comments

When writing annotations, missing a space between the annotation and the value will result in the annotation not being recognized. This PR adds a rule that specific annotations that have a parameter should be followed by a space before that parameter.

PrinsFrank avatar May 04 '22 19:05 PrinsFrank

@ondrejmirtes any change this can get merged? Or is there anything I can improve?

PrinsFrank avatar Jul 19 '22 22:07 PrinsFrank

Hi, I noticed your PR but I'm a bit torn on it because I've been a heavy PHPUnit user for the last 12 years at least, but I never used a single annotation like that (besides @dataProvider which I use a lot).

How severe are the bugs that are reported by this rule? Does it lead to something wrong being reported by PHPUnit, or is it that it plainly doesn't work and the user recognizes it easily?

ondrejmirtes avatar Jul 20 '22 09:07 ondrejmirtes

@ondrejmirtes for dataProviders the functionality breaks because a test with arguments doesn't work without a provider. That is not the case for all annotations. I've tried making this inspection generic, so I looked at the documentation for all annotations with arguments, but the one I actually created this rule for was the @covers annotation.

In open source packages, there is usually very tight control over the quality of code, but when you work in a bigger company on a monolith there is usually some variation of expertises between teams. To make sure unit tests are added for new features, at several of those projects we had a coverage that was not allowed to be decreased.

Sometimes though, a unit test was added that actually booted an entire framework or hit a lot of dependencies, causing the test coverage to be way higher than it should be for the unit tests because of the way coverage is measured. For this reason, we started to use @coversDefaultClass \F\Q\N above each test class and @covers ::methodName above each test method to make sure if extra code got executed it wouldn't impact the coverage. If you write it without the extra space, the annotation is not taken into consideration though: @covers::methodName, which is really hard to spot in a code review.

PrinsFrank avatar Jul 21 '22 19:07 PrinsFrank

This is really solid work, thanks! I'd like to merge it but for backward compatibility, the rule has to be only part of bleedingEdge: https://phpstan.org/blog/what-is-bleeding-edge

To do that, please follow this example: https://github.com/phpstan/phpstan-doctrine/blob/bc5036146b0badc1e07b450a6ae73086524cd953/rules.neon#L27-L29

ondrejmirtes avatar Oct 17 '22 12:10 ondrejmirtes

@ondrejmirtes Thanks for the feedback, I moved it to a bleedingEdge rule and made sure the branch was up to date again!

PrinsFrank avatar Oct 18 '22 20:10 PrinsFrank

Thank you!

ondrejmirtes avatar Oct 26 '22 17:10 ondrejmirtes