Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Test] Rename fixture and refactor to solve random error #3677

Merged
merged 10 commits into from
Apr 24, 2023
8 changes: 8 additions & 0 deletions packages/Testing/PHPUnit/AbstractRectorTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ protected function doTestFile(string $fixtureFilePath): void
// write temp file
FileSystem::write($inputFilePath, $inputFileContents);

if (! is_file($inputFilePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't the file exists? Where is it deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably overlapped by tearDown

protected function tearDown(): void
{
// clear temporary file
if (is_string($this->inputFilePath)) {
FileSystem::delete($this->inputFilePath);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be verified by dropping the delete.

Do tests exist which re-use the same fixture ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's temporary file with .php file instead of .php.inc so it needs to be deleted, I am thinking that internal phpunit tearDown may cause the overlap

// give enough time to write process
sleep(3);

// write temp file
FileSystem::write($inputFilePath, $inputFileContents);
}

$this->doTestFileMatchesExpectedContent($inputFilePath, $inputFileContents, $expectedFileContents, $fixtureFilePath);
}

Expand Down
12 changes: 12 additions & 0 deletions phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -806,3 +806,15 @@ parameters:
- '#Access to an undefined property Rector\\Core\\Contract\\PhpParser\\Node\\StmtsAwareInterface\:\:\$stmts#'
- '#Access to an undefined property PhpParser\\Node\\Stmt\\ClassLike\|Rector\\Core\\Contract\\PhpParser\\Node\\StmtsAwareInterface\:\:\$stmts#'

-
message: '#Method "renameVariableInFunctionLike\(\)" returns bool type, so the name should start with is/has/was#'
path: rules/Naming/VariableRenamer.php

-
message: '#Method call return value that should be used, but is not#'
paths:
- rules/Naming/ParamRenamer/ParamRenamer.php
- rules/Naming/PropertyRenamer/PropertyPromotionRenamer.php
- rules/Naming/Rector/Assign/RenameVariableToMatchMethodCallReturnTypeRector.php
- rules/Naming/Rector/ClassMethod/RenameVariableToMatchNewTypeRector.php
- rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace Rector\Tests\Naming\Rector\Class_\RenamePropertyToMatchTypeRector\Fixt

use Rector\Tests\Naming\Rector\Class_\RenamePropertyToMatchTypeRector\Source\EliteManager;

class SomeClass
class RenamePropertyAndPropertyFetch
{
/**
* @var EliteManager
Expand All @@ -25,7 +25,7 @@ namespace Rector\Tests\Naming\Rector\Class_\RenamePropertyToMatchTypeRector\Fixt

use Rector\Tests\Naming\Rector\Class_\RenamePropertyToMatchTypeRector\Source\EliteManager;

class SomeClass
class RenamePropertyAndPropertyFetch
{
/**
* @var EliteManager
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace Rector\Tests\Naming\Rector\Foreach_\RenameForeachValueVariableToMatchM

use Rector\Tests\Naming\Rector\Foreach_\RenameForeachValueVariableToMatchMethodCallReturnTypeRector\Source\DummyCollection;

final class ForeachPreferMethodNameRatherThanReturnTypeForIterables
final class ForeachPreferMethodNameOverReturnTypeForIterables
{
private DummyCollection $books;

Expand Down Expand Up @@ -34,7 +34,7 @@ namespace Rector\Tests\Naming\Rector\Foreach_\RenameForeachValueVariableToMatchM

use Rector\Tests\Naming\Rector\Foreach_\RenameForeachValueVariableToMatchMethodCallReturnTypeRector\Source\DummyCollection;

final class ForeachPreferMethodNameRatherThanReturnTypeForIterables
final class ForeachPreferMethodNameOverReturnTypeForIterables
{
private DummyCollection $books;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace Rector\Tests\Naming\ValueObjectFactory\PropertyRenameFactory\Fixture;

use Rector\Tests\Naming\ValueObjectFactory\PropertyRenameFactory\Source\EliteManager;

final class SkipSomeClass
final class RenamePropertyAndPropertyFetch2
{
/**
* @var EliteManager
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,6 @@ public function test(string $filePathWithProperty, string $expectedName, string

public static function provideData(): Iterator
{
yield [__DIR__ . '/Fixture/skip_some_class.php.inc', 'eliteManager', 'eventManager'];
yield [__DIR__ . '/Fixture/rename_property_and_property_fetch2.php.inc', 'eliteManager', 'eventManager'];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,18 @@ public function refactor(Node $node): ?Node
return null;
}

$this->renameVariable($variableAndCallForeach, $expectedName);
$hasRenamed = $this->variableRenamer->renameVariableInFunctionLike(
$variableAndCallForeach->getFunctionLike(),
$variableAndCallForeach->getVariableName(),
$expectedName,
null
);

if ($hasRenamed) {
return $node;
}

return $node;
return null;
}

private function shouldSkip(VariableAndCallForeach $variableAndCallForeach, string $expectedName): bool
Expand All @@ -120,14 +129,4 @@ private function shouldSkip(VariableAndCallForeach $variableAndCallForeach, stri
$variableAndCallForeach->getVariable()
);
}

private function renameVariable(VariableAndCallForeach $variableAndCallForeach, string $expectedName): void
{
$this->variableRenamer->renameVariableInFunctionLike(
$variableAndCallForeach->getFunctionLike(),
$variableAndCallForeach->getVariableName(),
$expectedName,
null
);
}
}
15 changes: 12 additions & 3 deletions rules/Naming/VariableRenamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,18 @@ public function renameVariableInFunctionLike(
string $oldName,
string $expectedName,
?Assign $assign = null
): void {
): bool {
$isRenamingActive = false;

if (! $assign instanceof Assign) {
$isRenamingActive = true;
}

$hasRenamed = false;

$this->simpleCallableNodeTraverser->traverseNodesWithCallable(
(array) $functionLike->getStmts(),
function (Node $node) use ($oldName, $expectedName, $assign, &$isRenamingActive): ?Variable {
function (Node $node) use ($oldName, $expectedName, $assign, &$isRenamingActive, &$hasRenamed): ?Variable {
if ($assign instanceof Assign && $node === $assign) {
$isRenamingActive = true;
return null;
Expand All @@ -71,9 +73,16 @@ function (Node $node) use ($oldName, $expectedName, $assign, &$isRenamingActive)
return null;
}

return $this->renameVariableIfMatchesName($node, $oldName, $expectedName);
$variable = $this->renameVariableIfMatchesName($node, $oldName, $expectedName);
if ($variable instanceof Variable) {
$hasRenamed = true;
}

return $variable;
}
);

return $hasRenamed;
}

private function isParamInParentFunction(Variable $variable): bool
Expand Down
2 changes: 0 additions & 2 deletions src/FileSystem/PhpFilesFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
namespace Rector\Core\FileSystem;

use Rector\Caching\UnchangedFilesFilter;
use Rector\Core\Util\StringUtils;
use Rector\Core\ValueObject\StaticNonPhpFileSuffixes;

final class PhpFilesFinder
{
Expand Down
6 changes: 5 additions & 1 deletion src/NodeManipulator/ClassMethodAssignManipulator.php
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,11 @@ private function collectReferenceVariableNames(ClassMethod $classMethod): array

foreach ($variables as $variable) {
$variableName = $this->nodeNameResolver->getName($variable);
if ($variableName === 'this' || $variableName === null) {
if ($variableName === 'this') {
continue;
}

if ($variableName === null) {
continue;
}

Expand Down
1 change: 0 additions & 1 deletion src/NonPhpFile/NonPhpFileProcessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Rector\Core\ValueObject\Configuration;
use Rector\Core\ValueObject\Error\SystemError;
use Rector\Core\ValueObject\Reporting\FileDiff;
use Rector\Core\ValueObject\StaticNonPhpFileSuffixes;
use Rector\Parallel\ValueObject\Bridge;
use Symfony\Component\Filesystem\Filesystem;

Expand Down