[CodeQuality] Skip call parent::__construct() on ConstructClassMethod…

…ToSetUpTestCaseRector (#251) * [CodeQuality] Skip call parent::__construct() on ConstructClassMethodToSetUpTestCaseRector * init early * register rules-tests * rename fixture * use StringUtils::isMatch * patch RemoveDataProviderTestPrefixRector * patch ReplaceTestAnnotationWithPrefixedFunctionRector * patch RemoveEmptyTestMethodRector * use fnmatch * remove unused imports * skip fixture * fix empty is not FuncCall * Fix AssertCallAnalyzer * skip indirect extends * cs fix * cs fix * current parent * Fixed 🎉 * fix exclude source * fix
rectorphp · Sep 10, 2023 · fde1cef · fde1cef
1 parent 2800f5c
commit fde1cef
Show file tree

Hide file tree

Showing 20 changed files with 140 additions and 44 deletions.
diff --git a/phpunit.xml b/phpunit.xml
@@ -8,8 +8,9 @@
     <testsuites>
         <testsuite name="main">
             <directory>tests</directory>
-            <exclude>tests/Rector/Class_/AddSeeTestAnnotationRector/Source</exclude>
-            <exclude>tests/Rector/ClassMethod/DependsAnnotationWithValueToAttributeRector/Source</exclude>
+            <directory>rules-tests</directory>
+            <exclude>rules-tests/CodeQuality/Rector/Class_/AddSeeTestAnnotationRector/Source</exclude>
+            <exclude>rules-tests/AnnotationsToAttributes/Rector/ClassMethod/DependsAnnotationWithValueToAttributeRector/Source</exclude>
         </testsuite>
     </testsuites>
 </phpunit>
diff --git a/rector.php b/rector.php
@@ -10,6 +10,7 @@
 
 return static function (RectorConfig $rectorConfig): void {
     $rectorConfig->importNames();
+    $rectorConfig->removeUnusedImports();
 
     $rectorConfig->paths([__DIR__ . '/src', __DIR__ . '/tests', __DIR__ . '/rules', __DIR__ . '/rules-tests']);
 

diff --git a/...nsToAttributes/Rector/Class_/TicketAnnotationToAttributeRector/config/configured_rule.php b/...nsToAttributes/Rector/Class_/TicketAnnotationToAttributeRector/config/configured_rule.php
@@ -3,7 +3,6 @@
 declare(strict_types=1);
 
 use Rector\PHPUnit\AnnotationsToAttributes\Rector\Class_\TicketAnnotationToAttributeRector;
-use Rector\PHPUnit\Tests\ConfigList;
 use Rector\Config\RectorConfig;
 
 return static function (RectorConfig $rectorConfig): void {

diff --git a/...nstructClassMethodToSetUpTestCaseRector/Fixture/skip_call_parent_construct_exists.php.inc b/...nstructClassMethodToSetUpTestCaseRector/Fixture/skip_call_parent_construct_exists.php.inc
@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\Fixture;
+
+use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\Source\TestResponse;
+
+final class SkipCallParentConstructExists extends TestResponse
+{
+    private $someValue;
+
+    public function __construct()
+    {
+        $this->someValue = 1000;
+
+        parent::__construct();
+    }
+}
diff --git a/...ctor/Class_/ConstructClassMethodToSetUpTestCaseRector/Fixture/skip_parameter_used.php.inc b/...ctor/Class_/ConstructClassMethodToSetUpTestCaseRector/Fixture/skip_parameter_used.php.inc
@@ -0,0 +1,18 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\Fixture;
+
+use PHPUnit\Framework\TestCase;
+
+final class SkipParameterUsed extends TestCase
+{
+    public function __construct($param)
+    {
+        $this->initEarly($param);
+    }
+
+    private function initEarly($param)
+    {
+        echo 'init';
+    }
+}
diff --git a/...deQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector/Source/TestResponse.php b/...deQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector/Source/TestResponse.php
@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\ConstructClassMethodToSetUpTestCaseRector\Source;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class TestResponse extends TestCase
+{
+    public function __construct()
+    {
+        echo "init";
+    }
+}
diff --git a/...rtIssetToSpecificMethodRector/Fixture/skip_if_magic_method_isset_exists_in_parent.php.inc b/...rtIssetToSpecificMethodRector/Fixture/skip_if_magic_method_isset_exists_in_parent.php.inc
@@ -14,24 +14,3 @@ final class SkipCustomIsset2 extends \PHPUnit\Framework\TestCase
         $this->assertTrue(isset($foo->bar));
     }
 }
-
-?>
------
-<?php
-
-namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertIssetToSpecificMethodRector\Fixture;
-
-// parent class not loaded
-final class CustomIsset2 extends SomeAbstractClass {
-}
-
-final class SkipCustomIsset2 extends \PHPUnit\Framework\TestCase
-{
-    public function test()
-    {
-        $foo = new \Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertIssetToSpecificMethodRector\Fixture\CustomIsset2();
-        $this->assertObjectHasAttribute('bar', $foo);
-    }
-}
-
-?>
diff --git a/...s/PHPUnit100/Rector/Class_/StaticDataProviderClassMethodRector/config/configured_rule.php b/...s/PHPUnit100/Rector/Class_/StaticDataProviderClassMethodRector/config/configured_rule.php
@@ -1,7 +1,6 @@
 <?php
 
 declare(strict_types=1);
-use Rector\PHPUnit\Tests\ConfigList;
 
 use Rector\Config\RectorConfig;
 

diff --git a/rules/AnnotationsToAttributes/Rector/ClassMethod/TestWithAnnotationToAttributeRector.php b/rules/AnnotationsToAttributes/Rector/ClassMethod/TestWithAnnotationToAttributeRector.php
@@ -6,9 +6,6 @@
 
 use Nette\Utils\Json;
 use PhpParser\Node;
-use PhpParser\Node\Attribute;
-use PhpParser\Node\AttributeGroup;
-use PhpParser\Node\Name\FullyQualified;
 use PhpParser\Node\Stmt\ClassMethod;
 use PHPStan\PhpDocParser\Ast\PhpDoc\GenericTagValueNode;
 use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagNode;

diff --git a/rules/AnnotationsToAttributes/Rector/Class_/TicketAnnotationToAttributeRector.php b/rules/AnnotationsToAttributes/Rector/Class_/TicketAnnotationToAttributeRector.php
@@ -17,10 +17,8 @@
 use PHPUnit\Framework\Attributes\Ticket;
 use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo;
 use Rector\BetterPhpDocParser\PhpDocManipulator\PhpDocTagRemover;
-use Rector\Core\Contract\Rector\ConfigurableRectorInterface;
 use Rector\Core\Rector\AbstractRector;
 use Rector\Core\ValueObject\PhpVersionFeature;
-use Rector\PhpAttribute\NodeFactory\PhpAttributeGroupFactory;
 use Rector\PHPUnit\ValueObject\AnnotationWithValueToAttribute;
 use Rector\VersionBonding\Contract\MinPhpVersionInterface;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;

diff --git a/rules/CodeQuality/Rector/ClassMethod/RemoveEmptyTestMethodRector.php b/rules/CodeQuality/Rector/ClassMethod/RemoveEmptyTestMethodRector.php
@@ -66,7 +66,7 @@ public function refactor(Node $node): ?int
             return null;
         }
 
-        if (! $this->isName($node->name, 'test*')) {
+        if (! fnmatch('test*', $node->name->toString(), FNM_NOESCAPE)) {
             return null;
         }
 

diff --git a/rules/CodeQuality/Rector/ClassMethod/ReplaceTestAnnotationWithPrefixedFunctionRector.php b/rules/CodeQuality/Rector/ClassMethod/ReplaceTestAnnotationWithPrefixedFunctionRector.php
@@ -71,7 +71,7 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        if ($this->isName($node->name, 'test*')) {
+        if (fnmatch('test*', $node->name->toString(), FNM_NOESCAPE)) {
             return null;
         }
 

diff --git a/rules/CodeQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector.php b/rules/CodeQuality/Rector/Class_/ConstructClassMethodToSetUpTestCaseRector.php
@@ -7,13 +7,17 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\StaticCall;
+use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Identifier;
 use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\Stmt\Expression;
+use PhpParser\NodeTraverser;
+use PHPStan\Reflection\ClassReflection;
 use Rector\Core\NodeAnalyzer\ClassAnalyzer;
 use Rector\Core\Rector\AbstractRector;
+use Rector\Core\Reflection\ReflectionResolver;
 use Rector\Core\ValueObject\MethodName;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\PHPUnit\NodeAnalyzer\SetUpMethodDecorator;
@@ -33,7 +37,8 @@ public function __construct(
         private readonly TestsNodeAnalyzer $testsNodeAnalyzer,
         private readonly ClassAnalyzer $classAnalyzer,
         private readonly VisibilityManipulator $visibilityManipulator,
-        private readonly SetUpMethodDecorator $setUpMethodDecorator
+        private readonly SetUpMethodDecorator $setUpMethodDecorator,
+        private readonly ReflectionResolver $reflectionResolver
     ) {
     }
 
@@ -105,6 +110,10 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
+        if ($this->shouldSkip($node, $constructClassMethod)) {
+            return null;
+        }
+
         $addedStmts = $this->resolveStmtsToAddToSetUp($constructClassMethod);
         $setUpClassMethod = $node->getMethod(MethodName::SET_UP);
 
@@ -125,6 +134,47 @@ public function refactor(Node $node): ?Node
         return $node;
     }
 
+    private function shouldSkip(Class_ $class, ClassMethod $classMethod): bool
+    {
+        $classReflection = $this->reflectionResolver->resolveClassReflection($class);
+        if (! $classReflection instanceof ClassReflection) {
+            return true;
+        }
+
+        $currentParent = current($classReflection->getParents());
+        if (! $currentParent instanceof ClassReflection) {
+            return true;
+        }
+
+        if ($currentParent->getName() !== 'PHPUnit\Framework\TestCase') {
+            return true;
+        }
+
+        $paramNames = [];
+        foreach ($classMethod->params as $param) {
+            $paramNames[] = $this->getName($param);
+        }
+
+        $isFoundParamUsed = false;
+        $this->traverseNodesWithCallable(
+            (array) $classMethod->stmts,
+            function (Node $subNode) use ($paramNames, &$isFoundParamUsed): ?int {
+                if ($subNode instanceof StaticCall && $this->isName($subNode->name, MethodName::CONSTRUCT)) {
+                    return NodeTraverser::DONT_TRAVERSE_CHILDREN;
+                }
+
+                if ($subNode instanceof Variable && $this->isNames($subNode, $paramNames)) {
+                    $isFoundParamUsed = true;
+                    return NodeTraverser::STOP_TRAVERSAL;
+                }
+
+                return null;
+            }
+        );
+
+        return $isFoundParamUsed;
+    }
+
     /**
      * @return Stmt[]
      */

diff --git a/rules/CodeQuality/Rector/Class_/PreferPHPUnitThisCallRector.php b/rules/CodeQuality/Rector/Class_/PreferPHPUnitThisCallRector.php
@@ -94,7 +94,7 @@ public function refactor(Node $node): ?Node
                 return null;
             }
 
-            if (! $this->isName($node->name, 'assert*')) {
+            if (! fnmatch('assert*', $methodName, FNM_NOESCAPE)) {
                 return null;
             }
 

diff --git a/rules/CodeQuality/Rector/MethodCall/AssertTrueFalseToSpecificMethodRector.php b/rules/CodeQuality/Rector/MethodCall/AssertTrueFalseToSpecificMethodRector.php
@@ -93,7 +93,8 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        $firstArgumentName = $this->getName($firstArgumentValue);
+        $firstArgumentName = $this->resolveFirstArgument($firstArgumentValue);
+
         if ($firstArgumentName === null || ! array_key_exists(
             $firstArgumentName,
             self::FUNCTION_NAME_WITH_ASSERT_METHOD_NAMES
@@ -132,6 +133,13 @@ public function refactor(Node $node): ?Node
         return $node;
     }
 
+    private function resolveFirstArgument(FuncCall|Empty_ $firstArgumentValue): ?string
+    {
+        return $firstArgumentValue instanceof Empty_
+            ? 'empty'
+            : $this->getName($firstArgumentValue);
+    }
+
     private function renameMethod(
         MethodCall|StaticCall $node,
         FunctionNameWithAssertMethods $functionNameWithAssertMethods

diff --git a/rules/PHPUnit100/Rector/Class_/AddProphecyTraitRector.php b/rules/PHPUnit100/Rector/Class_/AddProphecyTraitRector.php
@@ -9,7 +9,6 @@
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\TraitUse;
 use PHPStan\Reflection\ClassReflection;
-use Rector\Core\NodeManipulator\ClassManipulator;
 use Rector\Core\Rector\AbstractRector;
 use Rector\Core\Reflection\ReflectionResolver;
 use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer;

diff --git a/rules/PHPUnit60/Rector/MethodCall/GetMockBuilderGetMockToCreateMockRector.php b/rules/PHPUnit60/Rector/MethodCall/GetMockBuilderGetMockToCreateMockRector.php
@@ -7,7 +7,6 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr\MethodCall;
 use PHPStan\Type\ObjectType;
-use PHPStan\Type\TypeWithClassName;
 use Rector\Core\Rector\AbstractRector;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

diff --git a/rules/PHPUnit70/Rector/Class_/RemoveDataProviderTestPrefixRector.php b/rules/PHPUnit70/Rector/Class_/RemoveDataProviderTestPrefixRector.php
@@ -96,7 +96,7 @@ public function refactor(Node $node): ?Node
 
         $dataProviderClassMethods = $this->dataProviderClassMethodFinder->find($node);
         foreach ($dataProviderClassMethods as $dataProviderClassMethod) {
-            if (! $this->isName($dataProviderClassMethod, 'test*')) {
+            if (! fnmatch('test*', $dataProviderClassMethod->name->toString(), FNM_NOESCAPE)) {
                 continue;
             }
 

diff --git a/src/NodeAnalyzer/AssertCallAnalyzer.php b/src/NodeAnalyzer/AssertCallAnalyzer.php
@@ -7,6 +7,7 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\StaticCall;
+use PhpParser\Node\Identifier;
 use PhpParser\Node\Stmt\ClassMethod;
 use PHPStan\Type\TypeWithClassName;
 use Rector\Core\PhpParser\AstResolver;
@@ -161,16 +162,31 @@ private function resolveClassMethodFromCall(StaticCall | MethodCall $call): ?Cla
 
     private function isAssertMethodName(MethodCall|StaticCall $call): bool
     {
-        return $this->nodeNameResolver->isNames($call->name, [
-            // phpunit
+        if (! $call->name instanceof Identifier) {
+            return false;
+        }
+
+        $callname = $call->name->toString();
+
+        foreach (['doTestFileInfo', 'expectNotToPerformAssertions'] as $methodCallName) {
+            if ($callname === $methodCallName) {
+                return true;
+            }
+        }
+
+        foreach ([
             '*assert',
             'assert*',
             'expectException*',
             'setExpectedException*',
             'expectOutput*',
             'should*',
-            'doTestFileInfo',
-            'expectNotToPerformAssertions',
-        ]);
+        ] as $methodCallNamePattern) {
+            if (fnmatch($methodCallNamePattern, $callname, FNM_NOESCAPE)) {
+                return true;
+            }
+        }
+
+        return false;
     }
 }
diff --git a/src/NodeAnalyzer/TestsNodeAnalyzer.php b/src/NodeAnalyzer/TestsNodeAnalyzer.php
@@ -53,7 +53,7 @@ public function isTestClassMethod(ClassMethod $classMethod): bool
             return false;
         }
 
-        if ($this->nodeNameResolver->isName($classMethod, 'test*')) {
+        if (fnmatch('test*', $classMethod->name->toString(), FNM_NOESCAPE)) {
             return true;
         }