rectorphp / rector-phpunit

Rector upgrade rules for PHPUnit
https://getrector.com/find-rule?activeRectorSetGroup=phpunit
MIT License
64 stars 47 forks source link

WithConsecutiveRector is not working as expected #327

Closed nicolashohm closed 1 week ago

nicolashohm commented 5 months ago

I saw WithConsecutiveRector added in #246 to the PHPUnit 10 rule set, and was keen to use it. But unfortunately, it's not working.

I took the example from tests/Rector/StmtsAwareInterface/WithConsecutiveRector/Fixture/some_class.php.inc and defined PersonService like this:

class PersonService {
    public function prepare($value1, $value2) {}
}

Original test:

        $mock = $this->createMock(PersonService::class);
        $mock->expects($this->exactly(2))
            ->method('prepare')
            ->withConsecutive(
                [1, 2],
                [3, 4],
            );

        $mock->prepare(9, 9);
        $mock->prepare(9, 9);

=> Test fails as expected with:

Parameter 0 for invocation #0 PersonService::prepare(9, 9) does not match expected value.
Failed asserting that 9 matches expected 1.

after running the WithConsecutiveRector:

        $mock = $this->createMock(PersonService::class);
        $matcher = $this->exactly(2);
        $mock->expects($matcher)
            ->method('prepare')->willReturnCallback(static fn(): array => match ($matcher->numberOfInvocations()) {
            1 => [1, 2],
            2 => [3, 4],
        });

        $mock->prepare(9, 9);
        $mock->prepare(9, 9);

=> Test is successful even though it shouldn't cause 9 is not equals to the expected value.

the result I get from the rector is already different from what's in the documentation. So I also checked if it would work with the result the documentation provides:

        $mock = $this->createMock(PersonService::class);
        $matcher = $this->exactly(2);
        $mock->expects($matcher)
            ->method('prepare')->willReturnCallback(function () use ($matcher) {
                return match ($matcher->numberOfInvocations()) {
                    1 => [1, 2],
                    2 => [3, 4],
                };
            });

        $mock->prepare(9, 9);
        $mock->prepare(9, 9);

=> Test is successful again, even though it still shouldn't.

Let me know if I missed something. Otherwise, I've unfortunately no idea how to fix this rector.

samsonasik commented 5 months ago

@f-albert reimplement at https://github.com/rectorphp/rector-phpunit/pull/313, could you check?

nicolashohm commented 5 months ago

Cool I missed that. Now it looks way better. Running rector now, results in this:

        $mock = $this->createMock(PersonService::class);
        $matcher = $this->exactly(2);
        $mock->expects($matcher)
            ->method('prepare')->willReturnCallback(function ($parameters) use ($matcher) : void {
            match ($matcher->getInvocationCount()) {
                1 => $this->assertEquals([1, 2], $parameters),
                2 => $this->assertEquals([3, 4], $parameters),
            };
        });

        $mock->prepare(9, 9);
        $mock->prepare(9, 9);

Which has only one little issue which is easy to fix:

-    ->method('prepare')->willReturnCallback(function ($parameters) use ($matcher) : void {
+    ->method('prepare')->willReturnCallback(function (...$parameters) use ($matcher) : void {
samsonasik commented 5 months ago

/cc @f-albert could you check if @nicolashohm suggestion above is correct and can you try apply it? Thank you.

TomasVotruba commented 1 week ago

Closing as outdated and lack of feedback last 4 months. Thanks for understanding :+1:

This rule is quite a challange :) if you come across a weird case, please submit a test fixture here:

https://github.com/rectorphp/rector-phpunit/tree/bffd16339732095ba84bf21475eac749c1c9a154/rules-tests/PHPUnit100/Rector/StmtsAwareInterface/WithConsecutiveRector/Fixture