Calls info in BaseResponse #664

zeezdev · 2023-08-17T21:32:51Z

Sometimes it is more convenient to get information about the calls made directly from the mockend request, instead of calculating the order of your calls in the global list (responses.calls[n]).

What do you think of such a possibility?

…lList

codecov · 2023-08-22T20:35:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
responses/__init__.py	`100.00% <100.00%> (ø)`
responses/tests/test_responses.py	`100.00% <100.00%> (ø)`

📢 Thoughts on this report? Let us know!.

markstory · 2023-08-22T20:44:33Z

responses/__init__.py

                self._calls.add(request, response)
+                match.calls.add_call(self._calls[next_index])


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

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)

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.

Looks good, thanks, Let me try...

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.

… into README

beliaev-maksim

also, CHANGELOG entry is missing

README.rst

beliaev-maksim · 2023-09-22T23:23:04Z

README.rst

+        assert len(responses.calls) == 3  # total calls count
+        # Assert calls to the 1st resource ("http://www.example.com"):
+        assert rsp.call_count == 2
+        assert rsp.calls[0] is responses.calls[0]
+        assert rsp.calls[1] is responses.calls[1]
+        request = rsp.calls[0].request
+        assert request.url == "http://www.example.com/"
+        assert request.method == "GET"
+        request = rsp.calls[1].request
+        assert request.url == "http://www.example.com/?hello=world"
+        assert request.method == "GET"
+        # Assert calls to the 2nd resource (http://foo.bar):
+        assert rsp2.call_count == 1
+        assert rsp2.calls[0] is responses.calls[2]
+        request = rsp2.calls[0].request
+        assert request.url == "http://foo.bar/42/"
+        assert request.method == "PUT"
+        request_payload = json.loads(request.body)
+        assert request_payload == {"name": "Bazz"}


this could be sparse to increase readability

The example has been replaced. Is this still relevant?

Co-authored-by: Maksim Beliaev <beliaev.m.s@gmail.com>

zeezdev · 2023-09-26T21:06:36Z

also, CHANGELOG entry is missing

Added the CHANGELOG entry.

beliaev-maksim

LGTM

markstory · 2023-10-20T06:00:43Z

This is looking good, a few small formatting issues in the README that need to be fixed up before this can be merged.

zeezdev · 2023-10-20T08:11:51Z

This is looking good, a few small formatting issues in the README that need to be fixed up before this can be merged.

Fixed.

zeezdev · 2023-10-20T21:23:50Z

It looks like mypy problems is related to v1.6.1 that released several days ago.

I added several ignore comments related to my changes and old code. Please review again.

beliaev-maksim · 2023-10-20T21:39:24Z

responses/matchers.py

@@ -13,7 +13,7 @@
 from urllib.parse import urlparse

 from requests import PreparedRequest
-from requests.packages.urllib3.util.url import parse_url
+from requests.packages.urllib3.util.url import parse_url  # type: ignore[import-untyped]


why is that ?

beliaev-maksim · 2023-10-20T21:40:09Z

responses/__init__.py

@@ -252,11 +258,11 @@ def __len__(self) -> int:
        return len(self._calls)

    @overload
-    def __getitem__(self, idx: int) -> Call:
+    def __getitem__(self, idx: int) -> Call:  # type: ignore[misc]


too many ignores, something looks to be incorrect

fixed in #684

first merge #684, then current PR and we are good to ship the release

Merged, thanks!

# Conflicts: # responses/__init__.py # responses/matchers.py

beliaev-maksim · 2023-10-23T07:29:01Z

responses/tests/test_responses.py

+        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


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

make sense. Made tests readable. Please take a look

beliaev-maksim · 2023-10-23T07:29:42Z

responses/tests/test_responses.py

+        request = rsp2.calls[0].request
+        assert request.url == "http://www.foo.bar/42/"
+        assert request.method == "PUT"
+        request_payload = json.loads(request.body)


what exactly do we test here ?

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

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

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

responses/responses/__init__.py

Line 1041 in 5c6a3b1

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

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

@beliaev-maksim and what about this question?

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

beliaev-maksim · 2023-10-23T07:31:04Z

responses/tests/test_responses.py

+        assert response.content == b"test"
+        assert response.status_code == 200
+        assert rsp2.call_count == 1
+        response = rsp2.calls[0].response


same here, could be enumerated. Something like

Suggested change

response = rsp2.calls[0].response

response2 = rsp2.calls[0].response

agree, done

beliaev-maksim

I think we are very close to the conclusion

technically all looks good, just need to simplify user facing docs

beliaev-maksim · 2023-10-23T18:09:19Z

README.rst

+
+
+    @responses.activate
+    def test_assert_calls_on_resp():


I understand that this example might look very close to what you have in production. But can we unload it a bit and reduce complexity?

we probably still want to show an example of concurrent since this could be one of the main usages, but we do not need all those complicated matrices of settings to show the usage

can you please simplify it to remove additional arguments and have more clear names like
rsp1, rsp2, rsp3

Got you, I'll try to simplify this.

simplified, please take a look

@zeezdev now looks a way better, thank you!

beliaev-maksim · 2023-10-23T19:49:47Z

README.rst

+            for future in concurrent.futures.as_completed(future_to_uid):
+                uid = future_to_uid[future]
+                response = future.result()
+                print("%s updated with %d status code" % (uid, response.status_code))


Can you please update it with f-string

beliaev-maksim · 2023-10-23T19:50:44Z

README.rst

+
+
+    @responses.activate
+    def test_assert_calls_on_resp():


@zeezdev now looks a way better, thank you!

beliaev-maksim · 2023-10-23T20:22:23Z

@markstory can you please do a final pass here

beliaev-maksim · 2023-10-24T07:06:23Z

responses/tests/test_responses.py

+        rsp2 = responses.add(responses.GET, "http://www.example.com/1")
+        rsp3 = responses.add(
+            responses.GET, "http://www.example.com/2"
+        )  # won't be requested


that comment is probably true only with the default registry, or?

I mean, this mocked resource (http://www.example.com/2) isn't requested during the test to explain why we get empty rsp3.calls.
I can remove this if it confuses.

that is fine to keep it for the testing purpose.

I just mean that if the user applies a different registry or we switch the default registry (which is unlikely), then this comment will not be correct

https://github.com/getsentry/responses#ordered-registry

but this comment relates to the specific scope of the test. And is it relevant in this case?

If I were to initialize the test with
@responses.activate(registry=OrderedRegistry)

then it would be a different case with different expectations, wouldn’t it?

that is more of a note rather than a call for an action :)

I am +1 with the current state of PR

now wait for @markstory to make his round of review

beliaev-maksim · 2023-10-30T17:45:29Z

@zeezdev great work, thanks!

zeezdev added 2 commits August 15, 2023 22:37

Add calls to BaseResponse

3f7660a

Reuse calls for matched BaseResponse and add add_call method into Cal…

77ca597

…lList

zeezdev marked this pull request as ready for review August 17, 2023 21:33

markstory reviewed Aug 22, 2023

View reviewed changes

zeezdev added 2 commits August 24, 2023 23:13

Merge branch 'getsentry:master' into call-info-in-response

0d29ad1

Reuse call object with add_call & add example of usage Response.calls…

5669688

… into README

beliaev-maksim suggested changes Sep 22, 2023

View reviewed changes

zeezdev and others added 4 commits September 26, 2023 20:09

Merge branch 'getsentry:master' into call-info-in-response

34d825f

Update README.rst

7f7085e

Co-authored-by: Maksim Beliaev <beliaev.m.s@gmail.com>

Add CHANGELOG entry

0e515ff

Change example in README

d78499d

beliaev-maksim approved these changes Sep 27, 2023

View reviewed changes

beliaev-maksim requested a review from markstory September 27, 2023 20:47

Merge branch 'master' into call-info-in-response

48f9615

beliaev-maksim mentioned this pull request Oct 12, 2023

No access to request query string, body, etc. on mock object #680

Closed

beliaev-maksim mentioned this pull request Oct 19, 2023

slice the release #681

Closed

Fix for blacken-docs linter

aa29301

zeezdev added 2 commits October 20, 2023 23:45

Add ignore comment for mypy

d1b0650

Add ignore comments for mypy

f6a7dd3

beliaev-maksim reviewed Oct 20, 2023

View reviewed changes

Merge branch 'master' into call-info-in-response

9f5e733

# Conflicts: # responses/__init__.py # responses/matchers.py

beliaev-maksim reviewed Oct 23, 2023

View reviewed changes

Make tests readable

654c607

beliaev-maksim suggested changes Oct 23, 2023

View reviewed changes

Simplify readme example

41bb479

beliaev-maksim approved these changes Oct 23, 2023

View reviewed changes

Use f-string in readme

51f4e56

beliaev-maksim added enhancement Waiting for: Product Owner labels Oct 23, 2023

azaslavsky removed the Waiting for: Product Owner label Oct 23, 2023

Simplify new tests

ea1144d

beliaev-maksim reviewed Oct 24, 2023

View reviewed changes

beliaev-maksim mentioned this pull request Oct 25, 2023

Response count not reset when defining Response outside of test function #687

Closed

markstory merged commit bcad0cd into getsentry:master Oct 30, 2023
12 checks passed

		self._calls.add(request, response)
		match.calls.add_call(self._calls[next_index])

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

Calls info in BaseResponse #664

Calls info in BaseResponse #664

Conversation

zeezdev commented Aug 17, 2023

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliaev-maksim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeezdev commented Sep 26, 2023

beliaev-maksim left a comment

Choose a reason for hiding this comment

markstory commented Oct 20, 2023

zeezdev commented Oct 20, 2023

zeezdev commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeezdev Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliaev-maksim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliaev-maksim commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliaev-maksim commented Oct 30, 2023

codecov bot commented Aug 22, 2023 •

edited

Loading

zeezdev Oct 23, 2023 •

edited

Loading