Skip to content

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Jan 26, 2025

Fixes #210

@swissspidy swissspidy requested a review from a team as a code owner January 26, 2025 20:24
@swissspidy swissspidy mentioned this pull request Jan 26, 2025
1 task
@swissspidy
Copy link
Member Author

@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.

@swissspidy
Copy link
Member Author

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 :)

@swissspidy swissspidy added this to the 4.3.7 milestone Jan 29, 2025
@swissspidy
Copy link
Member Author

@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.

@mrsdizzie
Copy link
Member

Looks good to me if you think is ready to merge

@swissspidy
Copy link
Member Author

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.

@mrsdizzie
Copy link
Member

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:

When any request is made to $url
Then the response will always be:

But that doesn't really fit into how "When" is used otherwise in our tests, so maybe it creates a different type of confusion.

@swissspidy
Copy link
Member Author

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 When or Then or Given when doing regex matching. There are semantic differences though:

The purpose of Given steps is to put the system in a known state before the user (or external system) starts interacting with the system (in the When steps). Avoid talking about user interaction in givens. If you have worked with use cases, givens are your preconditions.

The purpose of When steps is to describe the key action the user performs

The purpose of Then steps is to observe outcomes. The observations should be related to the business value/benefit in your feature description. The observations should inspect the output of the system (a report, user interface, message, command output) and not something deeply buried inside it (that has no business value and is instead part of the implementation).

  • Verify that something related to the Given+When is (or is not) in the output
  • Check that some external system has received the expected message (was an email with specific content successfully sent?)

@mrsdizzie
Copy link
Member

That makes sense. Maybe something pretty similar to your original like

 Given any HTTP requests to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 will respond with:
    """
    HTTP/1.1 200
    Content-Type: application/json
    ...
    """

or

 Given HTTP requests to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 will always respond with:
    """
    HTTP/1.1 200
    Content-Type: application/json
    ...
    """

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:

 Given any HTTP requests to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 will respond with this mock data:
    """
    HTTP/1.1 200
    Content-Type: application/json
    ...
    """

@schlessera
Copy link
Member

How about something like this:

 Given that HTTP requests to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 will respond with:
    """
    HTTP/1.1 200
    Content-Type: application/json
    ...
    """

It reads more natural to me...

@mrsdizzie
Copy link
Member

How about something like this:

 Given that HTTP requests to https://api.github.com/repos/wp-cli/wp-cli/releases?per_page=100 will respond with:
    """
    HTTP/1.1 200
    Content-Type: application/json
    ...
    """

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.

@swissspidy
Copy link
Member Author

Sounds good!

@swissspidy swissspidy merged commit 892c5d7 into main Feb 13, 2025
46 of 47 checks passed
@swissspidy swissspidy deleted the try/210-http-mocking branch February 13, 2025 15:25
@swissspidy
Copy link
Member Author

Now I just need a review on #236 to fix those tests :)

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.

Mock HTTP requests
3 participants