-
Notifications
You must be signed in to change notification settings - Fork 63
[code-quality] Add MatchAssertSameExpectedTypeRector #510
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class MatchAssertType extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame('123', $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class MatchAssertType extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame(123, $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
|
|
||
| ?> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class MatchIntegerToString extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame(123, $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): string | ||
| { | ||
| return '123'; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class MatchIntegerToString extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame('123', $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): string | ||
| { | ||
| return '123'; | ||
| } | ||
| } | ||
|
|
||
| ?> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class NullableMatchAssertType extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame('123', $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): ?int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\Fixture; | ||
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class NullableMatchAssertType extends TestCase | ||
| { | ||
| public function test() | ||
| { | ||
| $this->assertSame(123, $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): ?int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
|
|
||
| ?> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector; | ||
|
|
||
| use Iterator; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use Rector\Testing\PHPUnit\AbstractRectorTestCase; | ||
|
|
||
| final class MatchAssertSameExpectedTypeRectorTest extends AbstractRectorTestCase | ||
| { | ||
| #[DataProvider('provideData')] | ||
| public function test(string $filePath): void | ||
| { | ||
| $this->doTestFile($filePath); | ||
| } | ||
|
|
||
| public static function provideData(): Iterator | ||
| { | ||
| return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); | ||
| } | ||
|
|
||
| public function provideConfigFilePath(): string | ||
| { | ||
| return __DIR__ . '/config/configured_rule.php'; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| use Rector\Config\RectorConfig; | ||
| use Rector\PHPUnit\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector; | ||
|
|
||
| return static function (RectorConfig $rectorConfig): void { | ||
| $rectorConfig->rule(MatchAssertSameExpectedTypeRector::class); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\PHPUnit\CodeQuality\Rector\MethodCall; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PhpParser\Node\Expr\StaticCall; | ||
| use PhpParser\Node\Scalar\LNumber; | ||
| use PhpParser\Node\Scalar\String_; | ||
| use PHPStan\Type\TypeCombinator; | ||
| use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; | ||
| use Rector\Rector\AbstractRector; | ||
| use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; | ||
| use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; | ||
|
|
||
| /** | ||
| * @see \Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\MatchAssertSameExpectedTypeRector\MatchAssertSameExpectedTypeRectorTest | ||
| */ | ||
| final class MatchAssertSameExpectedTypeRector extends AbstractRector | ||
| { | ||
| public function __construct( | ||
| private readonly TestsNodeAnalyzer $testsNodeAnalyzer | ||
| ) { | ||
| } | ||
|
|
||
| public function getRuleDefinition(): RuleDefinition | ||
| { | ||
| return new RuleDefinition( | ||
| 'Correct expected type in assertSame() method to match strict type of passed variable', | ||
| [ | ||
| new CodeSample( | ||
| <<<'CODE_SAMPLE' | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class SomeTest extends TestCase | ||
| { | ||
| public function run() | ||
| { | ||
| $this->assertSame('123', $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
| CODE_SAMPLE | ||
| , | ||
| <<<'CODE_SAMPLE' | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class SomeTest extends TestCase | ||
| { | ||
| public function run() | ||
| { | ||
| $this->assertSame(123, $this->getOrderId()); | ||
| } | ||
|
|
||
| private function getOrderId(): int | ||
| { | ||
| return 123; | ||
| } | ||
| } | ||
| CODE_SAMPLE | ||
| ), | ||
| ] | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @return array<class-string<Node>> | ||
| */ | ||
| public function getNodeTypes(): array | ||
| { | ||
| return [MethodCall::class, StaticCall::class]; | ||
| } | ||
|
|
||
| /** | ||
| * @param MethodCall|StaticCall $node | ||
| */ | ||
| public function refactor(Node $node): ?Node | ||
| { | ||
| if (! $this->testsNodeAnalyzer->isPHPUnitMethodCallNames($node, ['assertSame', 'assertEquals'])) { | ||
| return null; | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should add check:
I will apply patch for it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| $expectedArg = $node->getArgs()[0]; | ||
| if (! $expectedArg->value instanceof String_ && ! $expectedArg->value instanceof LNumber) { | ||
| return null; | ||
| } | ||
|
|
||
| $expectedType = $this->getType($expectedArg->value); | ||
|
|
||
| $variableExpr = $node->getArgs()[1] | ||
| ->value; | ||
| $variableType = $this->getType($variableExpr); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be public function run()
{
$this->assertSame('123', $this->getOrderId());
}
/**
* @return int
*/
private function getOrderId()
{
return '123';
}I will apply patch for it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| $directVariableType = TypeCombinator::removeNull($variableType); | ||
|
|
||
| if ($expectedType->isLiteralString()->yes() && $directVariableType->isInteger()->yes()) { | ||
| // update expected type to provided type | ||
| $expectedArg->value = new LNumber((int) $expectedArg->value->value); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| return $node; | ||
| } | ||
|
|
||
| if ($expectedType->isInteger()->yes() && $directVariableType->isString()->yes()) { | ||
| // update expected type to provided type | ||
| $expectedArg->value = new String_((string) $expectedArg->value->value); | ||
|
|
||
| return $node; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba on
assertSame, it actually already error on the first usage before its transformation, that means the test already invalid on very beginning:so it probably should only apply on
assertEquals?https://github.com/rectorphp/rector-phpunit/pull/510/files#diff-1d628c9179459ab3a6e79445318e1c19281e70418d547ad433a05f35d83cf8be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba I tested in our project, it cause error:
that compare:
this should indeed only on
assertEquals,assertSameis apply on actual same type, I will create a patch to only apply onassertEqualsand rename the rule to:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#514
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add test fixture here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on process: