From 9044f2eb704d3534a622efc0a0aeb03b7746e331 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 15 Nov 2026 00:48:40 +0100 Subject: [PATCH 1/2] [PHPUnit60] Skip tests in classes with asserting parent lifecycle methods in AddDoesNotPerformAssertionToNonAssertingTestRector --- ...verride_asserting_parent_tear_down.php.inc | 42 ++++ ...kip_asserting_set_up_in_deep_chain.php.inc | 17 ++ ...up_in_deep_chain_and_shallow_class.php.inc | 25 +++ ..._asserting_set_up_via_parent_trait.php.inc | 13 ++ ...skip_asserting_tear_down_in_parent.php.inc | 13 ++ .../AbstractClassWithAssertingTearDown.php | 15 ++ .../AbstractClassWithTraitAssertingSetUp.php | 17 ++ .../Source/AssertingHelperTrait.php | 13 ++ .../DataProviderArrayItemsNewLinedRector.php | 6 +- ...rformAssertionToNonAssertingTestRector.php | 2 +- .../LifecycleMethodAssertAnalyzer.php | 205 ++++++++++++++++-- 11 files changed, 340 insertions(+), 28 deletions(-) create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/override_asserting_parent_tear_down.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_via_parent_trait.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/Source/AbstractClassWithAssertingTearDown.php create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithTraitAssertingSetUp.php create mode 100644 rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AssertingHelperTrait.php diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/override_asserting_parent_tear_down.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/override_asserting_parent_tear_down.php.inc new file mode 100644 index 00000000..a69e9219 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/override_asserting_parent_tear_down.php.inc @@ -0,0 +1,42 @@ +value = null; + } + + public function testSomething() + { + $value = 5; + } +} + +?> +----- +value = null; + } + + #[\PHPUnit\Framework\Attributes\DoesNotPerformAssertions] + public function testSomething() + { + $value = 5; + } +} + +?> 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..8edf43f4 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_in_deep_chain.php.inc @@ -0,0 +1,17 @@ +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; + } +} 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..2cc7e483 --- /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,25 @@ +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; + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_via_parent_trait.php.inc b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_via_parent_trait.php.inc new file mode 100644 index 00000000..97e54b45 --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Fixture/skip_asserting_set_up_via_parent_trait.php.inc @@ -0,0 +1,13 @@ +assertTrue(true); + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithTraitAssertingSetUp.php b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithTraitAssertingSetUp.php new file mode 100644 index 00000000..f3760aac --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AbstractClassWithTraitAssertingSetUp.php @@ -0,0 +1,17 @@ +verifyReady(); + } +} diff --git a/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AssertingHelperTrait.php b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AssertingHelperTrait.php new file mode 100644 index 00000000..1041efbd --- /dev/null +++ b/rules-tests/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector/Source/AssertingHelperTrait.php @@ -0,0 +1,13 @@ +assertTrue(true); + } +} diff --git a/rules/CodeQuality/Rector/ClassMethod/DataProviderArrayItemsNewLinedRector.php b/rules/CodeQuality/Rector/ClassMethod/DataProviderArrayItemsNewLinedRector.php index c37e464b..d5deab56 100644 --- a/rules/CodeQuality/Rector/ClassMethod/DataProviderArrayItemsNewLinedRector.php +++ b/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) { diff --git a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php b/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php index 65abcee9..0e2f8021 100644 --- a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php +++ b/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; } diff --git a/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php b/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php index 5e501e50..18b8cc2b 100644 --- a/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php +++ b/src/NodeAnalyzer/LifecycleMethodAssertAnalyzer.php @@ -5,18 +5,26 @@ 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\Analyser\Scope; +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; /** - * PHPUnit counts assertions made in lifecycle methods toward every test of the class + * PHPUnit counts assertions made in lifecycle methods of the class and its parents toward every test of the class */ -final readonly class LifecycleMethodAssertAnalyzer +final class LifecycleMethodAssertAnalyzer { /** * @var string[] @@ -39,47 +47,196 @@ private const array HOOK_ANNOTATIONS = ['before', 'after', 'preCondition', 'postCondition']; /** - * @var WeakMap + * Every parent class is parsed with full scope, so stop walking deep chains for performance */ - private WeakMap $assertingByClass; + private const int MAX_PARENT_CLASS_LEVEL = 5; + + /** + * Assert, expectation and mock calls detected by AssertCallAnalyzer, matched in raw file contents + */ + private const string ASSERT_LIKE_TEXT_REGEX = '#assert|expect|should|mockbuilder|prophe#i'; + + /** + * @var array + */ + private array $assertingByParentClassAndOverriddenHooks = []; + + /** + * @var array + */ + private array $hasAssertLikeTextByClassName = []; public function __construct( - private AssertCallAnalyzer $assertCallAnalyzer, - private PhpAttributeAnalyzer $phpAttributeAnalyzer, - private PhpDocInfoFactory $phpDocInfoFactory, - private NodeNameResolver $nodeNameResolver, + private readonly AssertCallAnalyzer $assertCallAnalyzer, + private readonly PhpAttributeAnalyzer $phpAttributeAnalyzer, + private readonly PhpDocInfoFactory $phpDocInfoFactory, + private readonly NodeNameResolver $nodeNameResolver, + private readonly AstResolver $astResolver, + private readonly BetterNodeFinder $betterNodeFinder, ) { - $this->assertingByClass = new WeakMap(); } - public function hasAssertingLifecycleMethod(Class_ $class): bool + public function hasAssertingLifecycleMethod(Class_ $class, Scope $scope): bool { - // every test method of the class shares the result - return $this->assertingByClass[$class] ??= $this->resolveHasAssertingLifecycleMethod($class); + $classReflection = $scope->getClassReflection(); + if (! $classReflection instanceof ClassReflection) { + return false; + } + + return $this->isClassLikeAsserting($class, $classReflection, [], 0); } - private function resolveHasAssertingLifecycleMethod(Class_ $class): bool - { - foreach ($class->getMethods() as $classMethod) { - if (! $this->isLifecycleMethod($classMethod)) { - continue; + /** + * @param string[] $overriddenHookMethodNames + */ + private function isClassLikeAsserting( + ?ClassLike $classLike, + ClassReflection $classReflection, + array $overriddenHookMethodNames, + int $parentClassLevel + ): bool { + $classMethods = $classLike instanceof ClassLike ? $classLike->getMethods() : []; + + foreach ($classMethods as $classMethod) { + $hookMethodName = $this->resolveHookMethodName($classMethod); + + if ($this->isRelevantHook($classMethod, $hookMethodName, $overriddenHookMethodNames) + && $this->containsAssertCall($classMethod)) { + return true; } - $this->assertCallAnalyzer->resetNesting(); - if ($this->assertCallAnalyzer->containsAssertCall($classMethod)) { - return true; + // a hook overridden here hides the parent one, unless it calls it through "parent::" + if ($hookMethodName !== null && ! $this->hasParentCall($classMethod, $hookMethodName)) { + $overriddenHookMethodNames[] = $hookMethodName; } } - return false; + if ($parentClassLevel >= self::MAX_PARENT_CLASS_LEVEL) { + return false; + } + + $parentClassReflection = $classReflection->getParentClass(); + if (! $parentClassReflection instanceof ClassReflection) { + return false; + } + + // parsing parents with full scope is expensive, skip chains without any assert-like text first + if (! $this->hasAssertLikeTextInClassChain($parentClassReflection)) { + return false; + } + + return $this->isParentClassAsserting($parentClassReflection, $overriddenHookMethodNames, $parentClassLevel + 1); + } + + /** + * @param string[] $overriddenHookMethodNames + */ + private function isParentClassAsserting( + ClassReflection $classReflection, + array $overriddenHookMethodNames, + int $parentClassLevel + ): bool { + // do not analyze PHPUnit and Symfony base test cases, their hooks do not count as test assertions + if (in_array($classReflection->getName(), PHPUnitClassName::FRAMEWORK_TEST_CASES, true)) { + return false; + } + + // the result of a parent only depends on the hooks overridden below it and its level, so test classes share the parent chain + $overriddenHookMethodNames = array_unique($overriddenHookMethodNames); + sort($overriddenHookMethodNames); + $cacheKey = $classReflection->getName() . '|' . implode(',', $overriddenHookMethodNames) . '|' . $parentClassLevel; + + return $this->assertingByParentClassAndOverriddenHooks[$cacheKey] ??= $this->isClassLikeAsserting( + $this->astResolver->resolveClassFromClassReflection($classReflection), + $classReflection, + $overriddenHookMethodNames, + $parentClassLevel + ); + } + + private function hasAssertLikeTextInClassChain(ClassReflection $classReflection): bool + { + $className = $classReflection->getName(); + if (in_array($className, PHPUnitClassName::FRAMEWORK_TEST_CASES, true)) { + return false; + } + + if (isset($this->hasAssertLikeTextByClassName[$className])) { + return $this->hasAssertLikeTextByClassName[$className]; + } + + // hooks can call helpers from used traits or from any parent, so check the whole chain + $classReflections = [$classReflection, ...array_values($classReflection->getTraits(true))]; + $hasAssertLikeText = array_any( + $classReflections, + fn (ClassReflection $currentClassReflection): bool => $this->hasAssertLikeText( + $currentClassReflection->getFileName() + ) + ); + + if (! $hasAssertLikeText) { + $parentClassReflection = $classReflection->getParentClass(); + $hasAssertLikeText = $parentClassReflection instanceof ClassReflection + && $this->hasAssertLikeTextInClassChain($parentClassReflection); + } + + return $this->hasAssertLikeTextByClassName[$className] = $hasAssertLikeText; + } + + private function hasAssertLikeText(?string $fileName): bool + { + // internal classes have no file to parse either + if ($fileName === null) { + return false; + } + + return preg_match(self::ASSERT_LIKE_TEXT_REGEX, (string) file_get_contents($fileName)) === 1; + } + + private function containsAssertCall(ClassMethod $classMethod): bool + { + // every hook gets its own nesting budget, a "parent::" hook call is evaluated again in the parent itself + $this->assertCallAnalyzer->resetNesting(); + + return $this->assertCallAnalyzer->containsAssertCall($classMethod); + } + + private function hasParentCall(ClassMethod $classMethod, string $hookMethodName): bool + { + 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 isLifecycleMethod(ClassMethod $classMethod): bool + private function resolveHookMethodName(ClassMethod $classMethod): ?string { - if ($this->nodeNameResolver->isNames($classMethod->name, self::HOOK_METHOD_NAMES)) { + $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; } + // methods marked as hooks run too, also when overridden if ($classMethod->attrGroups !== [] && $this->phpAttributeAnalyzer->hasPhpAttributes($classMethod, self::HOOK_ATTRIBUTES)) { return true; From c87a7fe38c760cf2f7b19adc4eec4cd51081ba66 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 15 Nov 2026 00:48:40 +0100 Subject: [PATCH 2/2] [bench] Add AddDoesNotPerformAssertionToNonAssertingTestRector bench on parent lifecycle methods --- .github/workflows/code_analysis.yaml | 4 + ...rform_assertion_parent_lifecycle_bench.php | 139 ++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 bench/does_not_perform_assertion_parent_lifecycle_bench.php diff --git a/.github/workflows/code_analysis.yaml b/.github/workflows/code_analysis.yaml index 8fb0a9b5..b1dffd70 100644 --- a/.github/workflows/code_analysis.yaml +++ b/.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 diff --git a/bench/does_not_perform_assertion_parent_lifecycle_bench.php b/bench/does_not_perform_assertion_parent_lifecycle_bench.php new file mode 100644 index 00000000..fe5e45cb --- /dev/null +++ b/bench/does_not_perform_assertion_parent_lifecycle_bench.php @@ -0,0 +1,139 @@ +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); +}