Add Mock client to common classes.

[samrap]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#55
Documentation
License	MIT

What's in this PR?

This PR adds the php-http/mock-client to the list of discoverable classes via the Common Classes Strategy.

Why?

Since Puli is not a hard dependency on this package, all php-http clients should be able to be discovered via the discovery. Since package developers using php-http are likely to rely on the Mock client in tests, it is useful to make this client discoverable for better unit testing.

Example Usage

// ThirdPartyService.php

use Http\Client\HttpClient;
use Http\Discovery\HttpClientDiscovery;

class ThirdPartyService
{
    public function __construct(HttpClient $http = null)
    {
        $this->http = $http ?: HttpClientDiscovery::find();
    }
}

Now, before this PR, if we were to pass no value into ThirdPartyService's constructor, the code would fail complaining about missing Puli dependency (assuming our package requires only php-http/mock-client). That means the following test in our package will fail:

// ThirdPartyServiceTest.php

use ThirdPartyService

class ThirdPartyServiceTest extends PHPUnit_Framework_TestCase
{
    public function setUp()
    {
        $this->service = new ThirdPartyService;
    }
}

This PR fixes that issue and allows the above test to pass, without requiring Puli and using the Mock HTTP client.

Checklist

• Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
• Documentation pull request created (if not simply a bugfix)

To Do

• If the PR is not complete but you want to discuss the approach, list what remains to be done here

[sagikazarmark]

I have just been thinking about this in the last few days. However, is there a scenario when you don't configure and inject the MockClient manually? That's the only thing that kept me from opening this PR myself.

[samrap]

In my opinion it's useful just for the fact of it being a feature of the discovery package that should be useable in tests. If we rely heavily on this discovery in our app then there can be many cases where it would be nice to test against our code that it does in fact provide that functionality.

[Nyholm]

If you put the mock client in common strategy you will risk that it is found in production. That would be wrong. Of course one could argue and say the the mock client should never be installed in production.

If we rely heavily on this discovery in our app then there can be many cases where it would be nice to test against our code that it does in fact provide that functionality.

How about you add your own strategy when doing the tests?

use ThirdPartyService

class ThirdPartyServiceTest extends PHPUnit_Framework_TestCase
{
    public function setUpBeforeClass()
    {
        HttpClientDiscovery::prependStrategy(MockClientDiscoveryStrategy::class);
    }
    public function setUp()
    {
        $this->service = new ThirdPartyService;
    }
}

[samrap]

I'm definitely of the opinion that a mock client should never be installed in production thus it should not be a concern. However I see and understand your argument too. Custom strategy works, but my biggest issue with this is I shouldn't have to write that small functionality just to mock what's already supported in the Discovery package.

[dbu]

but dont you need to define behaviour on the mock client anyways? i guess not when you only test other things in your application, but that smells like very incomplete tests. and if you have integration tests with real network calls, you need an acutal client installed anyways. i am just thinking loud, if there are good use cases i have no problem if we merge this. as longcas its lowest preference, and with according documentation. and make sure we have a warning in the mock readme to only ever require-dev it.

[sagikazarmark]

I agree with @dbu: I don't really see the usecase. The Mock client should be injected in tests with some preconfiguration. Otherwise what's the point.

Also, this would introduce some kind of fragility of tests: if (for any reasons) another client is installed, it will be discovered instead of the Mock client, which could break your tests.

I don't see why the following is a problem:

// ThirdPartyServiceTest.php

use ThirdPartyService

class ThirdPartyServiceTest extends PHPUnit_Framework_TestCase
{
    public function setUp()
    {
        $this->service = new ThirdPartyService(new MockClient());
    }
}

Discovery is well tested, so testing it should not be the responsibility of your tests. Even if you want to do that, I am not sure if it provides any valid results, since in production (as you said) you will use something different from the mock client.

[samrap]

The only valid result it provides that I can see is ensuring that the app/package being developed allows the user to instantiate an object using discovery. It helps ensure in my tests that my code implements discovery properly. This may be a trivial and unnecessary test, but it still feels like it should be there to me. While I understand the argument against the need for it, I don't see it causing any harm like @dbu said, as long as it is lowest preference. Going back to @dbu comment, you are not always defining behavior on the mock client either. Sometimes you simply need to test that the right request was sent which can very well be independent of any client configurations.

[Nyholm]

Im 👎 for including it in the common strategy. I might be okey with a separate strategy class that isn't loaded by default. Put that class in this repo or the mock client repo... or maybe just in the docs.

[samrap]

@Nyholm would the MockClientDiscoveryStrategy be a class you would be willing to merge into this package or perhaps the Mock client repo? I can get a merge request going for that today if so.

[sagikazarmark]

What if you use HTTPlug in an application? In that case you have both an actual (like Guzzle) and the Mock implementation. Discovery will always find Guzzle, which brings us to the test fragility point. You would have to carefully configure the strategies in that case (and also care about some tear down, since discovery is global/static)

Regarding a custom strategy: I am fine with any of what @Nyholm suggested.

[Taluu]

Here is a need that I currently have ; I'm currently developping an extension on Behat, which uses DI for stuff. I wanted to use DI (and factories and all that) to load the correct client, so I actually don't have a wrapper (just a call to the discovery tool). I had the surprise of "please install something like the guzzle adapter`...

But a mock strategy would be fine too.

[samrap]

I'll submit a new PR with mock strategy this week

[barryvdh]

I was also wondering this. I wanted to test that a client was resolved with discovery and use that in my tests. I guess I could just inject it, but perhaps a dev toggle option Discovery::enableMockClient()

[sagikazarmark]

That sounds like an interesting idea. Maybe also provide a trait in the MockClient which adds setUp and tearDown with the appropriate (enable/disable) calls to discovery.

How would this enable option work? Put the MockStrategy in front of the queue? That would still leave us with the problem of leaky tests (what if the MockClient is not available?)

@Nyholm what do you think?

[Nyholm]

I do not like the idea of speacial treating the mock strategy with Discovery::enableMockClient.
The discovery should treat all strategies equal.

About the trait: not sure we need a trait for one line of code. And as said before, the trait would not be useful for non-phpunit.