Every client should have its own Mock instance

[ruudk]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
License	MIT

What's in this PR?

This PR makes the MockFactory a real factory.

Why?

When you have multiple configured clients, and want to mock them in test mode, you can change the factory method to httplug.factory.mock. That's awesome. But they all use the same instance of Http\Mock\Client which is problematic.

If you have 1 service that calls 2 http clients after each othere, there is no way of properly configuring things.

This PR makes sure every client gets its own mock instance that you can access by calling httplug.client.acme.client (see #287)

Checklist

• Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

[dbu]

if i remember correctly, the intention of this was that you could prepare a mock and set it in the factory, or get the mock from the factory and set things on it. that way you can manage what the mock should be doing even if the code you are testing is using the factory.

i can see what you want to do and with properly built code that uses DI, it would work fine. however, when you have such code, why use the factory at all? then you can define as many clients as you want as mock clients whithout using the factory at all.

i would rather not change this, as the side effects can be significant.

[ruudk]

I just tried this:

# app/config/httplug.yaml
httplug:
    clients:
        my_service:
            factory: 'httplug.factory.guzzle5'
            config:
                defaults:
                    connect_timeout: 3
                    timeout: 3

# app/config/httplug_test.yaml
services:
    httplug.clients.my_service.mock:
        class: Http\Mock\Client

httplug:
    clients:
        my_service:
            service: 'httplug.clients.my_service.mock'

Now it gives this error:

In BaseNode.php line 319:

  Invalid configuration for path "httplug.clients.money_machine": If you want to use the "service" key you ca
  nnot specify "factory" or "config".


In ExprBuilder.php line 189:

  If you want to use the "service" key you cannot specify "factory" or "config".

I think this is because it first loads httplug.yaml and then merges httplug_test.yaml on top of it. Resulting in the following configuration:

services:
    httplug.clients.my_service.mock:
        class: Http\Mock\Client

httplug:
    clients:
        my_service:
            factory: 'httplug.factory.guzzle5'
            service: 'httplug.clients.my_service.mock'
            config:
                defaults:
                    connect_timeout: 3
                    timeout: 3

So now the only way for me to solve this, is by removing the factory stuff from httplug.yaml and define the services upfront. Now imagine I have 15 clients defined. This is going to be a mess.

[ruudk]

Doing it like this seems to work, but still very ugly:

services:
    httplug.clients.my_service.mock:
        class: Http\Mock\Client
    httplug.clients.my_service.mock_factory:
        class: Http\HttplugBundle\ClientFactory\MockFactory
        calls:
            - ['setClient', ['@httplug.clients.my_service.mock']]

    httplug.clients.my_service_read.mock:
        class: Http\Mock\Client
    httplug.clients.my_service_read.mock_factory:
        class: Http\HttplugBundle\ClientFactory\MockFactory
        calls:
            - ['setClient', ['@httplug.clients.my_service_read.mock']]


httplug:
    clients:
        my_service:
            factory: 'httplug.clients.my_service.mock_factory'
            public: true
        my_service_read:
            factory: 'httplug.clients.my_service_read.mock_factory'
            public: true

[dbu]

can you configure the "real" services in httplug_prod.yaml only, and the mock ones in httplug_test.yaml?

[ruudk]

You mean:

// httplug_prod.yaml
http:
  clients:
    my_service:
      factory: 'guzzleblala'
      // options here

// httplug_dev.yaml
http:
  clients:
    my_service:
      factory: 'guzzleblala'
      // options here

// httplug_test.yaml
http:
  clients:
    my_service:
      factory: 'mock'
      // options here

?

[dbu]

hm, yeah i guess. maybe you can make httplug_shared.yaml to include that from httplug_dev and _prod to not duplicate there?

[ruudk]

Feels really complicated. Maybe I can make a PR to introduce a unique_mock_factory? Don't really get the 1 shared mock client for every http client approach here.

[dbu]

yeah, its not elegant. i think its better to have a separate class and service rather than make the existing "configurable" for both scenarios. i think the pull request makes sense, please do one.

[ruudk]

I'm trying hard to come up with a name for this Mock Factory. But the only thing that is right, is MockFactory. The thing is that the existing factory is more like a Singleton. It creates one instance of a client, and keeps returning that one. Any suggestions?

[dbu]

hm, what about adding a method to the existing factory , called createNewClient? ideally we would have called what we have getClient and call the new thing createClient but that would be some of the worst BC breaks as its really hard to detect the mistake.