search

cdent / gabbi

Declarative HTTP Testing for Python and anything else
http://gabbi.readthedocs.org/
Other
148 stars 34 forks source link

urllib>=1.24 is required to use server_hostname #307

Closed kajinamit closed 2 years ago

kajinamit commented 2 years ago

https://github.com/cdent/gabbi/pull/306 introduced usage of the server_hostname argument but this is available only in urllib>=1.24 [1].

[1] https://github.com/urllib3/urllib3/commit/3f2267a2cbe0619ee42249aa13a98a53edbcf07c

We should bump the version in requirements.txt to make sure that the argument is available. Otherwise test run fails with the following error.

TypeError: __init__() got an unexpected keyword argument 'server_hostname'
kajinamit commented 2 years ago

Hmm... I was looking into the actual error but it seems the TypeError exception is raised not by HTTPSConnection but by HTTPConnection... I've reported that issue in #309

cdent commented 2 years ago

This and #309 make pretty clear that the changes in 2.0.5 were incomplete and need some better testing.

What I suggest we do is I first implement those tests to see where things fail and then, once any changes in gabbi itself are made, we see where we need to pin versions.

I suspect the main problem here is that server_hostname is being sent when ssl is not being used, which is confusing the underlying libraries.

So I need to add some tests which set the host header in various scenarios.

I'll do this today.

kajinamit commented 2 years ago

After digging into the issue further, we've identified the root issue with urllib3 which I explained in https://github.com/urllib3/urllib3/issues/2534 . There is a proposed patch https://github.com/urllib3/urllib3/pull/2537 which will solve the issue, so we should bump urllib3 once the fixed version is released. Because the problem is caused by the way urllib3.PoolManager instantiate a connection pool, I'm not sure how much we can do from gabbi side.

I totally agree we definitely need a better testing to catch this kind of issue if possible.

cdent commented 2 years ago

I've got a patch in progress which adds additional tests that fail without urllib3/urllib3#2537 but pass with it. Will push that up for review in a moment. Thanks for digging into this.