Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Calls info in BaseResponse #664
Changes from 4 commits
3f7660a
77ca597
0d29ad1
5669688
34d825f
7f7085e
0e515ff
d78499d
48f9615
aa29301
d1b0650
f6a7dd3
9f5e733
654c607
41bb479
51f4e56
ea1144d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 usagecan you please simplify it to remove additional arguments and have more clear names like
rsp1, rsp2, rsp3
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.
Got you, I'll try to simplify this.
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.
simplified, please take a look
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.
@zeezdev now looks a way better, thank you!
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.
this could be sparse to increase readability
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.
The example has been replaced. Is this still relevant?
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.
I think we can make it a bit more readable by enumerating
request
objects as welland so on
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.
make sense. Made tests readable. Please take a look
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.
what exactly do we test here ?
why do we need to test that request body could be converted?
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.
(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 payloadrequest_payload = {"name": "Bazz"}
, that was used on the put request.PreparedRequest
contains only encoded data in thebody
field, so we decode it to compare with the original data -request_payload
.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.
correct, but do we need to check it at all as part of
responses
?we attach
request
object as is without modificationresponses/responses/__init__.py
Line 1041 in 5c6a3b1
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
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 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: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.
@beliaev-maksim and what about this question?
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.
@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.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.
same here, could be enumerated. Something like
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.
agree, done