AssertCompareToSpecificMethodRector shouldn't flip Assert* parameters

[VincentLanglet]

Bug Report

Subject	Details
Rector version	last dev-main
Installed as	composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/1565705e-66cb-46eb-a365-4b25236fa3c5

<?php

use PHPUnit\Framework\TestCase;

final class MyTest extends TestCase
{
    public function testSomeFoo()
    {   
        $ids = ['foo', 'bar'];
        $result = myMethod($ids);
        
        self::assertSame(count($ids), $result);
    }
}

Responsible rules

• AssertCompareToSpecificMethodRector

Expected Behavior

In this example self::assertSame(count($ids), $result); is changed to self::assertCount($result, $ids);,
but this give an inverted error message when the assertion is wrong.

It basically do the opposite of what FlipAssertRector users wants.

So I think such replacement shouldn't be done, or split in another rule to allow using "safe" replacement like

self::assertSame($expected, count($result));

to

self::assertCount($expected, $result);

which respect the order of the messages.

[VincentLanglet]

I just discovered there is AssertCompareOnCountableWithMethodToAssertCountRector which is very similar and does what I wanted https://getrector.com/rule-detail/assert-compare-on-countable-with-method-to-assert-count-rector

I feel like AssertCompareToSpecificMethodRector is really misleading

• The name doesn't explain it only change assert* to assertCount or assertNotCount (we may think it also change some other non-specific method call like assertSame(true, $foo))
• The doc exemple doesn't show it can remove the count call for the second argument OR the first argument.

On the opposite side AssertCompareOnCountableWithMethodToAssertCountRector example doesn't show it works for both
$foo->count() and count($foo) calls.

Maybe some improvements could be done like

• For AssertCompareOnCountableWithMethodToAssertCountRector

• A better example in the docs

• And for AssertCompareToSpecificMethodRector, some choices between

  • A better example in the docs
  • Renaming AssertCompareToSpecificMethodRector
  • Removing AssertCompareToSpecificMethodRector from the PHPUnit QualitySet
  • Deprecating AssertCompareToSpecificMethodRector

?

[samsonasik]

The AssertCompareOnCountableWithMethodToAssertCountRector seems not change it

https://getrector.com/demo/70985be7-0112-41d7-973a-8d308ce67e4c

I think the patch should be in AssertCompareToSpecificMethodRector since it verify count() func call.

[samsonasik]

Ok, the AssertCompareToSpecificMethodRector is changing behaviour of expected vs actual value, which imo should not, and the result cause invalid for count() usage on first arg.

I think deprecate AssertCompareToSpecificMethodRector is better, so we can focus on AssertCompareOnCountableWithMethodToAssertCountRector which verify Countable type.

[VincentLanglet]

I think deprecate AssertCompareToSpecificMethodRector is better, so we can focus on AssertCompareOnCountableWithMethodToAssertCountRector which verify Countable type.

I agree, since a fixed AssertCompareToSpecificMethodRector version would be totally included by AssertCompareOnCountableWithMethodToAssertCountRector

[samsonasik]

Let see how @TomasVotruba opinion about deprecating AssertCompareToSpecificMethodRector

[TomasVotruba]

Lets update docs first, deprecating rule is breaking change suitable for harming behavior only.

@VincentLanglet Could you send PR to fix the docs?

[VincentLanglet]

Lets update docs first, deprecating rule is breaking change suitable for harming behavior only.

@VincentLanglet Could you send PR to fix the docs?

If the rule is not deprecated, wouldn't it be at least a good move to remove it from the PHPUnit CodeQuality set ?
https://github.com/rectorphp/rector-phpunit/blob/9fe16ab87d2bb22c762b98ed1aaf27ac64fd3ede/config/sets/phpunit-code-quality.php#L62 because it's definitely in conflitct with the idea of the FlipAssertRector rule.

[TomasVotruba]

I see. No need to remove it from code quality set, it should be part of it.

self::assertCount($expected, $result);

This behavior I'd expect. Could you update the rule to print this result?

[VincentLanglet]

Could you update the rule to print this result?

This is the one printed:
https://getrector.com/rule-detail/assert-compare-to-specific-method-rector

self::assertCount($expected, $result);

This behavior I'd expect.

I agree, this one is expected.

But my issue was about
self::assertSame(count($expected), $result);
which is fixed to self::assertCount($result, $expected).

Do you expect this behavior too @TomasVotruba ?

• If yes, then the only thing to do is update the doc.
• If no, then behavior should be changed and/or rule should be remove from quality set.

[TomasVotruba]

behavior should be changed

👍

[VincentLanglet]

behavior should be changed

👍

So to remove such behavior, we just need to remove this code https://github.com/rectorphp/rector-phpunit/blob/9fe16ab87d2bb22c762b98ed1aaf27ac64fd3ede/rules/CodeQuality/Rector/MethodCall/AssertCompareToSpecificMethodRector.php#L106

And then, if the behavior is changed:

AssertCompareOnCountableWithMethodToAssertCountRector

• Change self::assertSame($expected, count($result)); to self::assertCount($expected, $result); (cf https://github.com/rectorphp/rector-phpunit/blob/9fe16ab87d2bb22c762b98ed1aaf27ac64fd3ede/rules/CodeQuality/Rector/MethodCall/AssertCompareOnCountableWithMethodToAssertCountRector.php#L75)
• Change self::assertSame($expected, $result->count()); to self::assertCount($expected, $result); (cf https://github.com/rectorphp/rector-phpunit/blob/9fe16ab87d2bb22c762b98ed1aaf27ac64fd3ede/rules/CodeQuality/Rector/MethodCall/AssertCompareOnCountableWithMethodToAssertCountRector.php#L85)

AssertCompareToSpecificMethodRector will

• Change self::assertSame($expected, count($result)); to self::assertCount($expected, $result); (cf https://github.com/rectorphp/rector-phpunit/blob/9fe16ab87d2bb22c762b98ed1aaf27ac64fd3ede/rules/CodeQuality/Rector/MethodCall/AssertCompareToSpecificMethodRector.php#L102)

So I thought, AssertCompareToSpecificMethodRector would be fully included in AssertCompareOnCountableWithMethodToAssertCountRector, that's why we thought this rule will be useless and could be deprecated. But this is wrong

When looking more at AssertCompareToSpecificMethodRector, it also does

• Change self::assertSame($expected, sizeof($result)); to self::assertCount($expected, $result);
• Change self::assertSame($expected, iterator_count($result)); to self::assertCount($expected, $result);
• Change self::assertSame($expected, get_class($result)); to self::assertInstanceOf($expected, $result);

The AssertCompareToSpecificMethodRector had the default of having a too generic name. We could think that rules like

• https://getrector.com/rule-detail/assert-false-strpos-to-contains-rector
• https://getrector.com/rule-detail/assert-property-exists-rector
• https://getrector.com/rule-detail/assert-reg-exp-rector
• https://getrector.com/rule-detail/assert-same-bool-null-to-specific-method-rector

would have been included in the "generic" rule AssertCompareToSpecificMethodRector. Also it's unclear the difference between AssertComparisonToSpecificMethodRector (https://getrector.com/rule-detail/assert-comparison-to-specific-method-rector) and AssertCompareToSpecificMethodRector just by looking at the name of such rule.

I would have done:

• Moving sizeof, iterator_count checks to AssertCompareOnCountableWithMethodToAssertCountRector
• Moving get_class check to another rule
• Deprecate AssertCompareToSpecificMethodRector

But it's not my choice.

So far I tried a PR to remove the unexpected/unwanted behavior rectorphp/rector-phpunit#423

[samsonasik]

@VincentLanglet do you think this issue is close-able now?

[VincentLanglet]

@VincentLanglet do you think this issue is close-able now?

Unless you plan to do something like

I would have done:

• Moving sizeof, iterator_count checks to AssertCompareOnCountableWithMethodToAssertCountRector
• Moving get_class check to another rule
• Deprecate AssertCompareToSpecificMethodRector

Yes it's closabled

[TomasVotruba]

I think first 2 can be done. Feel free to open the PRs.