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

Mock magic methods #80

Closed
armetiz opened this Issue Jan 13, 2014 · 27 comments

Comments

Projects
None yet
10 participants
@armetiz
Contributor

armetiz commented Jan 13, 2014

Hi there.

Sorry to open this issue, seems that it was already discussed, here, here and here.

But, right now there is no solution to mock object that didn't define public API using PHP implementations.
And sadly, many libraries uses magic calls, just talk about the example : AWS. I Guess that I can find other popular examples.

AWS use PHPDoc to describe API when doing magic functions, see PhpDoc @method.

So, what about using this information to read public API ? PHPStorm use this information to build auto-completion.

Regards,

@armetiz

This comment has been minimized.

Show comment
Hide comment
@armetiz

armetiz Jan 13, 2014

Contributor

For people in the trouble with this. You can implement a local class.

<?php

namespace spec\Acme\FileSystem\Storage;

use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class AmazonS3StorageSpec extends AbstractStorageSpec
{
    function let(S3Client $mockClient)
    {
        $mockClient->putObject(Argument::any())->willReturn(array());
        $this->beConstructedWith($mockClient);
    }
}

class S3Client extends \Aws\S3\S3Client
{
    public function putObject()
    {

    }
}
Contributor

armetiz commented Jan 13, 2014

For people in the trouble with this. You can implement a local class.

<?php

namespace spec\Acme\FileSystem\Storage;

use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class AmazonS3StorageSpec extends AbstractStorageSpec
{
    function let(S3Client $mockClient)
    {
        $mockClient->putObject(Argument::any())->willReturn(array());
        $this->beConstructedWith($mockClient);
    }
}

class S3Client extends \Aws\S3\S3Client
{
    public function putObject()
    {

    }
}
@everzet

This comment has been minimized.

Show comment
Hide comment
@everzet

everzet Jan 13, 2014

Member

@armetiz that's actually a neat idea. I'd accept a pull request with such a ClassPatch. Fancy to try?

Member

everzet commented Jan 13, 2014

@armetiz that's actually a neat idea. I'd accept a pull request with such a ClassPatch. Fancy to try?

@armetiz

This comment has been minimized.

Show comment
Hide comment
@armetiz

armetiz Jan 13, 2014

Contributor

I can try, I'm available on IRC #phpspec, ping me when you're available, I need some orientations to start this PR.

Contributor

armetiz commented Jan 13, 2014

I can try, I'm available on IRC #phpspec, ping me when you're available, I need some orientations to start this PR.

@dimsav

This comment has been minimized.

Show comment
Hide comment
@dimsav

dimsav May 30, 2014

@armetiz nice idea! Thanks for sharing :)

dimsav commented May 30, 2014

@armetiz nice idea! Thanks for sharing :)

@stof stof added the Feature request label Jun 2, 2014

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jul 18, 2014

Member

Closing as this was implemented months ago (and just released)

Member

stof commented Jul 18, 2014

Closing as this was implemented months ago (and just released)

@stof stof closed this Jul 18, 2014

@hackel

This comment has been minimized.

Show comment
Hide comment
@hackel

hackel May 10, 2015

Before I file this as a bug report, is this supposed to work if I add the @method tag to an interface and then mock the interface? I still get the "x is undefined" error in this case, but if I create a stub class that implements the interface, I can add the @method tag to it, and phpspec picks it up.

hackel commented May 10, 2015

Before I file this as a bug report, is this supposed to work if I add the @method tag to an interface and then mock the interface? I still get the "x is undefined" error in this case, but if I create a stub class that implements the interface, I can add the @method tag to it, and phpspec picks it up.

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal May 11, 2015

Member

@hackel seems very wrong to add an interface with the @method tags instead of actual methods. Why would you do that? Interface is a contract, and there's nothing to enforce it if you only add comments in.

Member

jakzal commented May 11, 2015

@hackel seems very wrong to add an interface with the @method tags instead of actual methods. Why would you do that? Interface is a contract, and there's nothing to enforce it if you only add comments in.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 11, 2015

Member

@hackel not reading @method phpdoc on interfaces was done on purpose (and this has been rejected when asking for it explicitly after that)

Member

stof commented May 11, 2015

@hackel not reading @method phpdoc on interfaces was done on purpose (and this has been rejected when asking for it explicitly after that)

@hackel

This comment has been minimized.

Show comment
Hide comment
@hackel

hackel May 11, 2015

@jakzal You're right--the only reason I would do it is to get phpspec to be able to mock methods on that interface without complaining with the "undefined" error.

@stof I wasn't able to find another issue discussing this, but why was it rejected?

PHP has no ability to specify that a method in an interface can be implemented in a class via a magic method, so it seems a (non-enforceable) annotation like this is the only way. I know many people seem to hate magic methods in general, but a lot of packages use them, and I find them incredibly useful. It allows to have a type of "inheritance" that is actually testable.

I can continue making stub classes and putting my annotations in there, it just seems like an unnecessary step to me.

hackel commented May 11, 2015

@jakzal You're right--the only reason I would do it is to get phpspec to be able to mock methods on that interface without complaining with the "undefined" error.

@stof I wasn't able to find another issue discussing this, but why was it rejected?

PHP has no ability to specify that a method in an interface can be implemented in a class via a magic method, so it seems a (non-enforceable) annotation like this is the only way. I know many people seem to hate magic methods in general, but a lot of packages use them, and I find them incredibly useful. It allows to have a type of "inheritance" that is actually testable.

I can continue making stub classes and putting my annotations in there, it just seems like an unnecessary step to me.

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal May 11, 2015

Member

@hackel think about it. If your interface doesn't have those methods, than it's actually not the right collaborator for your class. You seem to be relying on an implementation. It would break if someone provided an alternative implementation of your interface.

Member

jakzal commented May 11, 2015

@hackel think about it. If your interface doesn't have those methods, than it's actually not the right collaborator for your class. You seem to be relying on an implementation. It would break if someone provided an alternative implementation of your interface.

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal May 11, 2015

Member

see #169

Member

jakzal commented May 11, 2015

see #169

@hackel

This comment has been minimized.

Show comment
Hide comment
@hackel

hackel May 11, 2015

Thanks for the reference, @jakzal I forgot to search the PRs.

I know you're right that it would break if someone provided an alternative implementation that didn't implement those methods declared in the @method annotations, and there's zero enforcement of that. So are you saying there is no other way to do this "correctly" than to add all the methods to the interface, and create all those simple methods in the implementation that call the injected implementation's methods? As @jakubriedl said, this would result in an incredibly amount of seemingly unnecessary code duplication and maintenance.

I think this would be essential for a public package others may be re-using in their own projects, but for a private project, it seems unnecessary and not very agile. If there is a better way to do this that avoids that kind of redundant method duplication, I would love to change to that pattern!

hackel commented May 11, 2015

Thanks for the reference, @jakzal I forgot to search the PRs.

I know you're right that it would break if someone provided an alternative implementation that didn't implement those methods declared in the @method annotations, and there's zero enforcement of that. So are you saying there is no other way to do this "correctly" than to add all the methods to the interface, and create all those simple methods in the implementation that call the injected implementation's methods? As @jakubriedl said, this would result in an incredibly amount of seemingly unnecessary code duplication and maintenance.

I think this would be essential for a public package others may be re-using in their own projects, but for a private project, it seems unnecessary and not very agile. If there is a better way to do this that avoids that kind of redundant method duplication, I would love to change to that pattern!

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 24, 2015

I think @method docs should be parsed in Interfaces too. The best example I have is Predis : it defines all the method in the interface (with a __call), but in the Client, just an @inheritDoc is used.

You can't expect the interface to ask to implement all these methods (the list is quite long...) which is resolved by a simple __call implementation. Which is asked in the Interface too.

Basically, mocking this Client is impossible without making a dummy class extending the original Client and adding some @method tags...

Taluu commented Sep 24, 2015

I think @method docs should be parsed in Interfaces too. The best example I have is Predis : it defines all the method in the interface (with a __call), but in the Client, just an @inheritDoc is used.

You can't expect the interface to ask to implement all these methods (the list is quite long...) which is resolved by a simple __call implementation. Which is asked in the Interface too.

Basically, mocking this Client is impossible without making a dummy class extending the original Client and adding some @method tags...

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal Sep 27, 2015

Member

@Taluu that's an odd thing to do.

Anyway, don't mock what you don't own. Rather then mocking the interface from Predis, you should introduce your own one (perhaps simplified). Implementation of your interface would be a failry simple adapter for Predis, which could be integration tested (otherwise you have no guarantees that your assumptions about the predis client are correct). This way you'll avoid coupling to predis and will have an option to switch to another implementation.

Member

jakzal commented Sep 27, 2015

@Taluu that's an odd thing to do.

Anyway, don't mock what you don't own. Rather then mocking the interface from Predis, you should introduce your own one (perhaps simplified). Implementation of your interface would be a failry simple adapter for Predis, which could be integration tested (otherwise you have no guarantees that your assumptions about the predis client are correct). This way you'll avoid coupling to predis and will have an option to switch to another implementation.

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 27, 2015

@jakzal The need to reimplement (even with sub-interfaces and all) is, I think, a false argument. There will be a time when you will need to test said implementation, which is still a form of coupling. And at that time, doing what you are saying just won't work (loop).

And also, this class I am using is marked as a "Predis dependant", which itself respect an interface, which is Redis / Doctrine / whatever agnostic. And Redis (Well, PRedis actually) is used (and needed !) in other classes too, and I don't see myself doing an subjective implementation for every and each one of them ! Maintenance speaking, this would just be hell to maintain.

So I guess I'll have to continue by mocking a dummy extending the client class, which I think is too bad.

Taluu commented Sep 27, 2015

@jakzal The need to reimplement (even with sub-interfaces and all) is, I think, a false argument. There will be a time when you will need to test said implementation, which is still a form of coupling. And at that time, doing what you are saying just won't work (loop).

And also, this class I am using is marked as a "Predis dependant", which itself respect an interface, which is Redis / Doctrine / whatever agnostic. And Redis (Well, PRedis actually) is used (and needed !) in other classes too, and I don't see myself doing an subjective implementation for every and each one of them ! Maintenance speaking, this would just be hell to maintain.

So I guess I'll have to continue by mocking a dummy extending the client class, which I think is too bad.

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal Sep 27, 2015

Member

@Taluu as I said the implementation of adapter should be integration tested (not unit tested).

Member

jakzal commented Sep 27, 2015

@Taluu as I said the implementation of adapter should be integration tested (not unit tested).

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 27, 2015

@jakzal Maybe, but still, I can't bring myself having n adapters, having to maintain each one of them, and having to test them (via integration tests or unit tests) when they should always, knowing that they will do the same thing. They just won't depend on the the same calls (one would just need a set call, others will need that one and a get call, ... etc).

And also, IMO, unit tests and integrations tests are just testing things on a different POV, so they should not be testing differents things.

I know I am using the easy way out (mocking the dummy) though, and that's what I am saying it is too bad. I also agree that an interface shouldn't have @method block (I really do) but there are some times when it is needed (this was my point to begin with !). I also know that this mocking framework is highly opiniated (@everzet said it himself, I think it was about the mock chaining), and I'm fine with that too.

Just "meh".

Taluu commented Sep 27, 2015

@jakzal Maybe, but still, I can't bring myself having n adapters, having to maintain each one of them, and having to test them (via integration tests or unit tests) when they should always, knowing that they will do the same thing. They just won't depend on the the same calls (one would just need a set call, others will need that one and a get call, ... etc).

And also, IMO, unit tests and integrations tests are just testing things on a different POV, so they should not be testing differents things.

I know I am using the easy way out (mocking the dummy) though, and that's what I am saying it is too bad. I also agree that an interface shouldn't have @method block (I really do) but there are some times when it is needed (this was my point to begin with !). I also know that this mocking framework is highly opiniated (@everzet said it himself, I think it was about the mock chaining), and I'm fine with that too.

Just "meh".

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Jan 11, 2016

Hi,

Is there any way to solve this issue? AWS SDK (and mostly all Guzzle based clients) are using __call, which make it impossible to test a good part of my code. I'm reverting to PHPUnit mocks for now, but I'd be happy to hear about a simple solution :).

bakura10 commented Jan 11, 2016

Hi,

Is there any way to solve this issue? AWS SDK (and mostly all Guzzle based clients) are using __call, which make it impossible to test a good part of my code. I'm reverting to PHPUnit mocks for now, but I'd be happy to hear about a simple solution :).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 11, 2016

Member

@bakura10 magic methods can be mocked with Prophecy when they are documented with @method on the class.

Member

stof commented Jan 11, 2016

@bakura10 magic methods can be mocked with Prophecy when they are documented with @method on the class.

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Jan 11, 2016

Really? I tried that and it didn't work. I'll try again see if I made something wrong then.

bakura10 commented Jan 11, 2016

Really? I tried that and it didn't work. I'll try again see if I made something wrong then.

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Jan 11, 2016

Hi,

I tried again and it definitely seems to not work. I'm mocking the DynamoDbClient and trying to mock the query method, which is defined here in the @method annotation:

https://github.com/aws/aws-sdk-php/blob/master/src/DynamoDb/DynamoDbClient.php#L32

Here is a part of my code:

$dynamoDbClient = $this->prophesize(DynamoDbClient::class);
$dynamoDbClient->query()->willReturn($dynamoDbResult);

I can confirm this method is called, and it works with PHPUnit. It also works for any method explicitly defined in the DynamoDbClient. It just fails with any of the magic method. The error I'm having:

Prophecy\Exception\Call\UnexpectedCallException: Method call:
  - query(["TableName" => "shops", "KeyConditionExpression" => "#domain = :domain", "FilterExpression" => "authorization.authorized_at = :authorized_at", "ExpressionAttributeNames" => ["#domain" => "domain"], "ExpressionAttributeValues" => [":domain" => ["S" => "test.com"], ":authorized_at" => ["S" => "2015-01-01"]]])
on Double\Aws\DynamoDb\DynamoDbClient\P48 was not expected, expected calls were:
  - query()

Thanks!

bakura10 commented Jan 11, 2016

Hi,

I tried again and it definitely seems to not work. I'm mocking the DynamoDbClient and trying to mock the query method, which is defined here in the @method annotation:

https://github.com/aws/aws-sdk-php/blob/master/src/DynamoDb/DynamoDbClient.php#L32

Here is a part of my code:

$dynamoDbClient = $this->prophesize(DynamoDbClient::class);
$dynamoDbClient->query()->willReturn($dynamoDbResult);

I can confirm this method is called, and it works with PHPUnit. It also works for any method explicitly defined in the DynamoDbClient. It just fails with any of the magic method. The error I'm having:

Prophecy\Exception\Call\UnexpectedCallException: Method call:
  - query(["TableName" => "shops", "KeyConditionExpression" => "#domain = :domain", "FilterExpression" => "authorization.authorized_at = :authorized_at", "ExpressionAttributeNames" => ["#domain" => "domain"], "ExpressionAttributeValues" => [":domain" => ["S" => "test.com"], ":authorized_at" => ["S" => "2015-01-01"]]])
on Double\Aws\DynamoDb\DynamoDbClient\P48 was not expected, expected calls were:
  - query()

Thanks!

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Jan 11, 2016

Note: it actually works if I specify the FULL payload. Don't I have a way to tell the mocked method "accept any array, no matter what is inside" ?

bakura10 commented Jan 11, 2016

Note: it actually works if I specify the FULL payload. Don't I have a way to tell the mocked method "accept any array, no matter what is inside" ?

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Jan 11, 2016

You havnen't specified the arguments, so it is failing. You would need to use Argument::etc() or Argument::any()

Taluu commented Jan 11, 2016

You havnen't specified the arguments, so it is failing. You would need to use Argument::etc() or Argument::any()

@bakura10

This comment has been minimized.

Show comment
Hide comment
@bakura10

bakura10 Jan 11, 2016

Yeaah! Made the trick. Thanks you!

bakura10 commented Jan 11, 2016

Yeaah! Made the trick. Thanks you!

@jepster

This comment has been minimized.

Show comment
Hide comment
@ihr-it-projekt

This comment has been minimized.

Show comment
Hide comment
@ihr-it-projekt

ihr-it-projekt May 29, 2018

@jepster the link dosent work... Can you refresh the link?

ihr-it-projekt commented May 29, 2018

@jepster the link dosent work... Can you refresh the link?

@jepster

This comment has been minimized.

Show comment
Hide comment
@jepster

jepster May 30, 2018

@ihr-it-projekt The current URL is: https://mobilefish.de/mix-prophecy-mock-framework-phpunit-mock-builder. You can also use the search function, if the link is outdated in any case.

jepster commented May 30, 2018

@ihr-it-projekt The current URL is: https://mobilefish.de/mix-prophecy-mock-framework-phpunit-mock-builder. You can also use the search function, if the link is outdated in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment