[PHPUnit60] Cache containsAssertCall() per node in AddDoesNotPerformAssertionToNonAssertingTestRector

.github/workflows/code_analysis.yaml

@@ -44,6 +44,10 @@ jobs:
                         name: 'Bench abstract classes'
                         run: php bench/does_not_perform_assertion_abstract_class_bench.php
 
+                    -
+                        name: 'Bench cache'
+                        run: php bench/does_not_perform_assertion_cache_bench.php
+
         name: ${{ matrix.actions.name }}
         runs-on: ubuntu-latest
         timeout-minutes: 10

bench/does_not_perform_assertion_cache_bench.php

@@ -0,0 +1,131 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Benchmark for AddDoesNotPerformAssertionToNonAssertingTestRector on many test classes sharing helper methods.
+ *
+ * Each test method calls a helper declared in an abstract parent, and one calls "parent::setUp()" that is only
+ * declared in TestCase. This measures the containsAssertCall() cache and the "parent::" resolution.
+ *
+ * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy.
+ */
+
+use PhpParser\Node\Stmt\ClassMethod;
+use Rector\DependencyInjection\LazyContainerFactory;
+use Rector\PhpParser\Node\BetterNodeFinder;
+use Rector\PHPUnit\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector;
+use Rector\Testing\TestingParser\TestingParser;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+const PARENTS = 5;             // abstract parents extending TestCase, none overrides setUp()
+const TESTS_PER_PARENT = 60;   // test classes extending each parent
+const TEST_METHODS = 8;        // non-asserting test methods per test class
+const HELPER_STATEMENTS = 5;   // statements per helper, to make printing the body realistic
+const MAX_CPU_SECONDS = 1.0;   // fails in CI above this
+
+$directory = sys_get_temp_dir() . '/rector_phpunit_cache_bench_' . getmypid();
+@mkdir($directory);
+
+$writeClass = static function (string $className, string $body) use ($directory): string {
+    $filePath = $directory . '/' . $className . '.php';
+    file_put_contents($filePath, "<?php\n\nnamespace Bench;\n\n" . $body);
+    return $filePath;
+};
+
+// make generated classes visible to PHPStan reflection
+spl_autoload_register(static function (string $class) use ($directory): void {
+    if (! str_starts_with($class, 'Bench\\')) {
+        return;
+    }
+
+    $filePath = $directory . '/' . substr($class, 6) . '.php';
+    if (is_file($filePath)) {
+        require_once $filePath;
+    }
+});
+
+$testFilePaths = [];
+
+for ($parent = 0; $parent < PARENTS; ++$parent) {
+    $parentClassName = sprintf('AbstractCacheParent%d', $parent);
+
+    $methods = '';
+    for ($helper = 0; $helper < TEST_METHODS; ++$helper) {
+        $statements = '';
+        for ($statement = 0; $statement < HELPER_STATEMENTS; ++$statement) {
+            $statements .= "        \$items = array_map(fn (\$item) => \$item + {$statement}, \$items);\n";
+        }
+
+        $methods .= "    protected function helper{$helper}(array \$items): array\n    {\n{$statements}        return \$items;\n    }\n\n";
+    }
+
+    $writeClass(
+        $parentClassName,
+        "abstract class {$parentClassName} extends \PHPUnit\Framework\TestCase\n{\n{$methods}}\n"
+    );
+
+    for ($test = 0; $test < TESTS_PER_PARENT; ++$test) {
+        $className = sprintf('CacheParent%dTest%d', $parent, $test);
+
+        $methods = "    public function testRestart(): void\n    {\n        parent::setUp();\n    }\n\n";
+        for ($testMethod = 0; $testMethod < TEST_METHODS; ++$testMethod) {
+            $methods .= "    public function testSomething{$testMethod}(): void\n    {\n        \$value = \$this->helper{$testMethod}([1, 2, 3]);\n    }\n\n";
+        }
+
+        $testFilePaths[] = $writeClass(
+            $className,
+            "final class {$className} extends {$parentClassName}\n{\n{$methods}}\n"
+        );
+    }
+}
+
+$rectorConfig = (new LazyContainerFactory())->create();
+$rectorConfig->boot();
+
+/** @var TestingParser $testingParser */
+$testingParser = $rectorConfig->make(TestingParser::class);
+
+/** @var BetterNodeFinder $betterNodeFinder */
+$betterNodeFinder = $rectorConfig->make(BetterNodeFinder::class);
+
+$rector = $rectorConfig->make(AddDoesNotPerformAssertionToNonAssertingTestRector::class);
+
+// parse test files up front, parent files are left for the rule to resolve
+$classMethods = [];
+foreach ($testFilePaths as $testFilePath) {
+    $stmts = $testingParser->parseFileToDecoratedNodes($testFilePath);
+    $classMethods = [...$classMethods, ...$betterNodeFinder->findInstanceOf($stmts, ClassMethod::class)];
+}
+
+$cpuSeconds = static function (): float {
+    $usage = getrusage();
+    return $usage['ru_utime.tv_sec'] + $usage['ru_utime.tv_usec'] / 1_000_000
+        + $usage['ru_stime.tv_sec'] + $usage['ru_stime.tv_usec'] / 1_000_000;
+};
+
+$wallStart = microtime(true);
+$cpuStart = $cpuSeconds();
+
+$changedCount = 0;
+foreach ($classMethods as $classMethod) {
+    if ($rector->refactor($classMethod) instanceof ClassMethod) {
+        ++$changedCount;
+    }
+}
+
+$cpu = $cpuSeconds() - $cpuStart;
+$wall = microtime(true) - $wallStart;
+
+array_map('unlink', glob($directory . '/*.php'));
+@rmdir($directory);
+
+printf('test classes: %d, test methods: %d, changed: %d%s', count($testFilePaths), count($classMethods), $changedCount, PHP_EOL);
+printf('CPU:  %.4f s  (%.3f ms/method)%s', $cpu, $cpu / count($classMethods) * 1000, PHP_EOL);
+printf('wall: %.4f s%s', $wall, PHP_EOL);
+
+if ($cpu > MAX_CPU_SECONDS) {
+    printf('CPU %.4f s is over the %.1f s limit%s', $cpu, MAX_CPU_SECONDS, PHP_EOL);
+    exit(1);
+}

src/NodeAnalyzer/AssertCallAnalyzer.php

@@ -19,14 +19,15 @@
 use Rector\NodeTypeResolver\NodeTypeResolver;
 use Rector\PhpParser\AstResolver;
 use Rector\PhpParser\Node\BetterNodeFinder;
-use Rector\PhpParser\Printer\BetterStandardPrinter;
 use Rector\PHPUnit\Enum\PHPUnitClassName;
 use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType;
 
 final class AssertCallAnalyzer
 {
     private const int MAX_NESTED_METHOD_CALL_LEVEL = 5;
 
+    private const string CONTAINS_ASSERT_CALL_ATTRIBUTE = 'contains_assert_call';
+
     /**
      * @see https://docs.phpunit.de/en/12.4/assertions.html
      */
@@ -45,19 +46,13 @@ final class AssertCallAnalyzer
         'should',
     ];
 
-    /**
-     * @var array<string, bool>
-     */
-    private array $containsAssertCallByClassMethod = [];
-
     /**
      * This should prevent segfaults while going too deep into to parsed code. Without it, it might end-up with segfault
      */
     private int $classMethodNestingLevel = 0;
 
     public function __construct(
         private readonly AstResolver $astResolver,
-        private readonly BetterStandardPrinter $betterStandardPrinter,
         private readonly BetterNodeFinder $betterNodeFinder,
         private readonly NodeNameResolver $nodeNameResolver,
         private readonly NodeTypeResolver $nodeTypeResolver,
@@ -79,25 +74,22 @@ public function containsAssertCall(ClassMethod $classMethod): bool
                 return false;
             }
 
-            // the same body means a different method per class, e.g. "$this->helper()" or "parent::test()"
-            $cacheHash = md5(
-                $this->betterStandardPrinter->prettyPrint([$classMethod]) . $this->resolveOwnClassName($classMethod)
-            );
-
-            if (isset($this->containsAssertCallByClassMethod[$cacheHash])) {
-                return $this->containsAssertCallByClassMethod[$cacheHash];
+            // cached per node, as the same body means a different method per class, e.g. "$this->helper()"
+            $containsAssertCall = $classMethod->getAttribute(self::CONTAINS_ASSERT_CALL_ATTRIBUTE);
+            if (is_bool($containsAssertCall)) {
+                return $containsAssertCall;
             }
 
             // A. try "->assert" shallow search first for performance
             $hasDirectAssertOrMockCall = $this->hasDirectAssertOrMockCall($classMethod);
             if ($hasDirectAssertOrMockCall) {
-                $this->containsAssertCallByClassMethod[$cacheHash] = true;
+                $classMethod->setAttribute(self::CONTAINS_ASSERT_CALL_ATTRIBUTE, true);
                 return true;
             }
 
             // B. look for nested calls
             $hasNestedAssertOrMockCall = $this->hasNestedAssertCall($classMethod);
-            $this->containsAssertCallByClassMethod[$cacheHash] = $hasNestedAssertOrMockCall;
+            $classMethod->setAttribute(self::CONTAINS_ASSERT_CALL_ATTRIBUTE, $hasNestedAssertOrMockCall);
 
             return $hasNestedAssertOrMockCall;
         } finally {
@@ -216,25 +208,15 @@ private function resolveClassMethodFromCall(StaticCall|MethodCall $call): ?Class
             return null;
         }
 
-        $className = $this->resolveClassName($call);
+        $className = $this->resolveClassName($call, $methodName);
         if ($className === null) {
             return null;
         }
 
         return $this->astResolver->resolveClassMethod($className, $methodName);
     }
 
-    private function resolveOwnClassName(ClassMethod $classMethod): string
-    {
-        $classReflection = $this->resolveClassReflection($classMethod);
-        if (! $classReflection instanceof ClassReflection) {
-            return '';
-        }
-
-        return $classReflection->getName();
-    }
-
-    private function resolveClassName(StaticCall|MethodCall $call): ?string
+    private function resolveClassName(StaticCall|MethodCall $call, string $methodName): ?string
     {
         if ($call instanceof StaticCall && $this->nodeNameResolver->isName($call->class, ObjectReference::PARENT)) {
             // "parent" resolves to a union of all ancestors in type resolver, so use the direct parent class instead
@@ -243,12 +225,21 @@ private function resolveClassName(StaticCall|MethodCall $call): ?string
                 return null;
             }
 
+            if (! $parentClassReflection->hasNativeMethod($methodName)) {
+                return null;
+            }
+
+            // the method can be inherited from any ancestor
+            $declaringClassName = $parentClassReflection->getNativeMethod($methodName)
+                ->getDeclaringClass()
+                ->getName();
+
             // PHPUnit itself has no asserting test methods, no need to parse its huge classes
-            if ($parentClassReflection->getName() === PHPUnitClassName::TEST_CASE) {
+            if ($declaringClassName === PHPUnitClassName::TEST_CASE) {
                 return null;
             }
 
-            return $parentClassReflection->getName();
+            return $declaringClassName;
         }
 
         $objectType = $call instanceof MethodCall
@@ -262,9 +253,9 @@ private function resolveClassName(StaticCall|MethodCall $call): ?string
         return $objectType->getClassName();
     }
 
-    private function resolveClassReflection(Node $node): ?ClassReflection
+    private function resolveClassReflection(StaticCall $staticCall): ?ClassReflection
     {
-        $scope = $node->getAttribute(AttributeKey::SCOPE);
+        $scope = $staticCall->getAttribute(AttributeKey::SCOPE);
         if (! $scope instanceof Scope) {
             return null;
         }