-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
Changes from all 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2035,6 +2035,36 @@ def run(): | |
assert_reset() | ||
|
||
|
||
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") | ||
rsp3 = responses.add( | ||
responses.GET, "http://www.example.com/2" | ||
) # won't be requested | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that comment is probably true only with the default registry, or? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, this mocked resource ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 then it would be a different case with different expectations, wouldn’t it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
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) + len( | ||
rsp3.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] | ||
assert rsp3.call_count == 0 | ||
assert len(rsp3.calls) == 0 | ||
|
||
run() | ||
assert_reset() | ||
|
||
|
||
def test_fail_request_error(): | ||
""" | ||
Validate that exception is raised if request URL/Method/kwargs don't match | ||
|
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!