-
Notifications
You must be signed in to change notification settings - Fork 25
Add http request mocking support #235
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
@BrianHenryIE Would love to hear your thoughts on this especially :-) Note:The original suggestion was to store the mocked responses in files and use those, but this can be achieved with this solution just as well. Example: Background:
When I run `cat my/mocked/response.json`
Then save STDOUT as {HTTP_RESPONSE}
Scenario: Mock HTTP request in WP-CLI
Given an empty directory
And an HTTP request to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 with this response:
"""
HTTP/1.1 200
Content-Type: application/json
{HTTP_RESPONSE}
""" I haven't tested that though, but I could add a test for it. |
By the way this has already been successfully tested as part of wp-cli/wp-cli#6037, so I think it's good to go. Appreciate a good review though :) |
@wp-cli/committers This is ready for review :-) It works quite well so far, as evidenced by wp-cli/wp-cli#6037. The API seems intuitive for the existing use cases, but could always be adapted in the future if needed. |
Looks good to me if you think is ready to merge |
Thanks! The main concern @schlessera had about the API is the wording. Usually a "Given ..." step performs a specific action, like installing WordPress. Here, "Given an HTTP request..." doesn't actually perform an HTTP request but merely sets up hooks for mocking requests later on. So we could consider changing this to something like "Given an HTTP mock for requests to xyz with the following response", but not sure if that's better. |
I almost did suggest changing the wording but I'm not sure if what I'd want even fits into the existing way tests are written. What makes sense to me is something more like:
But that doesn't really fit into how "When" is used otherwise in our tests, so maybe it creates a different type of confusion. |
That is a good point. Technically it would be feasible, as Behat doesn't actually differentiate between
|
That makes sense. Maybe something pretty similar to your original like
or
Which feels clear (to me) that it is about future requests and is not in that moment making a request. Using the phrase Mock in the rule text itself feels a bit more awkward and more of an implementation detail. If we really did want to use Mock I think putting it at the end is more clear:
|
How about something like this:
It reads more natural to me... |
Sounds good to me! I think using "that" and "will" in these make it clear that the request isn't happening as part of the test which was the initial concern. |
Sounds good! |
Now I just need a review on #236 to fix those tests :) |
Fixes #210
http_request()
util wp-cli#6036 for mocking requests done by the WP-CLIhttp_request()
util