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

WithConsecutiveRector is not working as expected #327

Closed
nicolashohm opened this issue Apr 25, 2024 · 4 comments
Closed

WithConsecutiveRector is not working as expected #327

nicolashohm opened this issue Apr 25, 2024 · 4 comments

Comments

@nicolashohm
Copy link

nicolashohm commented Apr 25, 2024

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
Copy link
Member

@f-albert reimplement at #313, could you check?

@nicolashohm
Copy link
Author

nicolashohm commented Apr 26, 2024

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
Copy link
Member

samsonasik commented Apr 26, 2024

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

@TomasVotruba
Copy link
Member

Closing as outdated and lack of feedback last 4 months. Thanks for understanding 👍

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants