From a3a4def2466e637afdbe92a1efea1d4e1e4af712 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 13:40:20 +0200 Subject: [PATCH 1/9] :package: Move check for static above the method reflection resolving logic Avoid unnecessary resolving --- rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php index 9d29a191..99a113e5 100644 --- a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php +++ b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php @@ -41,12 +41,12 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool return false; } - $classReflection = $this->reflectionResolver->resolveClassReflection($call); - if (! $classReflection instanceof ClassReflection) { + if ($call instanceof StaticCall && ! $this->nodeNameResolver->isNames($call->class, ['static', 'self'])) { return false; } - if ($call instanceof StaticCall && ! $this->nodeNameResolver->isNames($call->class, ['static', 'self'])) { + $classReflection = $this->reflectionResolver->resolveClassReflection($call); + if (! $classReflection instanceof ClassReflection) { return false; } From 3251eb71f70f95801dd45b7e04b57425e3297a10 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 13:40:20 +0200 Subject: [PATCH 2/9] :bug: [9138] Ensure that rule `PreferPHPUnitSelfCall` refactor call only for static methods, not any https://github.com/rectorphp/rector/issues/9138 --- .../Fixture/include_exceptions.php.inc | 6 ++-- ... => NonAssertPrefixedAssertionMethods.php} | 2 +- .../NodeAnalyser/AssertMethodAnalyzer.php | 34 +++++++++++++++---- .../Class_/PreferPHPUnitSelfCallRector.php | 2 +- 4 files changed, 33 insertions(+), 11 deletions(-) rename rules/CodeQuality/Enum/{NonAssertStaticableMethods.php => NonAssertPrefixedAssertionMethods.php} (89%) diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc index eab2cb99..21d879ae 100644 --- a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc +++ b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc @@ -26,9 +26,9 @@ final class IncludeExceptions extends TestCase { public function testMe() { - self::expectException(\RuntimeException::class); - self::expectExceptionMessage('foo'); - self::expectExceptionCode(123); + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('foo'); + $this->expectExceptionCode(123); } } diff --git a/rules/CodeQuality/Enum/NonAssertStaticableMethods.php b/rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php similarity index 89% rename from rules/CodeQuality/Enum/NonAssertStaticableMethods.php rename to rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php index 608cf520..92ccdac6 100644 --- a/rules/CodeQuality/Enum/NonAssertStaticableMethods.php +++ b/rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php @@ -4,7 +4,7 @@ namespace Rector\PHPUnit\CodeQuality\Enum; -final class NonAssertStaticableMethods +final class NonAssertPrefixedAssertionMethods { /** * @var string[] diff --git a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php index 99a113e5..f2772907 100644 --- a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php +++ b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php @@ -7,10 +7,11 @@ use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\StaticCall; use PHPStan\Reflection\ClassReflection; +use PHPStan\Reflection\ExtendedMethodReflection; use PHPStan\Type\ObjectType; use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\NodeTypeResolver; -use Rector\PHPUnit\CodeQuality\Enum\NonAssertStaticableMethods; +use Rector\PHPUnit\CodeQuality\Enum\NonAssertPrefixedAssertionMethods; use Rector\PHPUnit\Enum\PHPUnitClassName; use Rector\Reflection\ReflectionResolver; @@ -36,7 +37,7 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool $methodName = $this->nodeNameResolver->getName($call->name); if (! str_starts_with((string) $methodName, 'assert') && ! in_array( $methodName, - NonAssertStaticableMethods::ALL + NonAssertPrefixedAssertionMethods::ALL )) { return false; } @@ -45,17 +46,38 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool return false; } - $classReflection = $this->reflectionResolver->resolveClassReflection($call); - if (! $classReflection instanceof ClassReflection) { + $extendedMethodReflection = $this->resolveMethodReflection($call); + if (! $extendedMethodReflection instanceof ExtendedMethodReflection) { return false; } - $extendedMethodReflection = $classReflection->getNativeMethod($methodName); - // only handle methods in TestCase or Assert class classes $declaringClassName = $extendedMethodReflection->getDeclaringClass() ->getName(); return in_array($declaringClassName, [PHPUnitClassName::TEST_CASE, PHPUnitClassName::ASSERT]); } + + public function detectTestCaseCallForStatic(MethodCall $methodCall): bool + { + if (! $this->detectTestCaseCall($methodCall)) { + return false; + } + + $extendedMethodReflection = $this->resolveMethodReflection($methodCall); + + return $extendedMethodReflection instanceof ExtendedMethodReflection && $extendedMethodReflection->isStatic(); + } + + private function resolveMethodReflection(MethodCall|StaticCall $call): ?ExtendedMethodReflection + { + $methodName = $this->nodeNameResolver->getName($call->name); + + $classReflection = $this->reflectionResolver->resolveClassReflection($call); + if (! $classReflection instanceof ClassReflection) { + return null; + } + + return $classReflection->getNativeMethod($methodName); + } } diff --git a/rules/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector.php b/rules/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector.php index fe9e38ab..2f6b1a25 100644 --- a/rules/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector.php +++ b/rules/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector.php @@ -83,7 +83,7 @@ public function refactor(Node $node): ?Node return null; } - if (! $this->assertMethodAnalyzer->detectTestCaseCall($node)) { + if (! $this->assertMethodAnalyzer->detectTestCaseCallForStatic($node)) { return null; } From 1980c36c8031fbff080ad470ea2f7193d8e5cabd Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 13:40:20 +0200 Subject: [PATCH 3/9] :book: [9138] Explain fixture behavior with comment https://github.com/rectorphp/rector/issues/9138 --- .../Fixture/include_exceptions.php.inc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc index 21d879ae..eb022e73 100644 --- a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc +++ b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc @@ -8,6 +8,7 @@ final class IncludeExceptions extends TestCase { public function testMe() { + // this calls should be preserved with $this, because methods are not static $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('foo'); $this->expectExceptionCode(123); @@ -26,6 +27,7 @@ final class IncludeExceptions extends TestCase { public function testMe() { + // this calls should be preserved with $this, because methods are not static $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('foo'); $this->expectExceptionCode(123); From 9dcb0e3b7bb7d13c59c0130b2c4bbc43d0de47cf Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 15:36:51 +0200 Subject: [PATCH 4/9] :package: Rename fixture to be clear about semantics --- .../Fixture/include_exceptions.php.inc | 37 ------------------- .../Fixture/skip_include_exceptions.php.inc | 18 +++++++++ 2 files changed, 18 insertions(+), 37 deletions(-) delete mode 100644 rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc create mode 100644 rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc deleted file mode 100644 index eb022e73..00000000 --- a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/include_exceptions.php.inc +++ /dev/null @@ -1,37 +0,0 @@ -expectException(\RuntimeException::class); - $this->expectExceptionMessage('foo'); - $this->expectExceptionCode(123); - } -} - -?> ------ -expectException(\RuntimeException::class); - $this->expectExceptionMessage('foo'); - $this->expectExceptionCode(123); - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc new file mode 100644 index 00000000..302a5016 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc @@ -0,0 +1,18 @@ +expectException(\RuntimeException::class); + $this->expectExceptionMessage('foo'); + $this->expectExceptionCode(123); + } +} + +?> From 65908dd0dae864e74815b5820cf8201471dbcb71 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 15:41:11 +0200 Subject: [PATCH 5/9] :package: Rename class in fixture to be consistent with filename --- ...nclude_exceptions.php.inc => skip_skip_exceptions.php.inc} | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) rename rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/{skip_include_exceptions.php.inc => skip_skip_exceptions.php.inc} (88%) diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_skip_exceptions.php.inc similarity index 88% rename from rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc rename to rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_skip_exceptions.php.inc index 302a5016..f400697e 100644 --- a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_include_exceptions.php.inc +++ b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_skip_exceptions.php.inc @@ -4,7 +4,7 @@ namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\PreferPHPUnitSelfCallRe use PHPUnit\Framework\TestCase; -final class IncludeExceptions extends TestCase +final class SkipExceptions extends TestCase { public function testMe() { @@ -14,5 +14,3 @@ final class IncludeExceptions extends TestCase $this->expectExceptionCode(123); } } - -?> From 35f0032afcb8cd5258ece2195db54d7e8db0a729 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 15:41:47 +0200 Subject: [PATCH 6/9] :package: Rename fixture file --- .../{skip_skip_exceptions.php.inc => skip_exceptions.php.inc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/{skip_skip_exceptions.php.inc => skip_exceptions.php.inc} (100%) diff --git a/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_skip_exceptions.php.inc b/rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_exceptions.php.inc similarity index 100% rename from rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_skip_exceptions.php.inc rename to rules-tests/CodeQuality/Rector/Class_/PreferPHPUnitSelfCallRector/Fixture/skip_exceptions.php.inc From 0498984eda2f234391b353f2e7f64bf4a25c0c21 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 15:54:06 +0200 Subject: [PATCH 7/9] :package: Rename enum to ensure that there is no static methods inside, check for static call before checking with enum As suggested in review comment https://github.com/rectorphp/rector-phpunit/pull/482#discussion_r2071508378 --- ...hp => NonStaticNonAssertPrefixedAssertionMethods.php} | 2 +- rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) rename rules/CodeQuality/Enum/{NonAssertPrefixedAssertionMethods.php => NonStaticNonAssertPrefixedAssertionMethods.php} (87%) diff --git a/rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php b/rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php similarity index 87% rename from rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php rename to rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php index 92ccdac6..71f6e408 100644 --- a/rules/CodeQuality/Enum/NonAssertPrefixedAssertionMethods.php +++ b/rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php @@ -4,7 +4,7 @@ namespace Rector\PHPUnit\CodeQuality\Enum; -final class NonAssertPrefixedAssertionMethods +final class NonStaticNonAssertPrefixedAssertionMethods { /** * @var string[] diff --git a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php index f2772907..d450fbb2 100644 --- a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php +++ b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php @@ -11,7 +11,7 @@ use PHPStan\Type\ObjectType; use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\NodeTypeResolver; -use Rector\PHPUnit\CodeQuality\Enum\NonAssertPrefixedAssertionMethods; +use Rector\PHPUnit\CodeQuality\Enum\NonStaticNonAssertPrefixedAssertionMethods; use Rector\PHPUnit\Enum\PHPUnitClassName; use Rector\Reflection\ReflectionResolver; @@ -35,10 +35,11 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool } $methodName = $this->nodeNameResolver->getName($call->name); - if (! str_starts_with((string) $methodName, 'assert') && ! in_array( + if (! str_starts_with((string) $methodName, 'assert') && ! ($call instanceof StaticCall && in_array( $methodName, - NonAssertPrefixedAssertionMethods::ALL - )) { + NonStaticNonAssertPrefixedAssertionMethods::ALL, + true + ))) { return false; } From ce1d4d6cb11e89afe0da7cb73e5dae45da8ca341 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 15:57:11 +0200 Subject: [PATCH 8/9] :package: Rename enum to be clear about semantics and short As suggested in the review comment https://github.com/rectorphp/rector-phpunit/pull/482#discussion_r2071633901 --- ...ixedAssertionMethods.php => NonAssertNonStaticMethods.php} | 2 +- rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename rules/CodeQuality/Enum/{NonStaticNonAssertPrefixedAssertionMethods.php => NonAssertNonStaticMethods.php} (87%) diff --git a/rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php b/rules/CodeQuality/Enum/NonAssertNonStaticMethods.php similarity index 87% rename from rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php rename to rules/CodeQuality/Enum/NonAssertNonStaticMethods.php index 71f6e408..a8813de5 100644 --- a/rules/CodeQuality/Enum/NonStaticNonAssertPrefixedAssertionMethods.php +++ b/rules/CodeQuality/Enum/NonAssertNonStaticMethods.php @@ -4,7 +4,7 @@ namespace Rector\PHPUnit\CodeQuality\Enum; -final class NonStaticNonAssertPrefixedAssertionMethods +final class NonAssertNonStaticMethods { /** * @var string[] diff --git a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php index d450fbb2..281ad005 100644 --- a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php +++ b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php @@ -11,7 +11,7 @@ use PHPStan\Type\ObjectType; use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\NodeTypeResolver; -use Rector\PHPUnit\CodeQuality\Enum\NonStaticNonAssertPrefixedAssertionMethods; +use Rector\PHPUnit\CodeQuality\Enum\NonAssertNonStaticMethods; use Rector\PHPUnit\Enum\PHPUnitClassName; use Rector\Reflection\ReflectionResolver; @@ -37,7 +37,7 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool $methodName = $this->nodeNameResolver->getName($call->name); if (! str_starts_with((string) $methodName, 'assert') && ! ($call instanceof StaticCall && in_array( $methodName, - NonStaticNonAssertPrefixedAssertionMethods::ALL, + NonAssertNonStaticMethods::ALL, true ))) { return false; From 1b83816ab09572913e28b6b16330b85c7bf6d3ab Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 2 May 2025 16:07:01 +0200 Subject: [PATCH 9/9] :package: Revert check for static call as unneeded here As of review comment https://github.com/rectorphp/rector-phpunit/pull/482#discussion_r2071660206 --- rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php index 281ad005..7c0ef823 100644 --- a/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php +++ b/rules/CodeQuality/NodeAnalyser/AssertMethodAnalyzer.php @@ -35,11 +35,11 @@ public function detectTestCaseCall(MethodCall|StaticCall $call): bool } $methodName = $this->nodeNameResolver->getName($call->name); - if (! str_starts_with((string) $methodName, 'assert') && ! ($call instanceof StaticCall && in_array( + if (! str_starts_with((string) $methodName, 'assert') && ! in_array( $methodName, NonAssertNonStaticMethods::ALL, true - ))) { + )) { return false; }