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

Calls info in BaseResponse #664

Merged
merged 17 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions responses/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@ def __getitem__(self, idx: Union[int, slice]) -> Union[Call, List[Call]]:
def add(self, request: "PreparedRequest", response: _Body) -> None:
self._calls.append(Call(request, response))

def add_call(self, call: Call) -> None:
self._calls.append(call)

def reset(self) -> None:
self._calls = []

Expand Down Expand Up @@ -394,7 +397,7 @@ def __init__(
)

self.match: "_MatcherIterable" = match
self.call_count: int = 0
self._calls: CallList = CallList()
self.passthrough = passthrough

def __eq__(self, other: Any) -> bool:
Expand Down Expand Up @@ -503,6 +506,14 @@ def matches(self, request: "PreparedRequest") -> Tuple[bool, str]:

return True, ""

@property
def call_count(self) -> int:
return len(self._calls)

@property
def calls(self) -> CallList:
return self._calls


def _form_response(
body: Union[BufferedReader, BytesIO],
Expand Down Expand Up @@ -1064,14 +1075,16 @@ def _on_request(
request, match.get_response(request)
)
except BaseException as response:
match.call_count += 1
next_index = len(self._calls)
self._calls.add(request, response)
match.calls.add_call(self._calls[next_index])
Copy link
Member

Choose a reason for hiding this comment

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

If you have the request and response, why do you need to read out of self._calls? Wouldn't match.calls.add() work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that's what I did initially. But then an idea came up:

  • reuse an already created call from the "global" list self._calls
  • have the same call instance in the global list and in the individual response list (match.calls)

(using match.calls.add() we'll create a separate instance of the same call)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, wanting to reuse the objects seems reasonable to me. What do you think of something like

call = Call(request, response)
self._calls.add_call(call)
match.calls.add_call(call)

That would save us having to index/count call objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, thanks, Let me try...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced the index with

        call = Call(request, response)
        self._calls.add_call(call)
        match.calls.add_call(call)

and added an example of usage Response.calls into the README.

raise

if resp_callback:
response = resp_callback(response) # type: ignore[misc]
match.call_count += 1
next_index = len(self._calls)
self._calls.add(request, response) # type: ignore[misc]
match.calls.add_call(self._calls[next_index])

retries = retries or adapter.max_retries
# first validate that current request is eligible to be retried.
Expand Down
83 changes: 83 additions & 0 deletions responses/tests/test_responses.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# coding: utf-8

import inspect
import json
import os
import re
import warnings
Expand Down Expand Up @@ -1988,6 +1989,88 @@ def run():
assert_reset()


def test_response_and_requests_mock_calls_are_equal():
@responses.activate
def run():
rsp = responses.add(responses.GET, "http://www.example.com")
rsp2 = responses.add(responses.GET, "http://www.example.com/1")

requests.get("http://www.example.com")
requests.get("http://www.example.com/1")

assert len(responses.calls) == 2
assert rsp.call_count == 1
assert rsp.calls[0] is responses.calls[0]
assert rsp2.call_count == 1
assert rsp2.calls[0] is responses.calls[1]

run()
assert_reset()


def test_response_call_request():
@responses.activate
def run():
rsp = responses.add(responses.GET, "http://www.example.com")
rsp2 = responses.add(
responses.PUT, "http://www.foo.bar/42/", json={"id": 42, "name": "Bazz"}
)

requests.get("http://www.example.com")
requests.get("http://www.example.com?hello=world")
requests.put(
"http://www.foo.bar/42/",
json={"name": "Bazz"},
)

assert rsp.call_count == 2
request = rsp.calls[0].request
assert request.url == "http://www.example.com/"
assert request.method == "GET"
request = rsp.calls[1].request
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 we can make it a bit more readable by enumerating request objects as well

Suggested change
assert rsp.call_count == 2
request = rsp.calls[0].request
assert request.url == "http://www.example.com/"
assert request.method == "GET"
request = rsp.calls[1].request
assert rsp.call_count == 2
request1 = rsp.calls[0].request
assert request.url == "http://www.example.com/"
assert request.method == "GET"
request2 = rsp.calls[1].request

and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense. Made tests readable. Please take a look

assert request.url == "http://www.example.com/?hello=world"
assert request.method == "GET"
assert rsp2.call_count == 1
request = rsp2.calls[0].request
assert request.url == "http://www.foo.bar/42/"
assert request.method == "PUT"
request_payload = json.loads(request.body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what exactly do we test here ?

why do we need to test that request body could be converted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I changed the test a little)

Here we validate that request rsp2_request1 = rsp2.calls[0].request obtained from the mock's call contains expected payload request_payload = {"name": "Bazz"}, that was used on the put request.

request body could be converted?

PreparedRequest contains only encoded data in the body field, so we decode it to compare with the original data - request_payload.

Copy link
Collaborator

Choose a reason for hiding this comment

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

PreparedRequest contains only encoded data in the body field, so we decode it to compare with the original data - request_payload.

correct, but do we need to check it at all as part of responses ?

we attach request object as is without modification

response.request = request

and in the case of this PR we can skip this check since it does not add a value.

although, you can fire a parallel PR that can add a separate test to validate that we have all the required data is attached to a request
we still will need to discuss if that is required or not but this we can do in a separate thread

Copy link
Contributor Author

@zeezdev zeezdev Oct 23, 2023

Choose a reason for hiding this comment

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

we attach request object as is without modification

You are right, I missed this moment.

So, in other words, in the current PR it is enough to test that calls of local mocks are identical to calls of the global list without deep validation of rsp.calls[N].request & rsp.calls[N].response?
I.e. merge all new tests (test_response_and_requests_mock_calls_are_equal, test_response_call_request, test_response_call_response) to something like this:

def test_response_calls_and_registry_calls_are_equal():
    @responses.activate
    def run():
        rsp1 = responses.add(responses.GET, "http://www.example.com")
        rsp2 = responses.add(responses.GET, "http://www.example.com/1")

        requests.get("http://www.example.com")
        requests.get("http://www.example.com/1")
        requests.get("http://www.example.com")

        assert len(responses.calls) == len(rsp1.calls) + len(rsp2.calls)
        assert rsp1.call_count == 2
        assert len(rsp1.calls) == 2
        assert rsp1.calls[0] is responses.calls[0]
        assert rsp1.calls[1] is responses.calls[2]
        assert rsp2.call_count == 1
        assert len(rsp2.calls) == 1
        assert rsp2.calls[0] is responses.calls[1]

    run()
    assert_reset()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@beliaev-maksim and what about this question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zeezdev sorry, missed this one in a long thread. Yes.
And if for some reason we have missed the tests that ensure the quality of Call objects. Then I would recommend to create separate tests for those and open it in a separate PR.

assert request_payload == {"name": "Bazz"}

run()
assert_reset()


def test_response_call_response():
@responses.activate
def run():
rsp = responses.add(responses.GET, "http://www.example.com", body=b"test")
rsp2 = responses.add(
responses.POST,
"http://www.foo.bar/42/",
json={"id": 42, "name": "Bazz"},
status=201,
)

requests.get("http://www.example.com")
requests.post(
"http://www.foo.bar/42/",
json={"name": "Bazz"},
)

assert rsp.call_count == 1
response = rsp.calls[0].response
assert response.content == b"test"
assert response.status_code == 200
assert rsp2.call_count == 1
response = rsp2.calls[0].response
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, could be enumerated. Something like

Suggested change
response = rsp2.calls[0].response
response2 = rsp2.calls[0].response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, done

assert response.json() == {"id": 42, "name": "Bazz"}
assert response.status_code == 201

run()
assert_reset()


def test_fail_request_error():
"""
Validate that exception is raised if request URL/Method/kwargs don't match
Expand Down