-
Notifications
You must be signed in to change notification settings - Fork 46
Add Mock Client strategy. #90
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. Make sure to create a PR to the docs explaining this strategy, why, how and that it is not loaded by default.
* Find the Mock client. | ||
* | ||
* @internal | ||
* @final |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for final annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/** | ||
* Find the Mock client. | ||
* | ||
* @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this strategy really internal? The others are, but it is meant that this class should be used by others, right?
*/ | ||
public static function getCandidates($type) | ||
{ | ||
return ($type === 'Http\Client\HttpClient') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the ::class constant here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, changing that now.
@samrap thanks for your contribution. What do you think about #89 (comment) ? |
@sagikazarmark I prefer sticking with a manual setup in the test class i.e: public function setUp()
{
HttpClientDiscovery::prependStrategy(MockClientStrategy::class);
} Adding a trait or method(s) to the Mock Client to add itself to the Discovery means the strategy now depends on the Discovery which seems like an unnecessary dependency to me. Unless I'm understanding this wrong. As far as @barryvdh suggestion for |
I rather meant a trait or base class for PHPUnit test cases, but I realized that MockClient is not limited to be used in PHPUnit, so that's probably not really good idea. |
I do not like Discovery::enableMockClient. No discovery strategy should be treated in special manners. |
@Nyholm good point. So I think we should keep this as is, and I will work on getting those docs completed. |
Just FYI still active on this, been busy at work but will get these docs by end of week. |
Thank you for this PR. |
* Add documentation for the MockClientStrategy See [#90](php-http/discovery#90). * Update discovery.rst * Break lines at 80 characters.
Thank you guys! |
What's in this PR?
Please see #89 for a full description of this PR.
Essentially, we need a way to test DI within our tests which in many cases cannot be fully implemented without having some way to discover the Mock Client. This new class allows us to add the
MockClientStrategy
to theHttpClientDiscovery
's strategies as shown in @Nyholm 's example in the above mentioned PR.Example Usage
Checklist