diff --git a/bench/does_not_perform_assertion_abstract_class_bench.php b/bench/does_not_perform_assertion_abstract_class_bench.php index 364392c4..2342aee1 100644 --- a/bench/does_not_perform_assertion_abstract_class_bench.php +++ b/bench/does_not_perform_assertion_abstract_class_bench.php @@ -5,11 +5,11 @@ /** * Benchmark for AddDoesNotPerformAssertionToNonAssertingTestRector on abstract and final test classes. * - * Half of the test classes are abstract, so the abstract class check runs on every test method. + * Half of the test classes are abstract, so the abstract class check runs on every test class. * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy. */ -use PhpParser\Node\Stmt\ClassMethod; +use PhpParser\Node\Stmt\Class_; use Rector\DependencyInjection\LazyContainerFactory; use Rector\PhpParser\Node\BetterNodeFinder; use Rector\PHPUnit\PHPUnit60\Rector\ClassMethod\AddDoesNotPerformAssertionToNonAssertingTestRector; @@ -68,10 +68,10 @@ $rector = $rectorConfig->make(AddDoesNotPerformAssertionToNonAssertingTestRector::class); -$classMethods = []; +$classes = []; foreach ($testFilePaths as $testFilePath) { $stmts = $testingParser->parseFileToDecoratedNodes($testFilePath); - $classMethods = [...$classMethods, ...$betterNodeFinder->findInstanceOf($stmts, ClassMethod::class)]; + $classes = [...$classes, ...$betterNodeFinder->findInstanceOf($stmts, Class_::class)]; } $cpuSeconds = static function (): float { @@ -84,8 +84,8 @@ $cpuStart = $cpuSeconds(); $changedCount = 0; -foreach ($classMethods as $classMethod) { - if ($rector->refactor($classMethod) instanceof ClassMethod) { +foreach ($classes as $class) { + if ($rector->refactor($class) instanceof Class_) { ++$changedCount; } } @@ -96,8 +96,8 @@ 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('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) { diff --git a/bench/does_not_perform_assertion_lifecycle_bench.php b/bench/does_not_perform_assertion_lifecycle_bench.php index 8bbe0d84..fa8df5e8 100644 --- a/bench/does_not_perform_assertion_lifecycle_bench.php +++ b/bench/does_not_perform_assertion_lifecycle_bench.php @@ -11,13 +11,11 @@ * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy. */ -use PhpParser\Node\Stmt\ClassMethod; -use Rector\Application\Provider\CurrentFileProvider; +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; -use Rector\ValueObject\Application\File; require __DIR__ . '/../vendor/autoload.php'; @@ -100,17 +98,11 @@ $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 in a real run -$classMethodsByFile = []; +$classes = []; 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)]; + $classes = [...$classes, ...$betterNodeFinder->findInstanceOf($stmts, Class_::class)]; } $cpuSeconds = static function (): float { @@ -123,15 +115,9 @@ $cpuStart = $cpuSeconds(); $changedCount = 0; -$classMethodCount = 0; -foreach ($classMethodsByFile as [$file, $classMethods]) { - $currentFileProvider->setFile($file); - - foreach ($classMethods as $classMethod) { - ++$classMethodCount; - if ($rector->refactor($classMethod) instanceof ClassMethod) { - ++$changedCount; - } +foreach ($classes as $class) { + if ($rector->refactor($class) instanceof Class_) { + ++$changedCount; } } @@ -141,8 +127,8 @@ 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('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) { diff --git a/bench/does_not_perform_assertion_own_lifecycle_bench.php b/bench/does_not_perform_assertion_own_lifecycle_bench.php index d3c8a08a..6c0b6ab8 100644 --- a/bench/does_not_perform_assertion_own_lifecycle_bench.php +++ b/bench/does_not_perform_assertion_own_lifecycle_bench.php @@ -5,18 +5,15 @@ /** * Benchmark for AddDoesNotPerformAssertionToNonAssertingTestRector on test classes with own lifecycle methods. * - * Every test class has its own setUp() and tearDown(), every other one asserts in setUp(). The class is taken from - * the current file, so the rule looks into its lifecycle methods for every test method. + * Every test class has its own setUp() and tearDown(), every other one asserts in setUp(). * Run on both branches to compare. CPU (user+sys) is the reliable metric; wall is noisy. */ -use PhpParser\Node\Stmt\ClassMethod; -use Rector\Application\Provider\CurrentFileProvider; +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; -use Rector\ValueObject\Application\File; require __DIR__ . '/../vendor/autoload.php'; @@ -72,16 +69,10 @@ $rector = $rectorConfig->make(AddDoesNotPerformAssertionToNonAssertingTestRector::class); -/** @var CurrentFileProvider $currentFileProvider */ -$currentFileProvider = $rectorConfig->make(CurrentFileProvider::class); - -// the rule finds the class of a method in the stmts of the current file, as in a real run -$classMethodsByFile = []; +$classes = []; 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)]; + $classes = [...$classes, ...$betterNodeFinder->findInstanceOf($stmts, Class_::class)]; } $cpuSeconds = static function (): float { @@ -94,15 +85,9 @@ $cpuStart = $cpuSeconds(); $changedCount = 0; -$classMethodCount = 0; -foreach ($classMethodsByFile as [$file, $classMethods]) { - $currentFileProvider->setFile($file); - - foreach ($classMethods as $classMethod) { - ++$classMethodCount; - if ($rector->refactor($classMethod) instanceof ClassMethod) { - ++$changedCount; - } +foreach ($classes as $class) { + if ($rector->refactor($class) instanceof Class_) { + ++$changedCount; } } @@ -112,8 +97,8 @@ 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('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) { diff --git a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php b/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php index f3308a9a..65abcee9 100644 --- a/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php +++ b/rules/PHPUnit60/Rector/ClassMethod/AddDoesNotPerformAssertionToNonAssertingTestRector.php @@ -14,6 +14,7 @@ use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\Nop; use PhpParser\NodeVisitor; +use PHPStan\Analyser\Scope; use PHPStan\PhpDocParser\Ast\PhpDoc\GenericTagValueNode; use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagNode; use PHPStan\Reflection\ClassReflection; @@ -21,7 +22,7 @@ use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory; use Rector\Comments\NodeDocBlock\DocBlockUpdater; use Rector\Php80\NodeAnalyzer\PhpAttributeAnalyzer; -use Rector\PhpParser\Node\BetterNodeFinder; +use Rector\PHPStan\ScopeFetcher; use Rector\PHPUnit\Enum\PHPUnitAttribute; use Rector\PHPUnit\Enum\PHPUnitClassName; use Rector\PHPUnit\NodeAnalyzer\AssertCallAnalyzer; @@ -29,7 +30,6 @@ use Rector\PHPUnit\NodeAnalyzer\MockedVariableAnalyzer; use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\Rector\AbstractRector; -use Rector\Reflection\ReflectionResolver; use Rector\RuleDoc\CodeSample\CodeSample; use Rector\RuleDoc\RuleDefinition; use Rector\VersionBonding\Contract\ComposerPackageConstraintInterface; @@ -58,10 +58,8 @@ public function __construct( private readonly PhpAttributeAnalyzer $phpAttributeAnalyzer, private readonly DocBlockUpdater $docBlockUpdater, private readonly PhpDocInfoFactory $phpDocInfoFactory, - private readonly ReflectionResolver $reflectionResolver, private readonly ReflectionProvider $reflectionProvider, private readonly LifecycleMethodAssertAnalyzer $lifecycleMethodAssertAnalyzer, - private readonly BetterNodeFinder $betterNodeFinder, ) { } @@ -105,43 +103,74 @@ public function test() */ public function getNodeTypes(): array { - return [ClassMethod::class]; + return [Class_::class]; } /** - * @param ClassMethod $node + * @param Class_ $node */ public function refactor(Node $node): ?Node { - if ($this->shouldSkipClassMethod($node)) { + if (! $this->testsNodeAnalyzer->isInTestClass($node)) { return null; } - $this->removeAddToAssertionCountCalls($node); + // a child class can add asserting lifecycle methods, that count toward the inherited tests + if ($node->isAbstract()) { + return null; + } + $scope = ScopeFetcher::fetch($node); + + // the parent test case asserts in its own integration methods + if ($this->isInTwigIntegrationTestCase($scope)) { + return null; + } + + // assertions in lifecycle methods count toward every test of the class + if ($this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod($node)) { + return null; + } + + $hasChanged = false; + + foreach ($node->getMethods() as $classMethod) { + if ($this->shouldSkipClassMethod($classMethod)) { + continue; + } + + $this->removeAddToAssertionCountCalls($classMethod); + $this->addDoesNotPerformAssertions($classMethod); + + $hasChanged = true; + } + + if (! $hasChanged) { + return null; + } + + return $node; + } + + private function addDoesNotPerformAssertions(ClassMethod $classMethod): void + { // the attribute is available since PHPUnit 10, prefer it over the annotation if ($this->reflectionProvider->hasClass(PHPUnitAttribute::DOES_NOT_PERFORM_ASSERTIONS)) { - $node->attrGroups[] = new AttributeGroup([ + $classMethod->attrGroups[] = new AttributeGroup([ new Attribute(new FullyQualified(PHPUnitAttribute::DOES_NOT_PERFORM_ASSERTIONS)), ]); - return $node; + return; } - $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($node); + $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($classMethod); $phpDocInfo->addPhpDocTagNode(new PhpDocTagNode('@doesNotPerformAssertions', new GenericTagValueNode(''))); - $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); - - return $node; + $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($classMethod); } private function shouldSkipClassMethod(ClassMethod $classMethod): bool { - if (! $this->testsNodeAnalyzer->isInTestClass($classMethod)) { - return true; - } - if (! $this->testsNodeAnalyzer->isTestClassMethod($classMethod)) { return true; } @@ -150,21 +179,6 @@ private function shouldSkipClassMethod(ClassMethod $classMethod): bool return true; } - // we have no idea how the trait is used, the using class can assert on its own - if ($this->isInTrait($classMethod)) { - return true; - } - - // a child class can add asserting lifecycle methods, that count toward the inherited tests - if ($this->isInAbstractClass($classMethod)) { - return true; - } - - // the parent test case asserts in its own integration methods - if ($this->isInTwigIntegrationTestCase($classMethod)) { - return true; - } - if ($this->hasAssertingAnnotationOrAttribute($classMethod)) { return true; } @@ -174,12 +188,6 @@ private function shouldSkipClassMethod(ClassMethod $classMethod): bool return true; } - // assertions in lifecycle methods count toward every test of the class - $class = $this->resolveCurrentClass($classMethod); - if ($class instanceof Class_ && $this->lifecycleMethodAssertAnalyzer->hasAssertingLifecycleMethod($class)) { - return true; - } - return $this->mockedVariableAnalyzer->containsMockAsUsedVariable($classMethod); } @@ -226,40 +234,9 @@ private function isAddToAssertionCountExpression(Node $node): bool return $this->isName($methodCall->name, 'addToAssertionCount'); } - private function isInTrait(ClassMethod $classMethod): bool - { - $classReflection = $this->reflectionResolver->resolveClassReflection($classMethod); - if (! $classReflection instanceof ClassReflection) { - return false; - } - - return $classReflection->isTrait(); - } - - private function isInAbstractClass(ClassMethod $classMethod): bool - { - $classReflection = $this->reflectionResolver->resolveClassReflection($classMethod); - if (! $classReflection instanceof ClassReflection) { - return false; - } - - return $classReflection->isAbstract(); - } - - private function resolveCurrentClass(ClassMethod $classMethod): ?Class_ - { - $class = $this->betterNodeFinder->findFirst( - $this->getFile() - ->getNewStmts(), - fn (Node $node): bool => $node instanceof Class_ && in_array($classMethod, $node->getMethods(), true) - ); - - return $class instanceof Class_ ? $class : null; - } - - private function isInTwigIntegrationTestCase(ClassMethod $classMethod): bool + private function isInTwigIntegrationTestCase(Scope $scope): bool { - $classReflection = $this->reflectionResolver->resolveClassReflection($classMethod); + $classReflection = $scope->getClassReflection(); if (! $classReflection instanceof ClassReflection) { return false; } diff --git a/tests/Issues/PHPUnit10DataProvider/Fixture/some_test.php.inc b/tests/Issues/PHPUnit10DataProvider/Fixture/some_test.php.inc index 3441ffb7..983460d4 100644 --- a/tests/Issues/PHPUnit10DataProvider/Fixture/some_test.php.inc +++ b/tests/Issues/PHPUnit10DataProvider/Fixture/some_test.php.inc @@ -26,8 +26,8 @@ namespace Rector\PHPUnit\Tests\Issues\PHPUnit10DataProvider\Fixture; final class SomeTest extends \PHPUnit\Framework\TestCase { - #[\PHPUnit\Framework\Attributes\DataProvider('fooProvider')] #[\PHPUnit\Framework\Attributes\DoesNotPerformAssertions] + #[\PHPUnit\Framework\Attributes\DataProvider('fooProvider')] #[\PHPUnit\Framework\Attributes\Test] public function it_should_do_stuff(string $foo): void {