From d17bdaf3566b0c4d1dd2e86a2442ba9e92778697 Mon Sep 17 00:00:00 2001 From: bjorn Date: Sat, 14 Nov 2026 16:52:53 +0100 Subject: [PATCH] [PHPUnit60] Skip tests in classes with asserting lifecycle methods in AddDoesNotPerformAssertionToNonAssertingTestRector (rectorphp/rector#9965) --- ..._not_perform_assertion_lifecycle_bench.php | 215 +++++++++++------- .../skip_asserting_before_in_trait.php.inc | 24 ++ ...kip_asserting_set_up_in_deep_chain.php.inc | 21 ++ ...up_in_deep_chain_and_shallow_class.php.inc | 29 +++ .../skip_asserting_set_up_in_trait.php.inc | 23 ++ ...ng_set_up_in_trait_from_other_file.php.inc | 16 ++ ...sserting_set_up_in_trait_of_parent.php.inc | 13 ++ ...skip_asserting_tear_down_in_parent.php.inc | 13 ++ ...t_up_calling_helper_of_using_class.php.inc | 28 +++ ...lling_parent_of_asserting_ancestor.php.inc | 31 +++ ...ait_set_up_calling_helper_of_trait.php.inc | 106 +++++++++ .../AbstractClassUsingAssertingTrait.php | 12 + .../AbstractClassWithAssertingTearDown.php | 15 ++ .../Source/TraitWithAssertingSetUp.php | 13 ++ ...rformAssertionToNonAssertingTestRector.php | 6 +- src/NodeAnalyzer/AssertCallAnalyzer.php | 24 +- .../LifecycleMethodAssertAnalyzer.php | 204 +++++++++++++++-- 17 files changed, 688 insertions(+), 105 deletions(-) create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_before_in_trait.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain_and_shallow_class.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait_from_other_file.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait_of_parent.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_tear_down_in_parent.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_trait_set_up_calling_helper_of_using_class.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_trait_set_up_calling_parent_of_asserting_ancestor.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/trait_set_up_calling_helper_of_trait.php.inc create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassUsingAssertingTrait.php create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithAssertingTearDown.php create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/TraitWithAssertingSetUp.php diff --git a/bench/does_not_perform_assertion_lifecycle_bench.php b/bench/does_not_perform_assertion_lifecycle_bench.php index 8bbe0d84..324185fb 100644 --- a/bench/does_not_perform_assertion_lifecycle_bench.php +++ b/bench/does_not_perform_assertion_lifecycle_bench.php @@ -6,7 +6,14 @@ * Benchmark for AddDoesNotPerformAssertionToNonAssertingTestRector on test classes with deep parent chains. * * Each test class has parent lifecycle hooks (setUp(), tearDown()) and parent::test*() calls. Resolving these - * via AstResolver parses and decorates parent files on first use, so this bench measures one cold pass, as in a real run. + * via AstResolver parses and decorates parent files on first use, so every scenario measures one cold pass, as in a real run. + * + * Scenarios, all with the same shape (see the constants below): + * - inherited: parents have non-asserting hooks, the test classes have none. Every test method changes. + * - own set up: like "inherited", but each test class has its own setUp() calling parent::setUp(). The file of + * the test class is already parsed by Rector, it must not be parsed a second time to look at its hooks. + * - asserting hooks: the top parent of each chain has an asserting hook (setUp(), assertPostConditions() or a + * #[Before] method), so the lifecycle analyzer walks the whole chain and no test method changes. * * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy. */ @@ -28,15 +35,30 @@ const FILLER_METHODS = 15; // extra methods per parent class, to make parsing realistic const MAX_CPU_SECONDS = 1.0; // fails in CI above this -$directory = sys_get_temp_dir() . '/rector_phpunit_bench_' . getmypid(); -@mkdir($directory); +const SCENARIOS = [ + 'inherited' => [ + 'own_set_up' => false, + 'asserting_hooks' => false, + ], + 'own set up' => [ + 'own_set_up' => true, + 'asserting_hooks' => false, + ], + 'asserting hooks' => [ + 'own_set_up' => false, + 'asserting_hooks' => true, + ], +]; -$writeClass = static function (string $className, string $body) use ($directory): string { - $filePath = $directory . '/' . $className . '.php'; - file_put_contents($filePath, " $scenario) { + // own class names per scenario, so no scenario profits from the parsed files of another + $prefix = 'S' . $scenarioIndex++; - for ($level = 0; $level < DEPTH; ++$level) { - $className = sprintf('AbstractChain%dLevel%d', $chain, $level); + $writeClass = static function (string $className, string $body) use ($directory): string { + $filePath = $directory . '/' . $className . '.php'; + file_put_contents($filePath, "prepare{$level}();\n }\n\n"; - $methods .= " protected function tearDown(): void\n {\n \$this->cleanup = null;\n parent::tearDown();\n }\n\n"; - $methods .= " public function testInherited(): void\n {\n \$value = {$level};\n }\n\n"; - $methods .= " private function prepare{$level}(): void\n {\n \$this->ready = true;\n }\n\n"; + $testFilePaths = []; - 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"; - } + for ($chain = 0; $chain < CHAINS; ++$chain) { + $parentClassName = '\PHPUnit\Framework\TestCase'; - $writeClass( - $className, - "abstract class {$className} extends {$parentClassName}\n{\n protected \$ready;\n protected \$cleanup;\n\n{$methods}}\n" - ); + for ($level = 0; $level < DEPTH; ++$level) { + $className = sprintf('%sAbstractChain%dLevel%d', $prefix, $chain, $level); - $parentClassName = $className; - } + $methods = " protected function setUp(): void\n {\n parent::setUp();\n \$this->prepare{$level}();\n }\n\n"; + $methods .= " protected function tearDown(): void\n {\n \$this->cleanup = null;\n parent::tearDown();\n }\n\n"; + $methods .= " public function testInherited(): void\n {\n \$value = {$level};\n }\n\n"; + $methods .= " private function prepare{$level}(): void\n {\n \$this->ready = true;\n }\n\n"; + + if ($scenario['asserting_hooks'] && $level === 0) { + // the asserting hook is in the top parent, furthest away from the test classes + $methods .= match ($chain % 3) { + 0 => " protected function assertPostConditions(): void\n {\n \$this->assertTrue(\$this->ready);\n }\n\n", + 1 => " #[\PHPUnit\Framework\Attributes\Before]\n protected function checkReady(): void\n {\n \$this->assertNotNull(\$this->cleanup);\n }\n\n", + default => " protected function assertPreConditions(): void\n {\n \$this->assertFalse(\$this->ready);\n }\n\n", + }; + } - for ($test = 0; $test < TESTS_PER_CHAIN; ++$test) { - $className = sprintf('Chain%dTest%d', $chain, $test); + 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"; + } - $methods = " public function testInherited(): void\n {\n parent::testInherited();\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"; + $writeClass( + $className, + "abstract class {$className} extends {$parentClassName}\n{\n protected \$ready;\n protected \$cleanup;\n\n{$methods}}\n" + ); + + $parentClassName = $className; } - $testFilePaths[] = $writeClass( - $className, - "final class {$className} extends {$parentClassName}\n{\n{$methods}}\n" - ); + for ($test = 0; $test < TESTS_PER_CHAIN; ++$test) { + $className = sprintf('%sChain%dTest%d', $prefix, $chain, $test); + + $methods = ''; + if ($scenario['own_set_up']) { + $methods .= " protected function setUp(): void\n {\n parent::setUp();\n \$this->ready = true;\n }\n\n"; + } + + $methods .= " public function testInherited(): void\n {\n parent::testInherited();\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(); + $rectorConfig = (new LazyContainerFactory())->create(); + $rectorConfig->boot(); -/** @var TestingParser $testingParser */ -$testingParser = $rectorConfig->make(TestingParser::class); + /** @var TestingParser $testingParser */ + $testingParser = $rectorConfig->make(TestingParser::class); -/** @var BetterNodeFinder $betterNodeFinder */ -$betterNodeFinder = $rectorConfig->make(BetterNodeFinder::class); + /** @var BetterNodeFinder $betterNodeFinder */ + $betterNodeFinder = $rectorConfig->make(BetterNodeFinder::class); -$rector = $rectorConfig->make(AddDoesNotPerformAssertionToNonAssertingTestRector::class); + $rector = $rectorConfig->make(AddDoesNotPerformAssertionToNonAssertingTestRector::class); -/** @var CurrentFileProvider $currentFileProvider */ -$currentFileProvider = $rectorConfig->make(CurrentFileProvider::class); + // parse test files up front, parent files are left for the rule to resolve + // the rule finds the class of a method in the stmts of the current file, as it does in a real run + /** @var CurrentFileProvider $currentFileProvider */ + $currentFileProvider = $rectorConfig->make(CurrentFileProvider::class); -// parse test files up front, parent files are left for the rule to resolve -// the rule finds the class of a method in the stmts of the current file, as in a real run -$classMethodsByFile = []; -foreach ($testFilePaths as $testFilePath) { - $stmts = $testingParser->parseFileToDecoratedNodes($testFilePath); - $file = new File($testFilePath, (string) file_get_contents($testFilePath)); - $file->hydrateStmtsAndTokens($stmts, $stmts, []); - $classMethodsByFile[] = [$file, $betterNodeFinder->findInstanceOf($stmts, ClassMethod::class)]; -} + $classMethodsByFile = []; + $methodCount = 0; + foreach ($testFilePaths as $testFilePath) { + $stmts = $testingParser->parseFileToDecoratedNodes($testFilePath); -$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; -}; + $file = new File($testFilePath, (string) file_get_contents($testFilePath)); + $file->hydrateStmtsAndTokens($stmts, $stmts, []); -$wallStart = microtime(true); -$cpuStart = $cpuSeconds(); + $classMethods = $betterNodeFinder->findInstanceOf($stmts, ClassMethod::class); + $classMethodsByFile[] = [$file, $classMethods]; + $methodCount += count($classMethods); + } + + $wallStart = microtime(true); + $cpuStart = $cpuSeconds(); -$changedCount = 0; -$classMethodCount = 0; -foreach ($classMethodsByFile as [$file, $classMethods]) { - $currentFileProvider->setFile($file); + $changedCount = 0; + foreach ($classMethodsByFile as [$file, $classMethods]) { + $currentFileProvider->setFile($file); - foreach ($classMethods as $classMethod) { - ++$classMethodCount; - if ($rector->refactor($classMethod) instanceof ClassMethod) { - ++$changedCount; + foreach ($classMethods as $classMethod) { + if ($rector->refactor($classMethod) instanceof ClassMethod) { + ++$changedCount; + } } } -} -$cpu = $cpuSeconds() - $cpuStart; -$wall = microtime(true) - $wallStart; + $cpu = $cpuSeconds() - $cpuStart; + $wall = microtime(true) - $wallStart; + + printf( + '%s: test classes: %d, test methods: %d, changed: %d%s', + $scenarioName, + count($testFilePaths), + $methodCount, + $changedCount, + PHP_EOL + ); + printf('CPU: %.4f s (%.3f ms/method)%s', $cpu, $cpu / $methodCount * 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); + $isFailed = true; + } + +} array_map('unlink', glob($directory . '/*.php')); @rmdir($directory); -printf('test classes: %d, test methods: %d, changed: %d%s', count($testFilePaths), $classMethodCount, $changedCount, PHP_EOL); -printf('CPU: %.4f s (%.3f ms/method)%s', $cpu, $cpu / $classMethodCount * 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); -} +exit($isFailed ? 1 : 0); diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_before_in_trait.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_before_in_trait.php.inc new file mode 100644 index 00000000..2c3d571e --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_before_in_trait.php.inc @@ -0,0 +1,24 @@ +assertTrue(true); + } +} + +final class TraitBefore extends TestCase +{ + use AssertingBeforeTrait; + + public function testSomething() + { + $value = 1; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc new file mode 100644 index 00000000..1e06e53f --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc @@ -0,0 +1,21 @@ +assertTrue(true); } } +abstract class Deep1 extends Deep0 { protected function setUp(): void { parent::setUp(); } } +abstract class Deep2 extends Deep1 { protected function setUp(): void { parent::setUp(); } } +abstract class Deep3 extends Deep2 { protected function setUp(): void { parent::setUp(); } } +abstract class Deep4 extends Deep3 { protected function setUp(): void { parent::setUp(); } } +abstract class Deep5 extends Deep4 { protected function setUp(): void { parent::setUp(); } } +abstract class Deep6 extends Deep5 { protected function setUp(): void { parent::setUp(); } } + +final class DeepTest extends Deep6 +{ + public function testSomething() + { + $value = 1; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain_and_shallow_class.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain_and_shallow_class.php.inc new file mode 100644 index 00000000..8a117518 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain_and_shallow_class.php.inc @@ -0,0 +1,29 @@ +assertTrue(true); } } +abstract class DeepC1 extends DeepC0 { protected function setUp(): void { parent::setUp(); } } +abstract class DeepC2 extends DeepC1 { protected function setUp(): void { parent::setUp(); } } +abstract class DeepC3 extends DeepC2 { protected function setUp(): void { parent::setUp(); } } +abstract class DeepC4 extends DeepC3 { protected function setUp(): void { parent::setUp(); } } +abstract class DeepC5 extends DeepC4 { protected function setUp(): void { parent::setUp(); } } +abstract class DeepC6 extends DeepC5 { protected function setUp(): void { parent::setUp(); } } + +final class DeepCTest extends DeepC6 +{ + public function testSomething() + { + $value = 1; + } +} + +final class DeepCShallowTest extends DeepC2 +{ + public function testOther() + { + $other = 2; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait.php.inc new file mode 100644 index 00000000..5f108918 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait.php.inc @@ -0,0 +1,23 @@ +assertTrue(true); + } +} + +final class TraitSetUp extends TestCase +{ + use AssertingSetUpTrait; + + public function testSomething() + { + $value = 1; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait_from_other_file.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait_from_other_file.php.inc new file mode 100644 index 00000000..d775f286 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_trait_from_other_file.php.inc @@ -0,0 +1,16 @@ +check(); + } +} + +final class SkipSetUpCallingUsingClass extends TestCase +{ + use SetUpCallingUsingClassTrait; + + private function check(): void + { + $this->assertTrue(true); + } + + public function testSomething() + { + $value = 1; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_trait_set_up_calling_parent_of_asserting_ancestor.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_trait_set_up_calling_parent_of_asserting_ancestor.php.inc new file mode 100644 index 00000000..a76e6303 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_trait_set_up_calling_parent_of_asserting_ancestor.php.inc @@ -0,0 +1,31 @@ +assertTrue(true); + } +} + +trait TraitParentAssertingTrait +{ + protected function setUp(): void + { + parent::setUp(); + } +} + +final class SkipTraitParentAssertingTest extends TraitParentAssertingBase +{ + use TraitParentAssertingTrait; + + public function testSomething() + { + $value = 1; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/trait_set_up_calling_helper_of_trait.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/trait_set_up_calling_helper_of_trait.php.inc new file mode 100644 index 00000000..8e45951c --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/trait_set_up_calling_helper_of_trait.php.inc @@ -0,0 +1,106 @@ +check(); + } + + private function check(): void + { + $value = 1; + } +} + +trait TraitHelperAssertingTrait +{ + protected function setUp(): void + { + $this->check(); + } + + private function check(): void + { + $this->assertTrue(true); + } +} + +final class TraitHelperA extends TestCase +{ + use TraitHelperNonAssertingTrait; + + public function testA() + { + $value = 1; + } +} + +final class TraitHelperB extends TestCase +{ + use TraitHelperAssertingTrait; + + public function testB() + { + $value = 1; + } +} +?> +----- +check(); + } + + private function check(): void + { + $value = 1; + } +} + +trait TraitHelperAssertingTrait +{ + protected function setUp(): void + { + $this->check(); + } + + private function check(): void + { + $this->assertTrue(true); + } +} + +final class TraitHelperA extends TestCase +{ + use TraitHelperNonAssertingTrait; + + #[\PHPUnit\Framework\Attributes\DoesNotPerformAssertions] + public function testA() + { + $value = 1; + } +} + +final class TraitHelperB extends TestCase +{ + use TraitHelperAssertingTrait; + + public function testB() + { + $value = 1; + } +} +?> diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassUsingAssertingTrait.php b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassUsingAssertingTrait.php new file mode 100644 index 00000000..1834089d --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassUsingAssertingTrait.php @@ -0,0 +1,12 @@ +assertTrue(true); + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/TraitWithAssertingSetUp.php b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/TraitWithAssertingSetUp.php new file mode 100644 index 00000000..32b6fe9b --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/TraitWithAssertingSetUp.php @@ -0,0 +1,13 @@ +assertTrue(true); + } +} diff --git a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php b/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php index f3308a9a..09d1338e 100644 --- a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php +++ b/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php @@ -175,8 +175,10 @@ private function shouldSkipClassMethod(ClassMethod $classMethod): bool } // assertions in lifecycle methods count toward every test of the class - $class = $this->resolveCurrentClass($classMethod); - if ($class instanceof Class_ && $this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod($class)) { + if ($this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod( + $classMethod, + $this->resolveCurrentClass($classMethod) + )) { return true; } diff --git a/src/NodeAnalyzer/AssertCallAnalyzer.php b/src/NodeAnalyzer/AssertCallAnalyzer.php index 678321ad..b4ac568a 100644 --- a/src/NodeAnalyzer/AssertCallAnalyzer.php +++ b/src/NodeAnalyzer/AssertCallAnalyzer.php @@ -205,7 +205,8 @@ private function hasNestedAssertCall(ClassMethod $classMethod): bool return $this->containsAssertCall($classMethod); } - return false; + // a trait is parsed on its own, so a method of the using class cannot be resolved, assume it asserts + return $this->isSelfReferenceInTrait($node); }); } @@ -251,6 +252,11 @@ private function resolveClassName(StaticCall|MethodCall $call): ?string return $parentClassReflection->getName(); } + // "$this" is unknown in a trait that is parsed on its own, but the methods of the trait itself can be found + if ($this->isSelfReferenceInTrait($call)) { + return $this->resolveClassReflection($call)?->getName(); + } + $objectType = $call instanceof MethodCall ? $this->nodeTypeResolver->getType($call->var) : $this->nodeTypeResolver->getType($call->class); @@ -262,6 +268,22 @@ private function resolveClassName(StaticCall|MethodCall $call): ?string return $objectType->getClassName(); } + private function isSelfReferenceInTrait(StaticCall|MethodCall $call): bool + { + $isSelfReference = $call instanceof MethodCall + ? $this->nodeNameResolver->isName($call->var, 'this') + : $this->nodeNameResolver->isNames( + $call->class, + [ObjectReference::SELF, ObjectReference::STATIC] + ); + + if (! $isSelfReference) { + return false; + } + + return $this->resolveClassReflection($call)?->isTrait() === true; + } + private function resolveClassReflection(Node $node): ?ClassReflection { $scope = $node->getAttribute(AttributeKey::SCOPE); diff --git a/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php b/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php index 5e501e50..0c7a664d 100644 --- a/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php +++ b/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php @@ -5,18 +5,27 @@ namespace Rector\PHPUnit\NodeAnalyzer; use PhpParser\Comment\Doc; +use PhpParser\Node; +use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Stmt\Class_; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; +use PHPStan\Reflection\ClassReflection; use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory; +use Rector\Enum\ObjectReference; use Rector\NodeNameResolver\NodeNameResolver; use Rector\Php80\NodeAnalyzer\PhpAttributeAnalyzer; +use Rector\PhpParser\AstResolver; +use Rector\PhpParser\Node\BetterNodeFinder; use Rector\PHPUnit\Enum\PHPUnitAttribute; -use WeakMap; +use Rector\PHPUnit\Enum\PHPUnitClassName; +use Rector\Reflection\ReflectionResolver; /** - * PHPUnit counts assertions made in lifecycle methods toward every test of the class + * PHPUnit counts assertions made in lifecycle methods toward every test of the class, + * so no test in such a class can be a test without assertions. */ -final readonly class LifecycleMethodAssertAnalyzer +final class LifecycleMethodAssertAnalyzer { /** * @var string[] @@ -39,47 +48,196 @@ private const array HOOK_ANNOTATIONS = ['before', 'after', 'preCondition', 'postCondition']; /** - * @var WeakMap + * Cheap file pre-check, so ancestor and trait files that cannot declare a hook are not parsed. The analyzed class + * itself is not pre-checked, as it is passed in. It must never miss a hook, so it matches every spelling of one: the hook method + * names (in any case, as PHP method names are case-insensitive), the annotations and the attribute class names. An + * attribute is always written with its class name, whether imported, aliased ("PU\Before") or fully qualified. + * The attribute names are case-sensitive on purpose, a lowercase "before" is just prose. A false positive only + * costs a parse. */ - private WeakMap $assertingByClass; + private const string HOOK_FILE_PATTERN = '#(?i:\b(?:setUp|tearDown|assertPreConditions|assertPostConditions)\b|@(?:before|after|preCondition|postCondition)\b)|\b(?:Before|After|PreCondition|PostCondition)\b#'; + + /** + * @var array + */ + private array $assertingByClassAndOverriddenHooks = []; public function __construct( - private AssertCallAnalyzer $assertCallAnalyzer, - private PhpAttributeAnalyzer $phpAttributeAnalyzer, - private PhpDocInfoFactory $phpDocInfoFactory, - private NodeNameResolver $nodeNameResolver, + private readonly ReflectionResolver $reflectionResolver, + private readonly AstResolver $astResolver, + private readonly AssertCallAnalyzer $assertCallAnalyzer, + private readonly PhpAttributeAnalyzer $phpAttributeAnalyzer, + private readonly PhpDocInfoFactory $phpDocInfoFactory, + private readonly BetterNodeFinder $betterNodeFinder, + private readonly NodeNameResolver $nodeNameResolver, ) { - $this->assertingByClass = new WeakMap(); } - public function hasAssertingLifecycleMethod(Class_ $class): bool + /** + * Parsing the file of the analyzed class again is expensive, while Rector already has it. Pass the class when at + * hand, then only its ancestors and traits have to be resolved. + */ + public function hasAssertingLifecycleMethod(ClassMethod $classMethod, ?Class_ $class = null): bool { - // every test method of the class shares the result - return $this->assertingByClass[$class] ??= $this->resolveHasAssertingLifecycleMethod($class); + $classReflection = $this->reflectionResolver->resolveClassReflection($classMethod); + if (! $classReflection instanceof ClassReflection) { + return false; + } + + return $this->resolveHasAssertingLifecycleMethod($classReflection, [], $class); } - private function resolveHasAssertingLifecycleMethod(Class_ $class): bool - { - foreach ($class->getMethods() as $classMethod) { - if (! $this->isLifecycleMethod($classMethod)) { - continue; + /** + * @param string[] $overriddenHookMethodNames + */ + private function resolveHasAssertingLifecycleMethod( + ClassReflection $classReflection, + array $overriddenHookMethodNames, + ?Class_ $class = null + ): bool { + // do not analyze PHPUnit itself + if ($classReflection->getName() === PHPUnitClassName::TEST_CASE) { + return false; + } + + // the result of a parent only depends on the hooks overridden below it, so test classes share the parent chain + sort($overriddenHookMethodNames); + $cacheKey = $classReflection->getName() . '|' . implode(',', $overriddenHookMethodNames); + + $this->assertingByClassAndOverriddenHooks[$cacheKey] ??= $this->resolveClassTraitsAndParents( + $classReflection, + $overriddenHookMethodNames, + $class + ); + + return $this->assertingByClassAndOverriddenHooks[$cacheKey]; + } + + /** + * @param string[] $overriddenHookMethodNames + */ + private function resolveClassTraitsAndParents( + ClassReflection $classReflection, + array $overriddenHookMethodNames, + ?Class_ $class + ): bool { + $class ??= $this->resolveClassLike($classReflection); + + $classMethods = $class instanceof ClassLike ? $class->getMethods() : []; + $newlyOverriddenHookMethodNames = []; + + foreach ($classMethods as $classMethod) { + $hookMethodName = $this->resolveHookMethodName($classMethod); + + // a hook overridden below hides this one, unless the override calls it through "parent::" + if ($hookMethodName !== null && ! $this->hasParentCall($classMethod, $hookMethodName)) { + $newlyOverriddenHookMethodNames[] = $hookMethodName; } - $this->assertCallAnalyzer->resetNesting(); - if ($this->assertCallAnalyzer->containsAssertCall($classMethod)) { + if ($this->isRelevantHook($classMethod, $hookMethodName, $overriddenHookMethodNames) + && $this->containsAssertCall($classMethod)) { return true; } } - return false; + // hooks from traits are used as if declared in the class, an override of the class itself is ignored to stay safe + foreach ($classReflection->getTraits(true) as $traitReflection) { + $trait = $this->resolveClassLike($traitReflection); + if (! $trait instanceof ClassLike) { + continue; + } + + foreach ($trait->getMethods() as $traitMethod) { + $hookMethodName = $this->resolveHookMethodName($traitMethod); + if (! $this->isRelevantHook($traitMethod, $hookMethodName, $overriddenHookMethodNames)) { + continue; + } + + if ($this->containsAssertCall($traitMethod)) { + return true; + } + } + } + + $parentClassReflection = $classReflection->getParentClass(); + if (! $parentClassReflection instanceof ClassReflection) { + return false; + } + + return $this->resolveHasAssertingLifecycleMethod( + $parentClassReflection, + array_values(array_unique([...$overriddenHookMethodNames, ...$newlyOverriddenHookMethodNames])) + ); + } + + private function containsAssertCall(ClassMethod $classMethod): bool + { + // every hook gets its own nesting budget, a hook calling "parent::" is evaluated again in the parent itself + $this->assertCallAnalyzer->resetNesting(); + + return $this->assertCallAnalyzer->containsAssertCall($classMethod); + } + + private function resolveClassLike(ClassReflection $classReflection): ?ClassLike + { + if (! $this->mayDeclareHooks($classReflection)) { + return null; + } + + return $this->astResolver->resolveClassFromClassReflection($classReflection); + } + + private function mayDeclareHooks(ClassReflection $classReflection): bool + { + $fileName = $classReflection->getFileName(); + if ($fileName === null) { + return false; + } + + $fileContents = file_get_contents($fileName); + if ($fileContents === false) { + return true; + } + + return preg_match(self::HOOK_FILE_PATTERN, $fileContents) === 1; } - private function isLifecycleMethod(ClassMethod $classMethod): bool + private function hasParentCall(ClassMethod $classMethod, string $hookMethodName): bool { - if ($this->nodeNameResolver->isNames($classMethod->name, self::HOOK_METHOD_NAMES)) { + return $this->betterNodeFinder->findFirst( + (array) $classMethod->stmts, + fn (Node $node): bool => $node instanceof StaticCall + && $this->nodeNameResolver->isName($node->class, ObjectReference::PARENT) + && $this->nodeNameResolver->isName($node->name, $hookMethodName) + ) instanceof StaticCall; + } + + private function resolveHookMethodName(ClassMethod $classMethod): ?string + { + $methodName = $classMethod->name->toString(); + + foreach (self::HOOK_METHOD_NAMES as $hookMethodName) { + if (strcasecmp($hookMethodName, $methodName) === 0) { + return $hookMethodName; + } + } + + return null; + } + + /** + * @param string[] $overriddenHookMethodNames + */ + private function isRelevantHook( + ClassMethod $classMethod, + ?string $hookMethodName, + array $overriddenHookMethodNames + ): bool { + if ($hookMethodName !== null && ! in_array($hookMethodName, $overriddenHookMethodNames, true)) { return true; } + // all methods marked as hooks run, also when overridden if ($classMethod->attrGroups !== [] && $this->phpAttributeAnalyzer->hasPhpAttributes($classMethod, self::HOOK_ATTRIBUTES)) { return true;