-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
misc(provider): Change User-Agent header signature #2964
misc(provider): Change User-Agent header signature #2964
Conversation
Thanks @BoboTiG! I like this change, but it's (arguably) breaking so I'd prefer to wait until v7 to pull it in. I added it to our v7 issue. |
27cfc0d
to
4c72874
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.
I think the change here is a good one. I made some comments to clean up the tests around this. @reedsa, it looks like you requested a review here. Did you want to take care of those if you agree with them and merge? Otherwise I'm good with this. Just seems like a more explicit test makes sense here since we are trying to get a very specific outcome out of the user-agent field.
""" | ||
provider = AsyncHTTPProvider() | ||
headers = provider.get_request_headers() | ||
assert "<" not in headers["User-Agent"] |
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.
These are kinda clunky.
-
We aren't testing
construct_user_agent()
. We are maybe indirectly "testing" it but there's no way to prove thatget_request_headers()
is actually calling it. So let's rename the test totest_get_request_headers()
. -
Since we are now testing the request headers method, let's test both fields that should be there. We are certain of what the value should be for the content-type, so let's validate that logic entirely.
-
The docstring can go here. I think the test name along with the logic inside it is pretty clear of what this is testing for and if it's not it should be changed to reflect it.
from web3 import (
__version__ as web3_version,
)
...
def test_get_request_headers():
provider = AsyncHTTPProvider()
headers = provider.get_request_headers()
assert len(headers) == 2
assert headers["Content-Type"] == "application/json"
assert (
headers["User-Agent"] == f"web3.py/{web3py_version}/"
f"{AsyncHTTPProvider.__module__}.{AsyncHTTPProvider.__qualname__}"
)
@@ -103,3 +103,20 @@ def test_user_provided_session(): | |||
assert isinstance(adapter, HTTPAdapter) | |||
assert adapter._pool_connections == 20 | |||
assert adapter._pool_maxsize == 20 | |||
|
|||
|
|||
def test_construct_user_agent(): |
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 should do the same with this test. See previous comment.
The User-Agent used to include a stringiffied class name like: "web3.py/6.4.0/<class 'web3.providers.rpc.HTTPProvider'>" "web3.py/6.4.0/<class 'web3.providers.async_rpc.AsyncHTTPProvider'>" This patch sligthly changes it to: "web3.py/6.4.0/web3.providers.rpc.HTTPProvider" "web3.py/6.4.0/web3.providers.async_rpc.AsyncHTTPProvider"
5cb5533
to
751808b
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.
looks good!
What was wrong?
The User-Agent used to include a stringiffied class name like:
This patch sligthly changes it to:
How was it fixed?
Use
type.__module__
, andtype.__qualname__
, rather thenstr(type))
.Todo:
Cute Animal Picture