Rector: [PHPUnit] Migrate stubs to `createStub`

[greg0ire]

Stubs are mocks with zero expectations.

Diff

The following diff should be applied _iff_ no expectations are performed later on on $service.

- $service = $this->createMock(Service::class);
+ $service = $this->createStub(Service::class);

So it should be applied iff expects() is never called.
expects() appears in the interface of MockObject: https://github.com/sebastianbergmann/phpunit/blob/5e523bdc7dd4d90fed9fb29d1df05347b3e7eaba/src/Framework/MockObject/MockObject.php#L24 and not in the interface of Stub.

$this->createMock(Service::class)->method('something')->willReturn('something else');
// should be left untouched

But after reading this line I think it can be applied if only method() is called (it's not an expectation)

- $this->createMock(Service::class)->method('something');
+ $this->createStub(Service::class)->method('something');

Implemented in https://github.com/sebastianbergmann/phpunit/pull/3810 , since 8.4.0

[OskarStark]

cc @localheinz

[TomasVotruba]

Just add RenameMethodCallRector here: https://github.com/rectorphp/rector/blob/master/config/set/phpunit/phpunit90.yaml

[greg0ire]

This would be a bit naive I think, since it will only work if no calls to expectations methods (expects, method, willReturn, etc.) are made afterwards.

[TomasVotruba]

That's not clear at all from the description. Could you update it?

[greg0ire]

I added more details. Is it clear enough now?

[TomasVotruba]

I think so.

Could you send PR?

[greg0ire]

I'm not sure, depends on how straightforward this is.

[TomasVotruba]

Go for it. Any improvement counts

[greg0ire]

In my mind, I think that would involve finding occurrences of createMock, then use static analysis to determine if any calls to expects() is done on the mock, no matter how many methods calls using the fluent interface there are. Do you know rules that do something similar and I could use for inspiration?

[TomasVotruba]

This rule collects all the chain calls on MethodCall: https://github.com/rectorphp/rector/blob/525ed1e2cbc41a1e49493a7e43e8bb686a59cf48/src/Rector/MethodBody/FluentReplaceRector.php#L111-L125

[greg0ire]

Looking into it :)

[OskarStark]

Thank you @greg0ire 😍

[greg0ire]

I think I'm a bit stuck, would love some help. I successfully obtained the name of the variable to which the result of createMock is assigned. When I try to get all the method calls that are made on this variable from MethodCallManipulator::findMethodCallNamesOnVariable, I always get an empty array. Here is what I tried:

<?php

declare(strict_types=1);

namespace Rector\PHPUnit\Rector\MethodCall;

use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\MethodCall;
use Rector\Core\PhpParser\Node\Manipulator\AssignManipulator;
use Rector\Core\PhpParser\Node\Manipulator\MethodCallManipulator;
use Rector\Core\Rector\AbstractRector;
use Rector\Core\RectorDefinition\CodeSample;
use Rector\Core\RectorDefinition\RectorDefinition;
use Rector\NodeTypeResolver\Node\AttributeKey;

/**
 * @see https://github.com/sebastianbergmann/phpunit/issues/3120
 *
 * @see \Rector\PHPUnit\Tests\Rector\MethodCall\CreateMockToCreateStubRector\CreateMockToCreateStubRectorTest
 */
final class CreateMockToCreateStubRector extends AbstractRector
{
    /**
     * @var MethodCallManipulator
     */
    private $methodCallManipulator;

    public function __construct(MethodCallManipulator $methodCallManipulator)
    {
        $this->methodCallManipulator = $methodCallManipulator;
    }

    public function getDefinition(): RectorDefinition
    {
        return new RectorDefinition('Replaces createMock() with createStub() when relevant', [
            new CodeSample(
                <<<'PHP'
use PHPUnit\Framework\TestCase
class MyTest extends TestCase {
    public function testItBehavesAsExpected(): void
    {
        $stub = $this->createMock(\Exception::class);
        $stub->method('getMessage')
            ->willReturn('a message');
        $mock = $this->createMock(\Exception::class);
        $mock->expects($this->once())
            ->method('getMessage')
            ->willReturn('a message');
        self::assertSame('a message', $stub->getMessage());
        self::assertSame('a message', $mock->getMessage());
    }
}
PHP
                 ,
                <<<'PHP'
use PHPUnit\Framework\TestCase
class MyTest extends TestCase {
    public function testItBehavesAsExpected(): void
    {
        $stub = $this->createStub(\Exception::class);
        $stub->method('getMessage')
            ->willReturn('a message');
        $mock = $this->createMock(\Exception::class);
        $mock->expects($this->once())
            ->method('getMessage')
            ->willReturn('a message');
        self::assertSame('a message', $stub->getMessage());
        self::assertSame('a message', $mock->getMessage());
    }
}
PHP
            ),
        ]);
    }

    /**
     * @return string[]
     */
    public function getNodeTypes(): array
    {
        return [MethodCall::class];
    }

    /**
     * @param MethodCall $node
     */
    public function refactor(Node $node): ?Node
    {
        if (! $this->isName($node->name, 'createMock')) {
            return null;
        }
        $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
        if (!$parentNode instanceof Assign) {
            return null;
        }

        dump($this->methodCallManipulator->findMethodCallNamesOnVariable($parentNode->var));die();

        return $node;
    }
}

I don't know much about parsers, but I suspect this method can only find method calls that have been made on the variable before reaching the current node, and not after. Here is what the fixture of the test I'm trying to pass looks like:

<?php

namespace Rector\phpunit\Tests\Rector\MethodCall\CreateMockToCreateStubRector\Fixture;

use PHPUnit\Framework\TestCase;

class MyTest extends TestCase
{
    public function testItBehavesAsExpected(): void
    {
        $stub = $this->createMock(\Exception::class);
        $stub->method('getMessage')
            ->willReturn('a message');
        $mock = $this->createMock(\Exception::class);
        $mock->expects($this->once())
            ->method('getMessage')
            ->willReturn('a message');
        self::assertSame('a message', $stub->getMessage());
        self::assertSame('a message', $mock->getMessage());
    }
}

?>
-----
<?php

namespace Rector\phpunit\Tests\Rector\MethodCall\CreateMockToCreateStubRector\Fixture;

use PHPUnit\Framework\TestCase;

class MyTest extends TestCase
{
    public function testItBehavesAsExpected(): void
    {
        $stub = $this->createStub(\Exception::class);
        $stub->method('getMessage')
            ->willReturn('a message');
        $mock = $this->createMock(\Exception::class);
        $mock->expects($this->once())
            ->method('getMessage')
            ->willReturn('a message');
        self::assertSame('a message', $stub->getMessage());
        self::assertSame('a message', $mock->getMessage());
    }
}

?>

[TomasVotruba]

What exactly you look for? I need 1-2 line exact code.

[greg0ire]

I managed to get $stub, and from that I was trying to a list of called methods (method and willReturn in the case of that variable, expects, method and willReturn in the case of $mock). From that list, I will be able to decide if I can change createMock to createStubor not. I thought$this->methodCallManipulator->findMethodCallNamesOnVariable($parentNode->var)` would provide that, but it does not.

Not sure what you mean by 1-2 line exact code, I hope I somehow answered your question.

[TomasVotruba]

Like:

$this->stub->call();

Match $this->stub if type is X

[greg0ire]

Ok, what about this:

        $mock = $this->createMock(\Exception::class);
        $mock->expects($this->once())
            ->method('getMessage')
            ->willReturn('a message');

Match $mock->expects() if type is MockObject, and refactor its creation (createMock) to createStub.

[TomasVotruba]

Good! Please send PR with WIP you have, so I can show it on code you already have instead of creating it from the zero.

[greg0ire]

Here you go: https://github.com/rectorphp/rector/pull/3042

[greg0ire]

Actually my example is wrong, I want to refactor only if expects is not called, so we should in fact match createMock

[TomasVotruba]

Could you update the PR, so it's in the code as well and we don't forget it?

[greg0ire]

I don't understand what you want me to update

[TomasVotruba]

Actually my example is wrong

This one. What example where? In the code? In test?

[greg0ire]

Match $mock->expects() if type is MockObject, and refactor its creation (createMock) to createStub.

This sentence is wrong. If you match $mock->expects(), then there is nothing to refactor already.

[TomasVotruba]

Ah, I get it.

That's why I prefer to have code and tests only and comment on them.
Text comments get messy and changed with no CI validation.

[TomasVotruba]

Working on it. It a bit tricky with nested method calls

[TomasVotruba]

Resolved by #3047

Thanks @greg0ire for the work