elasticsearch-php
elasticsearch-php copied to clipboard
elastic
Client should implement an interface to make unittesting possible.
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.
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.
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.
@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!
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?
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 :)
@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 ?
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.
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. 🙂
Solved in https://github.com/elastic/elasticsearch-php/pull/1249. Thanks @hkulekci for the suggestion!