googleads / google-ads-php

Google Ads API Client Library for PHP
https://developers.google.com/google-ads/api/docs/client-libs/php
Apache License 2.0
295 stars 262 forks source link

Mock GoogleAdsClient #347

Closed vienthuong closed 1 year ago

vienthuong commented 4 years ago

HI there, I got a similar issue like in #294 and the example doesn't help, I wish there is setCredentialsConfig in Google\Ads\GoogleAds\Lib\V3\ServiceClientFactoryTrait so that we can set some mock http client into the client then It won't send the actual request to Google Ads API.

fiboknacky commented 4 years ago

Pierrick, could you please confirm if this is the same as this one?

PierrickVoulet commented 4 years ago

Pierrick, could you please confirm if this is the same as this one?

I believe it is strongly related but I do not think it is exactly the same thing:

vienthuong commented 4 years ago

@PierrickVoulet P Yes that's it! Because the GoogleAdsClient is final class so I cant just mock it, I tried using an adapter but it looks ugly and redundant since it only created for testing purpose.

77web commented 3 years ago

Hi, transferred from #639 Why not simply remove "final" keyword from GoogleAdsClient? It solves most of testing problem.

PierrickVoulet commented 3 years ago

Hi, transferred from #639 Why not simply remove "final" keyword from GoogleAdsClient? It solves most of testing problem.

The reasons are not specific to this library or class, I find them pretty well explained in this article. Note: We do not implement any interface here because there are no public methods, only a constructor.

I never tested this package myself but you may be interested to experiment with it or some other similar solution for your testing.

We are open to discussion, feel free to propose alternatives. There may be other ways to ease testing while keeping the same level of control.

PierrickVoulet commented 3 years ago

It is possible to edit class & method flags with the extension uopz using the function uopz_flags.

Here is an example where I remove the final flag of the class GoogleAdsClientBuilder and test an extended version of the class with PHPUnit:

$flags = uopz_flags(GoogleAdsClientBuilder::class, null);
uopz_flags(GoogleAdsClientBuilder::class, null, $flags & ~ZEND_ACC_FINAL);

class GoogleAdsClientBuilderExt extends GoogleAdsClientBuilder {
    public function build() {
        return null;
    }
}

class GoogleAdsClientTest extends TestCase
{
    public function test()
    {
        assertNull((new GoogleAdsClientBuilderExt())->build());
    }
}

This can be used to mock GoogleAdsClient or GoogleAdsClientBuilder if you need:

public function test()
{
    $mock = $this->getMockBuilder(GoogleAdsClientBuilder::class)->getMock();
    // ...
}

I am closing this issue but feel free to add comments if you need to follow up.

bizurkur commented 2 years ago

Hi, transferred from #639 Why not simply remove "final" keyword from GoogleAdsClient? It solves most of testing problem.

The reasons are not specific to this library or class, I find them pretty well explained in this article. Note: We do not implement any interface here because there are no public methods, only a constructor.

If you read Marco's article that you referenced, the very first thing it says is ONLY apply final if they implement an interface. The Google client does not implement an interface. You're going against best practices and making things more difficult for people who want to write quality code and write tests that need access to a dummy client.

If you read Marco's post it very clearly describes

When to avoid final:

Final classes only work effectively under following assumptions:

  1. There is an abstraction (interface) that the final class implements
  2. All of the public API of the final class is part of that interface

If one of these two pre-conditions is missing, then you will likely reach a point in time when you will make the class extensible, as your code is not truly relying on abstractions.

The ONLY correct answer to this issue is final should be removed or the client should have an interface.

UltimateFlex commented 1 year ago

Hi there! I'm facing the same issue with GoogleAdsClient. I want to mock it but the final class make it impossible to do so. Is there any possibility to remove final from this class. I understand the meaning of final but it feels like bad practice to use final in libraries like this if there is no interface provided.

fiboknacky commented 1 year ago

Fixed with v20.1.0

77web commented 7 months ago

Hi, it seems that "final" has come back for V16 Service Client classes in v22.1.0 unfortunately. They were not marked as final previous versions like V15.

https://github.com/googleads/google-ads-php/tree/main/src/Google/Ads/GoogleAds/V16/Services/Client

They do not have proper interface to implement, so they should not be marked as final.

fiboknacky commented 7 months ago

Edited: Thanks for reporting. It looks like what you reported is different from the previously reported issue.

This one is about making the GoogleAdsClient final, but from what you said, you'd like generated clients to be final too, right? I know that the previously generated classes didn't have the "final" modifier before.

Could you tell me more what's your use case is? Are there any special functionalities that you need to inject into the classes before using them to send a request to the API server?