Skip to content
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.

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

[HttpClient] Add a Record & Replay callback to the MockHttpClient. #35677

Closed
wants to merge 4 commits into from

Conversation

GaryPEGEOT
Copy link
Contributor

@GaryPEGEOT GaryPEGEOT commented Feb 11, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets partially #30502
License MIT
Doc PR TODO

Allow to record & replay responses for test / dev purpose:

<?php

/* @var Psr\Cache\CacheItemPoolInterface $cache */
$cache = new ArrayAdapter();
$callback = new RecordReplayCallback(RecordReplayCallback::MODE_REPLAY_OR_RECORD, $cache)

$mockClient = new MockHttpClient($callback);

// Will make an actual HTTP request
$client->request('POST', 'https://example.org/whatever');

// Will replay the previous response
$client->request('POST', 'https://example.org/whatever');

TODO:

  • Create the PR for the documentation
  • Integrate with Framework bundle for easy config

@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

@GaryPEGEOT Do you have time to finish this PR?

@GaryPEGEOT
Copy link
Contributor Author

Hi @fabpot! I would need some feedback on the callback before creating the Framework integration. PR #35893 gives the ability to integrate the callback easily.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

What about making this way simpler, by just recording + replaying the req/resp in order?

Right now, the hashing logic makes me think of this as a special cache layer, not a record-replay tape.

I feel like we might prefer implementing a real HTTP cache and make it able to force-store+serve requests, and have this one specialized on in-order serving.

WDYT?

*/
private $client;

public function __construct(string $mode, CacheItemPoolInterface $cache, HttpClientInterface $client = null)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make the mode optional, defaulting to MODE_REPLAY_OR_RECORD?
That would ease integration with tests I believe: one wants to re-record? => bin/console cache:pool:clear some_pool_for_some_reqs then run the tests again.
WDYT?

$response = null;

if (isset($options['body'])) {
$parts[] = md5(static::getBodyAsString($options['body']));
Copy link
Member

Choose a reason for hiding this comment

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

better not use md5 for this.
we could use hash_init() instead of the $parts array

@nicolas-grekas
Copy link
Member

For inspiration btw: https://github.com/ikwattro/guzzle-stereo/

@fabpot
Copy link
Member

fabpot commented Aug 27, 2020

#35893 has been merged now.

@GaryPEGEOT GaryPEGEOT force-pushed the feature/http-rnr branch 2 times, most recently from 92aff83 to 2c57469 Compare September 6, 2020 09:29
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks this moves forward nicely.
One thing that doesn't work right now is async: the way this is currently implemented break async and streaming when recording.
It should be possible to record without breaking async by leveraging AsyncDecorator.
Up to continue this way?


protected function setUp(): void
{
$recorder = new ResponseRecorder(sys_get_temp_dir(), new ResponseSerializer());
Copy link
Member

Choose a reason for hiding this comment

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

we should clean up the temporary files created when running the tests

$this->client = new MockHttpClient($this->callback);
}

public function testReplayOrRecord(): void
Copy link
Member

Choose a reason for hiding this comment

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

please remove all : void from all test cases

private $client;

/**
* @var ResponseRecorder
Copy link
Member

Choose a reason for hiding this comment

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

let's remove all @var that duplicate the constructor, they're useless

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2020

Closing as we discussed on Slack: we should try another approach based on AsyncDecoratorTrait, that would share some logic/classes between this use case and the "client-side http-cache" use case.

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@rrajkomar
Copy link

@nicolas-grekas Has there been any progress or new leads on this feature ?
It would be awesome if we could get a working record/replay system for the HttpClient as currently there does not seem to be any viable alternative (aside from configuring a custom callback which would have to be duplicated accross projets, not ideal).
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants