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

setMethods(), setMethodsExcept(), addMethod(), onlyMethods() #3911

Closed
sebastianbergmann opened this issue Oct 28, 2019 · 13 comments
Closed

setMethods(), setMethodsExcept(), addMethod(), onlyMethods() #3911

sebastianbergmann opened this issue Oct 28, 2019 · 13 comments

Comments

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Oct 28, 2019

I failed at keeping track of the discussion of this subject as it is currently (or rather was until the creation of this ticket) scattered across #3858, #3886, #3897, and #3907. I have decided to close these issues and pull requests and would kindly ask you to discuss this topic here in this new issue. I apologize for any inconvenience that this may cause. I hope that we come to a conclusion on how to proceed quickly. At that time I will gladly and promptly merge a pull request that implements what is agreed upon here.

In #3687, @DFoxinator implemented MockBuilder::addMethods() and MockBuilder::onlyMethods() as alternatives to MockBuilder::setMethods(). These new methods were published as part of PHPUnit 8.3.

When developers started using MockBuilder::addMethods() and MockBuilder::onlyMethods() with PHPUnit 8.3, they ran into limitations for which the aforementioned separate issues and pull requests were created.

Can somebody please summarize these limitations here? Thanks!

@bikalbasnet
Copy link
Contributor

@bikalbasnet bikalbasnet commented Oct 28, 2019

Summary from the both issues.

  1. Issue 1 : setMethods() !== addMethods() + onlyMethods()

Originally the issue @DFoxinator wanted to solve was to

Right now, any time setMethods is used, the test using it is completely unprotected against any of the methods sent to setMethods not actually existing in the class

Thus two methods

  • addMethods for non existing methods
  • onlyMethods for existing methods
    were added by deprecating setMethods, but this is not backward compatible because there is a case when we might want to mock the class with both existing and non existing methods together. These case mainly arise when we are using magic methods and magic methods are very common still.

For example we can not mock this kind of class anymore

class Parent
{
    public function &__get($field)
    {
        return $this->get($field);
    }
    
    public function get($field)
    {
         // some code to return value from `$field`
    }
}

/**
* @method string getFirstName()
* @method string getLastName()
*/
class Children extends Parent
{
    public function getId()
    {
        return $this->get('id');
    }
}

Previously we could have mock this class like this

$mock = $this->getMockBuilder(Children::class)
    ->setMethods(['getId', 'getFirstName', 'getLastName'])
    ->getMock();

But now we can't write a code equivalent to above mock


  1. Issue 2 : setMethodsExcept() still mocks the method for a class with single public function
class ClassWithSinglePublicMethod
{
    public function lonelyMethod() {
        return 'lonely';
    }
}

For this above class setMethodsExcept(['loneyMethod']) still mocks with loneyMethod function

public function testSingleClass() {
    $mock = $this->getMockBuilder(ClassWithSinglePublicMethod::class)
         ->setMethodsExcept(['lonelyMethod'])
         ->getMock();
   $this->assertEquals('lonely', $mock->lonelyMethod());
   // This test should have passed but it fails
}

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Oct 28, 2019

@bikalbasnet I am sorry but your first example in #3911 (comment) makes no sense to me. Can you provide an example for the issue that does not use interceptors such as __get()?

Loading

@bikalbasnet
Copy link
Contributor

@bikalbasnet bikalbasnet commented Oct 28, 2019

@sebastianbergmann Sorry at the moment, I can't think of other cases that does not involve magic methods, but let me simplify the example 1 above with this

Let's say this our class that we need want to mock

class SomeClass
{
    public function aMethodThatExists()
    {
        return 123;
    }
}

Previously we could mock above class like this

public function testSomething()
{
    $mock = $this->getMockBuilder(SomeClass::class)
        ->setMethods(['aMethodThatExists', 'aMethodThatDoesNotExist'])
        ->getMock();

    $this->assertNull($mock->aMethodThatExists()); // PASS
    $this->assertNull($mock->aMethodThatDoesNotExist()); // PASS
}

But now we can't write the UTs equivalent to above. I want to do something like this but it's not possible.

$mock = $this->getMockBuilder(SomeClass::class)
    ->addMethods(['aMethodThatDoesNotExist'])
    ->onlyMethods(['aMethodThatExists'])
    ->getMock();

because I get error

Cannot use onlyMethods() on "\SomeClass" mock because mocked methods were already configured.

Loading

@DFoxinator
Copy link
Contributor

@DFoxinator DFoxinator commented Oct 28, 2019

The specific issue we ran into is with a class we're testing that has a client class as a dependency. The client class has a number of real public methods, but then also a lot of public magic methods. For the test to remain as-is, we'd need to mock both real methods and magic methods on the client class.

Specifically, this was an issue for us with the AWS SDK. It mixes real public methods and real magic methods, and some of our code calls combinations of both of those in the same test on the same client instance.

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Oct 28, 2019

Maybe we need more specific methods for methods that exist in the original class/interface and for methods that do not exist in the original class/interface?

Loading

@DFoxinator
Copy link
Contributor

@DFoxinator DFoxinator commented Oct 28, 2019

Any ideas/example of what that would look like? I'm not sure I understand completely.

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Oct 28, 2019

Does not make sense to me anymore, sorry for the noise.

Loading

@moskalen
Copy link

@moskalen moskalen commented Oct 28, 2019

Here is the example mentioned by @DFoxinator in #3911 (comment):

Class S3Client contains 175 magic methods and bunch of real methods from interface S3ClientInterface.

Our class uses both magic, and real methods from S3Client what causes us next issues:

  1. If we use either magic, or real methods we have to create a mock of S3Client inside each test
public function testADuringUsingRealMethods() {
   $s3_client = $this->getMockBuilder(S3Client::class)
      ->disableOriginalConstructor()
      ->onlyMethods(['createPresignedRequest', 'getCommand'])
      ->getMock();
   ...
}

public function testBDuringUsingMagicMethods() {
   $s3_client = $this->getMockBuilder(S3Client::class)
      ->disableOriginalConstructor()
      ->addMethods(['putObject'])
      ->getMock();
   ...
}

rather than to create one mock in setUp():

protected function setUp() {
  ...
  $this->s3_client = $this->getMockBuilder(S3Client::class)
      ->disableOriginalConstructor()
      ->onlyMethods(['createPresignedRequest', 'getCommand'])
      ->addMethods(['putObject'])
      ->getMock();
  ...
}
  1. If we use both magic, and real methods the code is not testable, and we have to add extra method to wrap magic method call:
public function main() {
  ...
  $this->putObjectToS3($params);
  ...
  $command = $this->s3_client->getCommand();
  ...
}

// extra methods which is needed only to wrap magic method 
// so that the code will be testable
public function pubObjectToS3(array $params) {
  $this->s3_client->putObject($params);
}

And, of course, a new test is required so that to test wrapper pubObjectToS3.

All these issues are resolved if both onlyMethods(), and addMethods are allowed for the same mock.

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Oct 29, 2019

Is there consensus that onlyMethods() and addMethods() should be combinable? If so, then I would accept a pull request for this for PHPUnit 8.5.

Loading

@bikalbasnet
Copy link
Contributor

@bikalbasnet bikalbasnet commented Oct 29, 2019

👍 to make it combinable

Loading

@DFoxinator
Copy link
Contributor

@DFoxinator DFoxinator commented Oct 30, 2019

@bikalbasnet are you going to update/make a PR for this?

Loading

@bikalbasnet
Copy link
Contributor

@bikalbasnet bikalbasnet commented Oct 30, 2019

@DFoxinator I was thinking I could re open my PR 🤔 #3907
and @mbrostami can re open his as these two issues can be solved separately
@sebastianbergmann or do we need to combine them?

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Oct 30, 2019

Whatever works.

Loading

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

Successfully merging a pull request may close this issue.

None yet
4 participants