Add rector rule to automatically add `#[CoversClass]` attribute to test files

[maks-rafalko]

Implements https://github.com/rectorphp/rector-phpunit/issues/318

Rationale

On PHPUnit side, we can configure our tests in the way that every Test File has to have #[CoversClass(...)] attributes, otherwise tests will be marked as Risky and PHPUnit will exit with non-zero error code.

This can be done using

• requireCoverageMetadata configuration attribute (previously forceCoversAnnotation="true")

Why is it needed?

• because it's a good practice (e.g. read https://localheinz.com/articles/2023/02/22/documenting-the-system-under-test-in-phpunit/)
• and for us, Infection maintainers, it helps to easier introduce new --map-source-class-to-test option to dramatically speed up Mutation Testing (MT) for projects

Real example with PHP-CS-Fixer: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7874#issuecomment-2010894591

We managed to decrease the MT execution from 2m 27s to 8s (!!!). And this was possible because in PHP-CS-Fixer their tests has 1-to-1 relation between test file and source file, thanks to #[CoversClass(...)] annotation.

But, I wasn't able to find an automatic way of how we can:

• update existing project and introduce this attribute to all the tests files (at Infection, without this feature I would need to manually update ~400 files)
• force this attribute on each PR when new test file is created.

So, I ended up creating this PR that will help other developers to achieve the same goals.

[maks-rafalko]

Classes with a parent/interface - possibly used by type
=======================================================

 * Rector\PHPUnit\CodeQuality\Rector\Class_\AddCoversClassAttributeRector
rules/CodeQuality/Rector/Class_/AddCoversClassAttributeRector.php

should I add it to config/sets/phpunit-code-quality.php to fix this issue?

By the way, composer.json has different scripts to check code quality (phpstan, ecs, etc.). But class-leak is not present there, that's why I even didn't know about it as a first-time contributor.

Should it be added too (in separate PR)? UPD: already opened other PRs

https://github.com/rectorphp/rector-phpunit/blob/6845db43ccd69ef990d399ff845a53ad66fa8085/composer.json#L36-L42

[VincentLanglet]

If I understand correctly the rule, it only works if the test and the tested class has the same namespace. Am I wrong ?

In Symfony doc (https://symfony.com/doc/current/create_framework/unit_testing.html and https://symfony.com/doc/current/testing.html), the namespace used are more often

• App\Foo, for the class
• App\Tests\Foo, for the test

1) Would it work ? 2) Would it be easy to add an option for the "namespace mapping"

(In my case, I use App\Foo and App\Tests\Unit\Foo or App\Tests\Functional\Foo ; so an option would be great)

[maks-rafalko]

If I understand correctly the rule, it only works if the test and the tested class has the same namespace.

No. The current implementation adds 2 possible variations to find a source class based on the name of the test class (logic is here)

If you have App\Tests\SubNamespace\FooTest (note *Test postfix, it's absent for some reason in your example), the followig candidates will be generated:

• App\Tests\SubNamespace\Foo - same namespace, but without *Test postfix. This will be useful for those projects who store test classes near source classes. But in real project case FQCNs would be App\SubNamespace\Foo for source class and App\SubNamespace\FooTest for test class
• App\SubNamespace\Foo - Tests subnamespace is removed

If you have namespaces for tests containing Unit / Functional subnamespaces, which don't present in source class FQCN, then it wouldn't find a source file with the current implementation.

Of course, we can make the logic of resolving source class name more complex, but I would like to first understand if it's really needed and will be accepted, just to not waste time.

---

App\Foo, for the class App\Tests\Foo, for the test Would it work ?

yes, this is exactly point 2 above and our case in Infection.

Would it be easy to add an option for the "namespace mapping"

could you please describe how it would work?

[VincentLanglet]

If you have App\Tests\SubNamespace\FooTest (note *Test postfix, it's absent for some reason in your example)

i meant App\Tests\Foo\ServiceTest, it just wrote the namespace, but yes App\Tests\SubNamespace\FooTest is definitly what I had in mind.

Would it be easy to add an option for the "namespace mapping"

could you please describe how it would work?

Not sure what was the best solution, I thought we could have a $testMapping option, by default it's for instance in my personal use case I would have pass something like

[
    'App\Test\Unit' => 'App',
    'App\Test\Functional' => 'App',
]

But I definetly understand it's a special use case and might not be worth it in this generic rule.

[maks-rafalko]

other solution would be to try generatig more combinations, removing commonly used subnamespaces, e.g. for App\Tests\Unit\Foo generate:

• App\Unit\Foo - remove Tests
• App\Foo - remove Tests and Unit (where both are reserved words)

But yeah, let's first listen to maintainers before doing any changes. Probably makes sense to merge the common case and then upgrade resolving logic as a separate PR (if needed).

[staabm]

other solution would be to try generatig more combinations, removing commonly used subnamespaces, e.g. for App\Tests\Unit\Foo generate:

another solution could be to make this rule "configurable", see other rules implementing the configure method. but I agree that we should do this in a separate iteration. lets first get the rule landed and see whether it works as is

[maks-rafalko]

https://github.com/rectorphp/rector-phpunit/actions/runs/8475251813/job/23383638862?pr=319#step:5:16 - this is unrelated to the changes in this PR

Comments addressed, please re-review

[samsonasik]

Rebase is needed to make CI green.

[maks-rafalko]

Rebase is needed to make CI green.

Could you please run actions? should be green now

[maks-rafalko]

Green :+1:

If you guys decide to merge it, can I ask you to make a release so we can use this new feature at Infection?

Thanks!

[staabm]

@maks-rafalko maybe you could give a few notes, why this annotation is useful for projects and when one should use it?

this could give rectors devs an idea why it is important

[samsonasik]

I prefer it to be configurable, and exclude from code quality set tbh, as it addition is by guessing, and may probably cause unwanted changes, as I personally not want to use it.

I will let @TomasVotruba decide

[maks-rafalko]

@maks-rafalko maybe you could give a few notes, why this annotation is useful for projects and when one should use it?

added to PR description

and exclude from code quality set tbh, as it addition is by guessing, and may probably cause unwanted changes, as I personally not want to use it.

can we have a rule that is not part of any set at Rector? Cause if I remember correctly, class-leak was failing because initially I didn't add it anywhere.

[samsonasik]

--skip-type should can be used, ref https://github.com/rectorphp/rector-src/blob/a43f37190db97fb8807cb08f4e7171a338625387/.github/workflows/code_analysis.yaml#L44

[maks-rafalko]

Ok, then let me clarify before I do any changes:

• should I remove this new rector rule from existing set and add --skip-type to the place you mentioned above?
• if yes, what do you think if we add --skip-type on the new composer script that I added here https://github.com/rectorphp/rector-phpunit/pull/320 and instead of duplicating command line on GH Action, we will just use composer class-leak there?

---

Actually, the right place is here:

https://github.com/rectorphp/rector-phpunit/blob/a56b646588d42fce925cb270753980441bdd2e17/.github/workflows/code_analysis.yaml#L21

so I propose ^ this to replace with run: composer class-leak

[samsonasik]

double -- should add it as argument:

composer class-leak -- --skipe-type="..."

[maks-rafalko]

what about the first point?

should I remove this new rector rule from existing set and add --skip-type to the place you mentioned above?

[samsonasik]

Yes, imo, remove from set and skip from class-leak, it also need to be registered in rector-src MissingInSetCommand as well if it merged

[maks-rafalko]

ready for review again

[maks-rafalko]

Is there anything I need to do to move it forward? @TomasVotruba @samsonasik

[samsonasik]

Let's give it a try, thank you @maks-rafalko

[staabm]

awesome, thank you!

[staabm]

@maks-rafalko running the new rector on phpunit itself yields a few false positives, e.g.

55) tests/unit/Framework/MockObject/TestDoubleTestCase.php:21

    ---------- begin diff ----------
@@ @@
 use PHPUnit\TestFixture\MockObject\InterfaceWithReturnTypeDeclaration;
 use stdClass;

+#[\PHPUnit\Framework\Attributes\CoversClass(\PHPUnit\Framework\MockObject\TestDoubleTestCase::class)]
 abstract class TestDoubleTestCase extends TestCase
 {
     final public function testMethodReturnsNullWhenReturnValueIsNullableAndNoReturnValueIsConfigured(): void
    ----------- end diff -----------

Applied rules:
 * AddCoversClassAttributeRector

56) tests/unit/Metadata/Parser/AnnotationParserTestCase.php:41

    ---------- begin diff ----------
@@ @@
 use PHPUnit\TestFixture\Metadata\Annotation\TestDoxTest;
 use PHPUnit\TestFixture\Metadata\Annotation\UsesTest;

+#[\PHPUnit\Framework\Attributes\CoversClass(\PHPUnit\Metadata\Parser\AnnotationParserTestCase::class)]
 abstract class AnnotationParserTestCase extends TestCase
 {
     public static function provideRequiresPhpTestMethods(): array
    ----------- end diff -----------

Applied rules:
 * AddCoversClassAttributeRector

57) tests/unit/Metadata/Parser/AttributeParserTestCase.php:52

    ---------- begin diff ----------
@@ @@
 use PHPUnit\TestFixture\Metadata\Attribute\UsesTest;
 use PHPUnit\TestFixture\Metadata\Attribute\WithoutErrorHandlerTest;

+#[\PHPUnit\Framework\Attributes\CoversClass(\PHPUnit\Metadata\Parser\AttributeParserTestCase::class)]
 abstract class AttributeParserTestCase extends TestCase
 {
     #[TestDox('Parses #[BackupGlobals] attribute on class')]
    ----------- end diff -----------

Applied rules:
 * AddCoversClassAttributeRector

I think we should make sure that the class beeing added in the CoversClass annotation is not a test-class itself.

wdyt?

[samsonasik]

I am on mobile, it seems also needs check that the target class should also need to be in test class; eg:

$isBeingTested = (bool) $this->betterNodeFinder->findFirst($node->getMethods(), fn(Node $subNode): bool => $subNode instanceof FullyQualified && $subNode->toString() === $targetClassName);

[staabm]

my comment is addressed in https://github.com/rectorphp/rector-phpunit/pull/324

[maks-rafalko]

Could you please guys give an idea when this can be released? As I understand, this package is part of rector/rector and we need to wait untill next rector/rector release?

[samsonasik]

@maks-rafalko rector 1.0.5 is tagged https://github.com/rectorphp/rector/releases/tag/1.0.5