Client should implement an interface to make unittesting possible.

[ghost]

Summary of problem or feature request

Since client was made final we can not mock the class anymore. However, good practice would tell us that we need to mock the edges of our systems. Of course ES is such an edge.

Therefore, whenever important classes like this are made final, an interface should be created to help us mock that instead. Of course it also helps us think about the system through an interface instead of an implementation.

I assume there are more people running into this problem so I have a pull request ready to go.

[Ekman]

I agree. Another solution would be to simply remove final. Sure, it makes sense to not extend the client but the hassle it creates with testing makes it simply not worth adding the keyword.

[hkulekci]

Do we have any updates here? the implementation of #1228 is good but the problem here is the traits generated by a script. So, in my opinion, that generator needs to create interfaces automatically per trait, and the client interface needs to extend these interfaces. @ezimuel what do you think? We struggled in this library also to be able to test some parts after upgrading to the latest version.

[ezimuel]

@davidmaes, @Ekman , @hkulekci first of all I'm sorry for the long wait. You are right, we need to provide an Interface to be sure that we can mock the Client easily. As pointed out by @hkulekci this interface should be generated, since the Client used some generated code (i.e. ClientEndpointsTrait. I'm working on a PR for that. Thanks!

[ezimuel]

Since I need to generate the interface based on the API endpoints released in Elasticsearch I can have different interfaces between two minor versions. This can be a BC break! I'm considering to remove the final keyword for the Client. Thoughts?

[hkulekci]

What do you think about creating an interface for the Client method instead of all functions only? The libraries can mock the sendRequest method easily. This is a quick opinion. I did not think about it much :)

[ezimuel]

@hkulekci Do you mean an interface only for the Client methods without the ClientEndpointsTrait and NamespaceTrait? It sounds a great idea! Any thoughts @davidmaes and @Ekman ?

[hkulekci]

Yes exactly. As I see from my limited view, all the public methods which are sending a request to Elasticsearch use sendRequest method as well. So, mocking this request will be a solution. For the NamespaceTrait, the classes are using AbstractEndpoint and this class depends on Client. All the methods of these classes also use the same method of Client.

[Ekman]

Mocking sendRequest would mean users needs to concern themselves with internal lib stuff in order to mock. I mean, it works, but it's not ideal. I'd prefer removing final or an interface but I also understand implementing simpler solutions over more complex. 🙂

[ezimuel]

@hkulekci , @Ekman I just sent this PR #1249

[hkulekci]

I put a comment there. Thanks 🥳

[ezimuel]

Solved in https://github.com/elastic/elasticsearch-php/pull/1249. Thanks @hkulekci for the suggestion!

[alcarper]

I think the best will be to delete the final keyword in the client, the implementation of the interface is not enough, I will put a case as an example

use Elastic\Elasticsearch\Client;

class ProductionClass
{
    private Client $elasticClient;

    public function __construct(Client $elasticClient)
    {
        $this->elasticClient = $elasticClient;
    }

    public function sendSomething(): void
    {
         $this->elasticClient->index([
            //whatever
        ]);
    }
}

class TestClass extends TestCase
{
    public function testSendSomethingMakeTheCorrectRequestToElastic(): void
    {
        $elasticClient = $this->createMock(Client::class);
        $sut = new ProductionClass($elasticClient);

        $elasticClient->expects($this->once())
           ->method('index')
           ->with([/*whatever*/]);

        $sut->sendSomething();
    }
} 

This case is possible without the final keyword but it can't be done with the final keyword.

The accepted solution of creating the interface is not working either because the interface doesn't have the index method.

Adding all the API endpoints to the interface are discarded based on this comment so I think that the solution should be to remove the final keyword.

I don't see an advantage in adding a final keyword in a public class of a package. I think it's good to let people extend or override whatever method they want to.

@ezimuel what do you think? I consider that the main problem of this issue is not solved, so my suggestion is to reopen it