-
Notifications
You must be signed in to change notification settings - Fork 12
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
Remove mocket #148
Remove mocket #148
Conversation
8c0d126
to
dcb884d
Compare
dcb884d
to
19a37fe
Compare
tests/test_webservice.py
Outdated
last = None | ||
|
||
def custom_handler(r): | ||
nonlocal last | ||
last = r | ||
return '{"status": 204}' |
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 library seems to have a better way for querying the request data but it is not yet available in the latest release.
d00a378
to
064bd76
Compare
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 seems to be missing
tests/test_webservice.py
Outdated
content_type=f"application/vnd.maxmind.com-minfraud-{self.type}+json; charset=UTF-8; version=2.0", | ||
last = None | ||
|
||
def custom_handler(r): |
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 this should be updated similar to how the geoip2
one was updated.
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 wrote a comment before about why this is still using custom_handler. #148 (comment)
This specific test is checking the data of the request not the headers . I found this doc about how to do that RequestMatcherhttps://pytest-httpserver.readthedocs.io/en/latest/howto.html#querying-the-log
However the class used for doing this "RequestMatcher" is not available on the release 1.0.10 (which is the latest release I installed from pip) but from master.
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.
Ah, I am sorry, I didn't realize that comment was referring to this. Is there a reason why self.httpserver.expect_request(uri, method="POST", json=request)...
doesn't work?
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 it can work but we have to modify the assertion. How do you suggest to do that?
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 am not sure I fully understand. I would expect something like:
def test_200_with_email_hashing(self):
uri = "/".join(["/minfraud/v2.0", self.type])
self.httpserver.expect_request(
uri,
method="POST",
json={
"email": {
"address": "977577b140bfb7c516e4746204fbdb01",
"domain": "maxmind.com",
}
},
).respond_with_data(self.response)
request = {"email": {"address": "[email protected]"}}
self.run_client(getattr(self.client, self.type)(request, hash_email=True))
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 test has an assertion a few lines below. Can we remove if we go with what you suggested?
self.assertEqual(
{
"email": {
"address": "977577b140bfb7c516e4746204fbdb01",
"domain": "maxmind.com",
}
},
json.loads(httpretty.last_request.body),
)
`
```
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.
Right, my suggested code moved that to the expected_request
. If the request does not match that expectation, the server will return a 500 and the test will fail.
self.client._score_uri = self.httpserver.url_for("/minfraud/v2.0/score") | ||
self.client._report_uri = self.httpserver.url_for( | ||
"/minfraud/v2.0/transactions/report" | ||
) |
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.
We shouldn't be modifying many private attributes on the client class. I probably should have raised this on the geoip2
one, but there it was just one. Beyond modifying the internal state of the object in a confusing way, this bypasses any testing of the constructor.
I think it would be better to do something more similar to this and then passing the host and port through using the host
paramter.
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.
how do you want the scheme (i.e. http or https) to be set for the client? pytest_httpserver runs on http, to run it as https we need another dependancy as described in this doc: https://pytest-httpserver.readthedocs.io/en/latest/howto.html#running-an-https-server
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.
See the link in my comment for how we set the scheme temporarily when there was an issue with mocket
. I think we can just do that again. I don't think we want to make it part of the public API.
tests/test_webservice.py
Outdated
self.client._report_uri = self.httpserver.url_for( | ||
"/minfraud/v2.0/transactions/report" | ||
) | ||
self.base_uri = self.client._base_uri |
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.
Are we using this anywhere?
No description provided.