search

eloquent / phony

Mocks, stubs, and spies for PHP.
http://eloquent-software.com/phony
MIT License
194 stars 7 forks source link

API improvement discussion #229

Closed enumag closed 6 years ago

enumag commented 7 years ago

Currently my biggest issue with Phony is that after

$handle = Phony::mock(MyClass::class);
$handle->method->returns('whatever');
$mock = $handle->get();

PHPStorm and tools like phpstan do not know that $mock instanceof MyClass. Of course I can add @var annotation for PHPStorm and add an ignored error to phpstan but I don't like that. And I don't like having so many variables in the code either.

Recently I was thinking about some API like this.

$mock = Helpers::createPhonyMock(
    MyClass::class,
    function (InstanceHandle $handle) {
        $handle->method->returns('whatever');
    }
);

It is limiting though because InstanceHandle is not easily available outside of the callback and writing the callback is a pain.

The point of such API is that Helpers::createPhonyMock() always returns an instance of the class that is passed in the first argument. Unlike the first example, this is actually possible to handle in both PHPStorm and phpstan using DynamicReturnTypePlugin and dynamic return type extensions.

Today I found out there is a method Phony::on() which can return InstanceHandle for existing mock. With this method it might be better to use API like this.

$mock = Helpers::createPhonyMock(MyClass::class);
Phony::on($mock)->method->returns('whatever');

It removes the downsides of the second approach while preserving the option of using the dynamic return type tools linked above.

  1. Do you think something like $mock = Phony::createMock($class); should be added to Phony directly?
  2. Do you have some other hidden trick that you use to solve this?
ezzatron commented 7 years ago

Thanks for the detailed question!

I don't use an IDE with those kinds of capabilities, and I've only used phpstan on regular source code, not tests. So I haven't really considered this use case thoroughly. I will have to do a bit of research and think about it.

Off the top of my head, there are three other ways to acquire mocks you could look into:

# 1: Switch to using Phony for Kahlan or Phony for Peridot. Both of these support mocking via dependency injection using regular PHP type hints. I'm almost certain static analysis tools will work fine using these tools:

Injected mock objects

Phony for Kahlan supports automatic injection of mock objects. Because Phony doesn't alter the interface of mocked objects, it is necessary to use on() to retrieve the mock handle in order to perform stubbing and verification:

use function Eloquent\Phony\Kahlan\on;

describe('Phony for Kahlan', function () {
    it('Supports stubbing', function (PDO $db) {
        on($db)->exec->with('DELETE FROM users')->returns(111);

        expect($db->exec('DELETE FROM users'))->toBe(111);
    });

    it('Supports verification', function (PDO $db) {
        $db->exec('DROP TABLE users');

        on($db)->exec->calledWith('DROP TABLE users');
    });
});

This option will require a small amount of configuration. See the individual repos linked above for more info.

# 2: Mock builders do return "bare" mocks, but they still work like:

$mock = Phony::mockBuilder(ClassA::class)->full();

so I don't know if they're going to make it any easier for static analysis.

# 3: The emptyValue() facade function will return "bare" mocks, but you'd have to supply it with a ReflectionType, and those are non-trivial to create:

$type = new ReflectionFunction(function(): ClassA {})->getReturnType();
$mock = Phony::emptyValue($type);

This probably doesn't help you much either.

Aside from those options, you might not be aware that mock creation does not always take a single type. For instance, you can create a union mock of multiple different types:

$handle = Phony::mock([ClassA:class, InterfaceA::class, TraitA::class]);

And you can also create ad-hoc mocks:

$handle = Phony::partialMock(
    [
        '__toString' => function () {
            return 'What is this sorcery?';
        },
        '__call' => function ($name, array $arguments) {
            return sprintf('%s(%s)', $name, implode(', ', $arguments));
        },
    ]
);

Which could both (possibly) be challenging situations to support in the tools you mentioned.

Anyway, I'll definitely think about it, and ask some people at work who use phpstan more than I do. Thanks again for raising the issue!

enumag commented 7 years ago

Kahlan approach looks interesting but it's not really compatible with PHPUnit tests, mainly it would be difficult to implement together with data providers.

MockBuilder does not help. The problem is the same as with normal Phony::mock() - the mock itself is not returned by the method I pass the type to but only by a following call like ->get() or ->full(). For the dynamic return type tools to work it would have to be a single call:

public static function createMock(string $class)
{
    return Phony::mock($class)->get();
}

Phony::emptyValue() is pretty much what I thought about with Phony::createMock() except it would have to accept string instead of ReflectionType.

I'm aware of multi-type and ad-hoc mocks, I just barely ever need them. I just want to solve the most simple case which I use pretty much everywhere. I don't mind using annotations and phpstan ignoreErrors for more complicated cases like these.

ezzatron commented 7 years ago

A couple of quick questions:

  1. How would you feel about a dedicated Phony plugin for PHPStorm? Do you think typical storm users would consider that too much hassle? Depending on what's actually possible, a dedicated plugin could theoretically do some pretty cool stuff like argument lists for stubbing and verification too.
  2. Would you mind creating a gist with, or providing a link to, some "real-world" example of how you're using Phony? I'm pretty sure I understand the case well, but that would help me to be sure.
enumag commented 7 years ago

  1. PHPStorm plugin would be great. Typical user has at least a plugin for his framework and couple of others so I don't think it would be too much hassle. Actually I believe it might raise the popularity of Phony a little because then there would be pretty much no reason to stick with some normal expect-run-verify framework.

  2. I think I'll just link you to my open-source projects where I use Phony. You can look at other tests in these repositories or in that organization if you need more.

Quick question about how Phony::on() works.

  1. What is the point of using $handle = Phony::on($otherHandle);? Wouldn't the variables be the same?
  2. If I call Phony::on($mock)->method->with()->returns(); multiple times, then some code with the mock, then Phony::on($mock)->method->calledWith(); will everything work as expected or should I expect some issues with using different handle instance in each of those chains? Or does on return the same instance every time?
enumag commented 7 years ago

... That said I'd still appreciate some slight improvement to the API as described in the first post. PHPStorm plugin can surely offer many cool features but it should not be a necessity for even the most simple use-case.

ondrejmirtes commented 7 years ago

I just reacted to https://github.com/phpstan/phpstan/issues/163 - something like Helpers::createPhonyMock is not needed for PHPStan at all, because I already support syntax similar to:

$handle = Phony::mock(MyClass::class);
$mock = $handle->get();

for PHPUnit's MockBuilder, so I'm sure the same can be done for Phony as well.

ezzatron commented 7 years ago

@enumag I had a quick look at some of your tests, and one thing I noticed is that you're using $handle->get() unnecessarily in lots of places. This probably explains, in large part, why the API feels cumbersome to you.

Phony has a feature called mock handle substitution which allows you to use the mock handle to represent the mock in pretty much every interaction with the Phony API. When properly utilized, it means that you almost never have to call $handle->get(), outside of injecting mocks into the system you are testing.

To give you an example, these two methods from PermissionAuthorizatorTest.php (reformatted slightly to fit comment width):

    protected function _before(): void
    {
        $this->firewallHandle = Phony::mock(FirewallInterface::class);
        $this->permissionHandle = Phony::mock(Permission::class);
        $this->authorizator = new PermissionAuthorizator(
            $this->firewallHandle->get(),
            $this->permissionHandle->get()
        );
    }

    // ...

    public function testRoles(): void
    {
        $identityHandle = Phony::mock(IIdentity::class);
        $identityHandle
            ->getRoles
            ->returns(
                [
                    'role1',
                    'role2',
                    'role3',
                ]
            );
        $identity = $identityHandle->get();
        $this->firewallHandle->getIdentity->returns($identityHandle);
        $this->permissionHandle
            ->isAllowed
            ->with('role1', 'resource', 'privilege')
            ->returns(false);
        $this->permissionHandle
            ->isAllowed
            ->with('role2', 'resource', 'privilege')
            ->returns(true);
        self::assertTrue($this->authorizator->isAllowed('resource', 'privilege'));
        $this->permissionHandle
            ->setIdentity
            ->calledWith($identity);
    }

Can be written more succinctly like this:

    protected function _before(): void
    {
        $this->firewall = Phony::mock(FirewallInterface::class);
        $this->permission = Phony::mock(Permission::class);
        $this->authorizator = new PermissionAuthorizator(
            $this->firewall->get(),
            $this->permission->get()
        );
    }

    // ...

    public function testRoles(): void
    {
        $identity = Phony::mock(IIdentity::class);
        $identity->getRoles->returns(['role1', 'role2', 'role3']);

        $this->firewall->getIdentity->returns($identity);

        $this->permission->isAllowed
            ->with('role1', 'resource', 'privilege')
            ->returns(false);
        $this->permission->isAllowed
            ->with('role2', 'resource', 'privilege')
            ->returns(true);

        self::assertTrue($this->authorizator->isAllowed('resource', 'privilege'));
        $this->permission->setIdentity->calledWith($identity);
    }

I was initially confused when you mentioned having "so many variables in the code", but it makes sense that if you don't know about this feature, you would probably assume that you have to assign the mock instance somewhere just to write these kinds of rules involving multiple mocks. Thankfully, you don't have to do that in most cases.

I will try to submit a couple of PRs for your test suites, because I think that will be the best way to illustrate how much this can simplify your tests.

  1. What is the point of using $handle = Phony::on($otherHandle);? Wouldn't the variables be the same?

There are static handles as well (pretty edge case stuff). You can get a static handle from an instance handle and vice versa.

When designing the API, I decided it would be more consistent and fool-proof to allow any type of handle to be retrieved from any other type of handle or mock instance, rather than explicitly disallowing it for handles of the same type.

  1. If I call Phony::on($mock)->method->with()->returns(); multiple times, then some code with the mock, then Phony::on($mock)->method->calledWith(); will everything work as expected or should I expect some issues with using different handle instance in each of those chains? Or does on return the same instance every time?

The on() function returns the same instance every time, so everything would work as you expect.

enumag commented 7 years ago

@ezzatron I don't see any change aside from the fact that you removed the Handle suffix from some variables and properties. Did I miss something?

ezzatron commented 7 years ago

The real difference in that example is not the renaming of variables, but the fact that you don't need to "pull out" the actual mock object with ->get() in many places where you currently are. A simpler example:

$database = mock(Database::class);
$query = mock(Query::class);
$result = mock(Result::class);

// these two statements are equivalent
$database->select->returns($result);
$database->select->returns($result->get());

// so are these
$database->select->with($query)->returns($result);
$database->select->with($query->get())->returns($result->get());

// so are these
$database->select->calledWith($query);
$database->select->calledWith($query->get());

In "typical" Phony usage, taking advantage of mock handle substitution, it is not necessary to use $handle->get() once the mock has been injected into the system you are testing. This means there is usually no need to maintain a reference to both the handle and the mock.

This is why I dropped the "Handle" suffix from your variable names. Once you've removed direct usage of the mock object, there's no need to disambiguate the handle and the actual mock with a suffix.

enumag commented 7 years ago

I see, that's a good point. Don't worry about sending PRs to my tests though, I think I can handle the refactoring myself. Your time is better spent on PhpStorm and/or PHPStan plugins (if you decide to make them). Thank you!

ezzatron commented 7 years ago

Some updates here: