Skip to content

Add mock client factory for test purpose. #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jan 9, 2018
Merged

Add mock client factory for test purpose. #242

merged 6 commits into from
Jan 9, 2018

Conversation

GaryPEGEOT
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes partially #212
Documentation php-http/documentation#220
License MIT

What's in this PR?

Add a mock client factory to ease functional testing.

Example Usage

Configuration:

    # config_test.yml
    httplug:
        clients:
            my_awesome_client:
                factory: 'httplug.factory.mock' # replace factory

Usage:

$client = static::createClient();
// $client->disableReboot(); You might uncomment this if your client (BrowserKit) make multiple requests as kernel is rebooted on each request.

$response = $this->createMock('Psr\Http\Message\ResponseInterface');
$response->method('getBody')->willReturn(/* Psr\Http\Message\Interface instance containing expected response content. */);
$client->getContainer()->get('httplug.client.mock')->addResponse($response);

dbu
dbu reviewed Jan 4, 2018
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea, thanks!

you could add the mock client to the composer.json suggest section and propose to add it in require-dev of the users project.

ClientFactory/MockFactory.php Outdated
*/
public function createClient(array $config = [])
{
if (!class_exists('Http\Mock\Client')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the ::class pseudoconstant. we are on php 5.5 or newer.

DependencyInjection/HttplugExtension.php Outdated
@@ -106,6 +106,16 @@ private function configureClients(ContainerBuilder $container, array $config)
$container->setAlias('httplug.client.default', 'httplug.client.'.$first);
}
}

$mockClass = 'Http\Mock\Client';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets use the ::class pseudoconstant here. and make the use statement say use Http\Mock\Client as MockClient; to make things more readable.

dbu
dbu reviewed Jan 4, 2018
View reviewed changes
ClientFactory/MockFactory.php Outdated
throw new \LogicException('To use the mock adapter you need to install the "php-http/mock-client" package.');
}

return $this->client ?: new Client();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create a new client on each call, if none has been registered. should we instead register the newly created client if none was set?

Resources/config/services.xml Outdated
@@ -68,5 +68,6 @@
<service id="httplug.factory.socket" class="Http\HttplugBundle\ClientFactory\SocketFactory" public="false">
<argument type="service" id="httplug.message_factory"/>
</service>
<service id="httplug.factory.mock" class="Http\HttplugBundle\ClientFactory\MockFactory" public="false" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this service should be removed from the container when the mock client is not available. that way, users notice problems at container build time instead of at runtime.

dbu
dbu approved these changes Jan 4, 2018
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, looks good to me!

fbourigault
fbourigault reviewed Jan 5, 2018
View reviewed changes
Copy link
Contributor

@fbourigault fbourigault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I wrote a comment to improve the readability.

DependencyInjection/HttplugExtension.php Outdated
$mockServiceId = 'httplug.client.mock';

if (!\class_exists(MockClient::class)) {
$container->removeDefinition('httplug.factory.mock');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing a definition when the class does not exist, could you move the relevant service definition to a dedicated file and load this file if the class is available?

dbu
dbu approved these changes Jan 8, 2018
View reviewed changes
Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks!

@dbu dbu merged commit 7564416 into php-http:master Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbourigault fbourigault fbourigault left review comments

@dbu dbu dbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GaryPEGEOT @dbu @fbourigault