From 818f82bfc51607005fcfefc537587c7ba4993904 Mon Sep 17 00:00:00 2001 From: yoannblot Date: Thu, 24 Sep 2026 08:13:21 +0200 Subject: [PATCH] [CodeQuality] Extend FlipAssertRector to more assert methods and expected value shapes --- .../Fixture/array_literal.php.inc | 27 ++++ .../array_literal_with_variable.php.inc | 27 ++++ .../Fixture/canonicalizing_and_delta.php.inc | 37 +++++ .../Fixture/concat_literal.php.inc | 35 +++++ .../Fixture/enum_value.php.inc | 29 ++++ .../Fixture/negative_number.php.inc | 35 +++++ .../Fixture/new_with_literal_args.php.inc | 29 ++++ .../Fixture/skip_actual_array_literal.php.inc | 14 ++ .../Fixture/skip_both_array_literal.php.inc | 13 ++ ...puted_expected_against_built_array.php.inc | 19 +++ .../Fixture/skip_concat_with_variable.php.inc | 13 ++ .../skip_correct_canonicalizing.php.inc | 14 ++ .../Fixture/skip_enum_value_first.php.inc | 14 ++ .../Fixture/skip_named_args.php.inc | 13 ++ .../skip_new_with_variable_arg.php.inc | 21 +++ .../Rector/MethodCall/FlipAssertRector.php | 129 +++++++++++++++++- 16 files changed, 462 insertions(+), 7 deletions(-) create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal_with_variable.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/canonicalizing_and_delta.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/concat_literal.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/enum_value.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/negative_number.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/new_with_literal_args.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_actual_array_literal.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_both_array_literal.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_computed_expected_against_built_array.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_concat_with_variable.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_correct_canonicalizing.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_enum_value_first.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_named_args.php.inc create mode 100644 rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_new_with_variable_arg.php.inc diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal.php.inc new file mode 100644 index 00000000..29df7562 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal.php.inc @@ -0,0 +1,27 @@ +assertSame($response->json(), ['id' => 1, 'status' => SomeEnum::Active]); + } +} + +?> +----- +assertSame(['id' => 1, 'status' => SomeEnum::Active], $response->json()); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal_with_variable.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal_with_variable.php.inc new file mode 100644 index 00000000..b1013928 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/array_literal_with_variable.php.inc @@ -0,0 +1,27 @@ +assertEquals($model->value, ['transformations' => $transformations]); + } +} + +?> +----- +assertEquals(['transformations' => $transformations], $model->value); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/canonicalizing_and_delta.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/canonicalizing_and_delta.php.inc new file mode 100644 index 00000000..cf01fb2d --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/canonicalizing_and_delta.php.inc @@ -0,0 +1,37 @@ +assertEqualsCanonicalizing($result, [1, 2]); + $this->assertNotEqualsCanonicalizing($result, [3, 4]); + $this->assertEqualsIgnoringCase($result, 'expected'); + $this->assertNotEqualsIgnoringCase($result, 'other'); + $this->assertEqualsWithDelta($result, 1.5, 0.01); + $this->assertNotEqualsWithDelta($result, 2.5, 0.01); + } +} + +?> +----- +assertEqualsCanonicalizing([1, 2], $result); + $this->assertNotEqualsCanonicalizing([3, 4], $result); + $this->assertEqualsIgnoringCase('expected', $result); + $this->assertNotEqualsIgnoringCase('other', $result); + $this->assertEqualsWithDelta(1.5, $result, 0.01); + $this->assertNotEqualsWithDelta(2.5, $result, 0.01); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/concat_literal.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/concat_literal.php.inc new file mode 100644 index 00000000..b5ab16d2 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/concat_literal.php.inc @@ -0,0 +1,35 @@ +assertSame($result->getName(), 'foo' . 'bar'); + $this->assertSame($result->getPath(), __DIR__ . '/fixture.json'); + $this->assertSame($result->getId(), self::PREFIX . '-1'); + } +} + +?> +----- +assertSame('foo' . 'bar', $result->getName()); + $this->assertSame(__DIR__ . '/fixture.json', $result->getPath()); + $this->assertSame(self::PREFIX . '-1', $result->getId()); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/enum_value.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/enum_value.php.inc new file mode 100644 index 00000000..5a148140 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/enum_value.php.inc @@ -0,0 +1,29 @@ +assertSame($result->status, Status::Active->value); + $this->assertSame($result->getStatusName(), Status::Active->name); + } +} + +?> +----- +assertSame(Status::Active->value, $result->status); + $this->assertSame(Status::Active->name, $result->getStatusName()); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/negative_number.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/negative_number.php.inc new file mode 100644 index 00000000..1d7ba6da --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/negative_number.php.inc @@ -0,0 +1,35 @@ +assertSame($result->getOffset(), -1); + $this->assertSame($result->getMin(), -PHP_INT_MAX); + $this->assertSame($result->getLimit(), -self::LIMIT); + } +} + +?> +----- +assertSame(-1, $result->getOffset()); + $this->assertSame(-PHP_INT_MAX, $result->getMin()); + $this->assertSame(-self::LIMIT, $result->getLimit()); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/new_with_literal_args.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/new_with_literal_args.php.inc new file mode 100644 index 00000000..de9fa971 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/new_with_literal_args.php.inc @@ -0,0 +1,29 @@ +assertEquals($result, new Money(100, Currency::EUR)); + $this->assertEquals($result->getPeriod(), new Period(['from' => 1, 'to' => -1])); + } +} + +?> +----- +assertEquals(new Money(100, Currency::EUR), $result); + $this->assertEquals(new Period(['from' => 1, 'to' => -1]), $result->getPeriod()); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_actual_array_literal.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_actual_array_literal.php.inc new file mode 100644 index 00000000..c3bb06a2 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_actual_array_literal.php.inc @@ -0,0 +1,14 @@ +assertSame($result, [$model->x, $model->y]); + $this->assertSame($result, [$model->getX()]); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_both_array_literal.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_both_array_literal.php.inc new file mode 100644 index 00000000..789e4cc6 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_both_array_literal.php.inc @@ -0,0 +1,13 @@ +assertSame(['id' => 1], [1, 2]); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_computed_expected_against_built_array.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_computed_expected_against_built_array.php.inc new file mode 100644 index 00000000..9a30cc28 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_computed_expected_against_built_array.php.inc @@ -0,0 +1,19 @@ +assertSame($this->expectedIds, [$first->id, $second->id]); + $this->assertSame($expected['ids'], [$user->id, $team->id]); + $this->assertSame(static::$fixtures, [$result->name]); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_concat_with_variable.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_concat_with_variable.php.inc new file mode 100644 index 00000000..2a11490b --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_concat_with_variable.php.inc @@ -0,0 +1,13 @@ +assertSame($result->getId(), 'id-' . $id); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_correct_canonicalizing.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_correct_canonicalizing.php.inc new file mode 100644 index 00000000..45b54841 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_correct_canonicalizing.php.inc @@ -0,0 +1,14 @@ +assertEqualsCanonicalizing([1, 2], $result); + $this->assertEqualsWithDelta(1.5, $result, 0.01); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_enum_value_first.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_enum_value_first.php.inc new file mode 100644 index 00000000..fb5581ae --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_enum_value_first.php.inc @@ -0,0 +1,14 @@ +assertSame(Status::Active->value, [$x]); + $this->assertSame($result->status, $status->value); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_named_args.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_named_args.php.inc new file mode 100644 index 00000000..a51819af --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_named_args.php.inc @@ -0,0 +1,13 @@ +assertSame(expected: $result->ids(), actual: [1, 2]); + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_new_with_variable_arg.php.inc b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_new_with_variable_arg.php.inc new file mode 100644 index 00000000..106acdde --- /dev/null +++ b/rules-tests/CodeQuality/Rector/MethodCall/FlipAssertRector/Fixture/skip_new_with_variable_arg.php.inc @@ -0,0 +1,21 @@ +assertEquals($result, new Money($amount)); + $this->assertEquals($result, new $className(100)); + $this->assertEquals($result, new Money(...$args)); + $this->assertEquals($result, new Money(amount: 100)); + $this->assertEquals($result, new class(100) { + public function __construct(public int $amount) + { + } + }); + } +} + +?> diff --git a/rules/CodeQuality/Rector/MethodCall/FlipAssertRector.php b/rules/CodeQuality/Rector/MethodCall/FlipAssertRector.php index 56ae1915..330258cd 100644 --- a/rules/CodeQuality/Rector/MethodCall/FlipAssertRector.php +++ b/rules/CodeQuality/Rector/MethodCall/FlipAssertRector.php @@ -5,11 +5,26 @@ namespace Rector\PHPUnit\CodeQuality\Rector\MethodCall; use PhpParser\Node; +use PhpParser\Node\Arg; +use PhpParser\Node\ArrayItem; use PhpParser\Node\Expr; +use PhpParser\Node\Expr\Array_; +use PhpParser\Node\Expr\ArrayDimFetch; +use PhpParser\Node\Expr\BinaryOp\Concat; use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\ConstFetch; +use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\New_; +use PhpParser\Node\Expr\NullsafeMethodCall; +use PhpParser\Node\Expr\NullsafePropertyFetch; +use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Expr\StaticPropertyFetch; +use PhpParser\Node\Expr\UnaryMinus; +use PhpParser\Node\Expr\Variable; +use PhpParser\Node\Identifier; +use PhpParser\Node\Name; use PhpParser\Node\Scalar; use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\Rector\AbstractRector; @@ -26,6 +41,22 @@ final class FlipAssertRector extends AbstractRector */ private const array METHOD_NAMES = [ 'assertSame', 'assertNotSame', 'assertNotEquals', 'assertEquals', 'assertStringContainsString', + 'assertEqualsCanonicalizing', 'assertNotEqualsCanonicalizing', 'assertEqualsIgnoringCase', + 'assertNotEqualsIgnoringCase', 'assertEqualsWithDelta', 'assertNotEqualsWithDelta', + ]; + + /** + * @var array> + */ + private const array COMPUTED_EXPR_CLASSES = [ + MethodCall::class, + StaticCall::class, + FuncCall::class, + PropertyFetch::class, + StaticPropertyFetch::class, + NullsafeMethodCall::class, + NullsafePropertyFetch::class, + ArrayDimFetch::class, ]; public function __construct( @@ -97,12 +128,12 @@ public function refactor(Node $node): ?Node $firstArg = $node->getArgs()[0]; $secondArg = $node->getArgs()[1]; - // correct location - if ($this->isScalarValue($firstArg->value)) { + // named args keep their meaning regardless of position + if ($firstArg->name instanceof Identifier || $secondArg->name instanceof Identifier) { return null; } - if (! $this->isScalarValue($secondArg->value)) { + if (! $this->isFlipNeeded($firstArg->value, $secondArg->value)) { return null; } @@ -114,16 +145,100 @@ public function refactor(Node $node): ?Node return $node; } - private function isScalarValue(Expr $expr): bool + private function isFlipNeeded(Expr $firstExpr, Expr $secondExpr): bool { - if ($expr instanceof Scalar) { + // correct location + if ($this->isExpectedValue($firstExpr)) { + return false; + } + + if ($this->isExpectedValue($secondExpr)) { return true; } - if ($expr instanceof ConstFetch) { + // e.g. assertSame($obj->getValues(), [$a, $b]) + return $secondExpr instanceof Array_ + && $this->isComputedExpr($firstExpr) + && $this->isLiteralArray($secondExpr, allowVariables: true); + } + + /** + * @param bool $allowVariables accept variables as array keys and values, not inside concat or new + */ + private function isExpectedValue(Expr $expr, bool $allowVariables = false): bool + { + if ($this->isScalarValue($expr) || $this->isEnumCaseValue($expr)) { + return true; + } + + if ($allowVariables && $expr instanceof Variable) { return true; } - return $expr instanceof ClassConstFetch; + if ($expr instanceof New_) { + return $this->isNewWithLiteralArgs($expr); + } + + if ($expr instanceof Concat) { + return $this->isExpectedValue($expr->left) && $this->isExpectedValue($expr->right); + } + + return $expr instanceof Array_ && $this->isLiteralArray($expr, $allowVariables); + } + + private function isNewWithLiteralArgs(New_ $new): bool + { + if (! $new->class instanceof Name) { + return false; + } + + return array_all( + $new->getArgs(), + fn (Arg $arg): bool => ! $arg->unpack + && ! $arg->name instanceof Identifier + && $this->isExpectedValue($arg->value) + ); + } + + private function isLiteralArray(Array_ $array, bool $allowVariables = false): bool + { + return array_all( + $array->items, + fn (ArrayItem $arrayItem): bool => ! $arrayItem->unpack + && (! $arrayItem->key instanceof Expr || $this->isExpectedValue($arrayItem->key, $allowVariables)) + && $this->isExpectedValue($arrayItem->value, $allowVariables) + ); + } + + private function isComputedExpr(Expr $expr): bool + { + return array_any( + self::COMPUTED_EXPR_CLASSES, + fn (string $computedExprClass): bool => $expr instanceof $computedExprClass + ); + } + + /** + * e.g. Status::Active->value, Status::Active->name + */ + private function isEnumCaseValue(Expr $expr): bool + { + if (! $expr instanceof PropertyFetch || ! $this->isNames($expr->name, ['value', 'name'])) { + return false; + } + + return $expr->var instanceof ClassConstFetch && ! $this->isName($expr->var->name, 'class'); + } + + /** + * Scalar or constant, optionally negated, e.g. 'value', -1, PHP_INT_MAX, -self::LIMIT + */ + private function isScalarValue(Expr $expr): bool + { + if ($expr instanceof UnaryMinus) { + return $this->isScalarValue($expr->expr); + } + + return $expr instanceof Scalar || $expr instanceof ConstFetch || $expr instanceof ClassConstFetch; } }