[PHPUnit60] Skip tests in classes with asserting parent lifecycle methods in AddDoesNotPerformAssertionToNonAssertingTestRector

.github/workflows/code_analysis.yaml

@@ -56,6 +56,10 @@ jobs:
                         name: 'Bench cache'
                         run: php bench/does_not_perform_assertion_cache_bench.php
 
+                    -
+                        name: 'Bench parent lifecycle methods'
+                        run: php bench/does_not_perform_assertion_parent_lifecycle_bench.php
+
         name: ${{ matrix.actions.name }}
         runs-on: ubuntu-latest
         timeout-minutes: 10

bench/does_not_perform_assertion_parent_lifecycle_bench.php

@@ -0,0 +1,139 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Benchmark for AddDoesNotPerformAssertionToNonAssertingTestRector on test classes with parent lifecycle methods.
+ *
+ * Test classes extend chains of abstract parents, every parent has setUp() and tearDown() calling "parent::".
+ * Every other chain asserts in setUp() of its top parent, every third test class overrides setUp() without "parent::".
+ * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy.
+ */
+
+use PhpParser\Node\Stmt\Class_;
+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 CHAINS = 10;           // independent parent chains, every other one asserts in setUp() of its top parent
+const DEPTH = 4;             // abstract parents per chain, between TestCase and the test class
+const TESTS_PER_CHAIN = 30;  // test classes extending the deepest parent of each chain
+const TEST_METHODS = 5;      // non-asserting test methods per test class
+const FILLER_METHODS = 15;   // extra methods per parent class, to make parsing realistic
+const MAX_CPU_SECONDS = 1.5; // fails in CI above this
+
+$directory = sys_get_temp_dir() . '/rector_phpunit_parent_lifecycle_bench_' . getmypid();
+@mkdir($directory);
+
+// 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;
+    }
+});
+
+$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;
+};
+
+$testFilePaths = [];
+
+for ($chain = 0; $chain < CHAINS; ++$chain) {
+    $isAssertingChain = $chain % 2 === 0;
+    $parentClassName = '\PHPUnit\Framework\TestCase';
+
+    for ($level = 0; $level < DEPTH; ++$level) {
+        $className = sprintf('AbstractChain%dLevel%d', $chain, $level);
+
+        $setUpBody = $isAssertingChain && $level === 0 ? '$this->assertTrue(true);' : '$this->ready = true;';
+        $methods = "    protected function setUp(): void\n    {\n        parent::setUp();\n        {$setUpBody}\n    }\n\n";
+        $methods .= "    protected function tearDown(): void\n    {\n        \$this->ready = null;\n        parent::tearDown();\n    }\n\n";
+
+        for ($filler = 0; $filler < FILLER_METHODS; ++$filler) {
+            $methods .= "    protected function helper{$filler}(array \$items): array\n    {\n        return array_map(fn (\$item) => \$item * {$filler}, \$items);\n    }\n\n";
+        }
+
+        $writeClass(
+            $className,
+            "abstract class {$className} extends {$parentClassName}\n{\n    protected \$ready;\n\n{$methods}}\n"
+        );
+
+        $parentClassName = $className;
+    }
+
+    for ($test = 0; $test < TESTS_PER_CHAIN; ++$test) {
+        $className = sprintf('Chain%dTest%d', $chain, $test);
+
+        // hides the parent setUp()
+        $methods = $test % 3 === 0
+            ? "    protected function setUp(): void\n    {\n        \$this->ready = true;\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);
+
+$classes = [];
+foreach ($testFilePaths as $testFilePath) {
+    $stmts = $testingParser->parseFileToDecoratedNodes($testFilePath);
+    $classes = [...$classes, ...$betterNodeFinder->findInstanceOf($stmts, Class_::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 ($classes as $class) {
+    if ($rector->refactor($class) instanceof Class_) {
+        ++$changedCount;
+    }
+}
+
+$cpu = $cpuSeconds() - $cpuStart;
+$wall = microtime(true) - $wallStart;
+
+array_map('unlink', glob($directory . '/*.php'));
+@rmdir($directory);
+
+printf('test classes: %d, changed: %d%s', count($classes), $changedCount, PHP_EOL);
+printf('CPU:  %.4f s  (%.3f ms/class)%s', $cpu, $cpu / count($classes) * 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);
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/override_asserting_parent_tear_down.php.inc

@@ -0,0 +1,42 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source\AbstractClassWithAssertingTearDown;
+
+class OverrideAssertingParentTearDown extends AbstractClassWithAssertingTearDown
+{
+    protected function tearDown(): void
+    {
+        $this->value = null;
+    }
+
+    public function testSomething()
+    {
+        $value = 5;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source\AbstractClassWithAssertingTearDown;
+
+class OverrideAssertingParentTearDown extends AbstractClassWithAssertingTearDown
+{
+    protected function tearDown(): void
+    {
+        $this->value = null;
+    }
+
+    #[\PHPUnit\Framework\Attributes\DoesNotPerformAssertions]
+    public function testSomething()
+    {
+        $value = 5;
+    }
+}
+
+?>

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc

@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class Deep0 extends TestCase { protected function setUp(): void { $this->assertTrue(true); } }
+abstract class Deep1 extends Deep0 { protected function setUp(): void { parent::setUp(); } }
+abstract class Deep2 extends Deep1 { protected function setUp(): void { parent::setUp(); } }
+
+final class DeepTest extends Deep2
+{
+    public function testSomething()
+    {
+        $value = 1;
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain_and_shallow_class.php.inc

@@ -0,0 +1,25 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class DeepC0 extends TestCase { protected function setUp(): void { $this->assertTrue(true); } }
+abstract class DeepC1 extends DeepC0 { protected function setUp(): void { parent::setUp(); } }
+abstract class DeepC2 extends DeepC1 { protected function setUp(): void { parent::setUp(); } }
+
+final class DeepCTest extends DeepC2
+{
+    public function testSomething()
+    {
+        $value = 1;
+    }
+}
+
+final class DeepCShallowTest extends DeepC1
+{
+    public function testOther()
+    {
+        $other = 2;
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_via_parent_trait.php.inc

@@ -0,0 +1,13 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source\AbstractClassWithTraitAssertingSetUp;
+
+class SkipAssertingSetUpViaParentTrait extends AbstractClassWithTraitAssertingSetUp
+{
+    public function testSomething()
+    {
+        $value = 5;
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_tear_down_in_parent.php.inc

@@ -0,0 +1,13 @@
+<?php
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Fixture;
+
+use Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source\AbstractClassWithAssertingTearDown;
+
+class SkipAssertingTearDownInParent extends AbstractClassWithAssertingTearDown
+{
+    public function testSomething()
+    {
+        $value = 5;
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithAssertingTearDown.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class AbstractClassWithAssertingTearDown extends TestCase
+{
+    protected function tearDown(): void
+    {
+        $this->assertTrue(true);
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithTraitAssertingSetUp.php

@@ -0,0 +1,17 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source;
+
+use PHPUnit\Framework\TestCase;
+
+abstract class AbstractClassWithTraitAssertingSetUp extends TestCase
+{
+    use AssertingHelperTrait;
+
+    protected function setUp(): void
+    {
+        $this->verifyReady();
+    }
+}

rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AssertingHelperTrait.php

@@ -0,0 +1,13 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\PHPUnit\Tests\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector\Source;
+
+trait AssertingHelperTrait
+{
+    protected function verifyReady(): void
+    {
+        $this->assertTrue(true);
+    }
+}

rules/CodeQuality/Rector/ClassMethod/DataProviderArrayItemsNewLinedRector.php

@@ -107,7 +107,7 @@ public function refactor(Node $node): ?Node
         // find array in data provider - must contain a return node
         /** @var Return_[] $returns */
         $returns = $this->betterNodeFinder->findInstanceOf((array) $node->stmts, Return_::class);
-        $hasChanged = \false;
+        $hasChanged = false;
         foreach ($returns as $return) {
             if (! $return->expr instanceof Array_) {
                 continue;
@@ -123,10 +123,10 @@ public function refactor(Node $node): ?Node
             }
 
             // ensure newlined printed
-            $array->setAttribute(AttributeKey::NEWLINED_ARRAY_PRINT, \true);
+            $array->setAttribute(AttributeKey::NEWLINED_ARRAY_PRINT, true);
             // invoke reprint
             $array->setAttribute(AttributeKey::ORIGINAL_NODE, null);
-            $hasChanged = \true;
+            $hasChanged = true;
         }
 
         if ($hasChanged) {

rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php

@@ -128,7 +128,7 @@ public function refactor(Node $node): ?Node
         }
 
         // assertions in lifecycle methods count toward every test of the class
-        if ($this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod($node)) {
+        if ($this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod($node, $scope)) {
             return null;
         }