fix: use urllib3.connection where needed.

[shifqu]

Since urllib3 v2 the re-export of connection.HTTPConnection in urllib3.connectionpool was removed.

In this commit we use urllib3.connection where needed. Some references to connectionpool.HTTPConnection are still there for backward compatibility.

Closes #688

[shifqu]

Hi!

Please, ensure to give patch.py a thorough review because there are quite some changes and it touches code that wasn't touched in a long time :)

[shifqu]

Hmm, not quite sure what the failure means. I am not that familiar with tox.

Error for 3.10 seems to be lint: FAIL code 1 (12.35=setup[10.79]+cmd[0.19,1.37] seconds)
Error for 3.11 seems to be py311-aiohttp: FAIL code 1 (33.72=setup[10.55]+cmd[23.17] seconds)

Ok, the first word in above error is actually like a step which was executed before. So eventually I found the failing test.

For 3.10 it was a linting error, weird change as it removes a line and adds one on the same number. But running black locally gave the same result, so likely a hidden character. This is fixed.

For 3.11 I think it was a flakey httpbin call, this has been mentioned before here. So maybe re-running the CI might help?

[therve]

Note that CI only runs against urllib3<2, so you should add an env to test >2.

[shifqu]

I think I just fixed that by reverting the tox.ini back to use urllib3 instead of urllib3<2

[codecov-commenter]

Codecov Report

Merging #697 (b956f2f) into master (14cef83) will decrease coverage by 0.20%.
The diff coverage is 70.58%.

❗ Current head b956f2f differs from pull request most recent head e219244. Consider uploading reports for the commit e219244 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
- Coverage   89.74%   89.55%   -0.20%     
==========================================
  Files          27       27              
  Lines        1746     1752       +6     
  Branches      311      311              
==========================================
+ Hits         1567     1569       +2     
- Misses        144      148       +4     
  Partials       35       35              

Impacted Files	Coverage Δ
vcr/stubs/boto3_stubs.py	56.25% <ø> (ø)
vcr/patch.py	85.00% <67.74%> (-0.44%)	⬇️
vcr/stubs/requests_stubs.py	100.00% <100.00%> (ø)
vcr/stubs/urllib3_stubs.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[shifqu]

Hmpf, the failure is again on the linting step for 3.10. This is due to me being too lazy to figure out how to setup the tox env properly.

I will do it some time soon, but for now the lint step will be trial and error :)

I did run isort and found the step for linting in tox.ini. So also ran flake8 on patch, but this seemed fine.
Workflows should now pass 🤞

[shifqu]

Ah, @therve! I get your point now. My revert made us test against v2+, but no longer against earlier versions. Should we create an env for this? I am not sure how this is best done, so if you have experience with it, feel free to do it or provide guidance :)

[shifqu]

I am somewhat baffled by the errors on this workflow. They are only on the two steps where I removed the urllib3 version constraint.

Guess I will have to run the tests locally :D

[hartwork]

Guess I will have to run the tests locally :D

@shifqu all you need is tox — either through PyPI or a distro package — and then run tox from the repository root. If broken state is re-used, you can wipe folder .tox to start clean. tox list shows all the environments, and tox -e <env>[,<env>,..] will only run the selected envs, e.g. tox -e lint and tox -e py311-aiohttp could be of interest.

[shifqu]

Guess I will have to run the tests locally :D

@shifqu all you need is tox — either through PyPI or a distro package — and then run tox from the repository root. If broken state is re-used, you can wipe folder .tox to start clean. tox list shows all the environments, and tox -e <env>[,<env>,..] will only run the selected envs, e.g. tox -e lint and tox -e py311-aiohttp could be of interest.

Thanks! I started reading up on tox and was indeed amazed how simple it actually is. I am currently attempting to debug what is going on :)

[hartwork]

@shifqu cool — good luck! 👍 👍

[hartwork]

@shifqu could you rebase this onto latest master? The new conflicts are mostly deletions. Please let me know if you need help with resolving the conflicts.

[shifqu]

Hey @hartwork, I just rebased.

For this PR in general, I am confident that the AttributeError is fixed, but I suspect something in the new version of urllib messes with our mocked requests and returns None as a response.

I haven't been able to look at it more closely during the weekend, but will attempt to make some time this week.

[shifqu]

For line 53 (new version), should we not have the else branch in case the error is not raised? So that we save the requests types in the variables _VerifiedHTTPSConnection, _connHTTPConnection, _connHTTPSConnection

[shifqu]

@hartwork forgot to tag :)

[therve]

For this PR in general, I am confident that the AttributeError is fixed, but I suspect something in the new version of urllib messes with our mocked requests and returns None as a response.

As I mentioned in the other pull request the change of the response type messes up vcrpy big time: urllib3/urllib3#2649. When I tried that was the cause of most of the test failures once imports were fixed.

[kuzminrobin]

FYI.
you may want to

• switch from VerifiedHTTPSConnection to HTTPSConnection class too.

[hartwork]

@shifqu thanks for the rebase! I'm experimenting with a few bits on top of yours at the moment, hopefully with some results before 02:00 UTC+2 tonight.

[hartwork]

FYI. you may want to

• switch from VerifiedHTTPSConnection to HTTPSConnection class too.

@kuzminrobin that seems mistaken, both symbols exist, both symbols need to be patched, I think. Could you elaborate why you think this way? The fact that the code sets VerifiedHTTPSConnection = HTTPSConnection does not matter, I think.

[kuzminrobin]

FYI. you may want to

• switch from VerifiedHTTPSConnection to HTTPSConnection class too.

@kuzminrobin that seems mistaken, both symbols exist, both symbols need to be patched, I think. Could you elaborate why you think this way? The fact that the code sets VerifiedHTTPSConnection = HTTPSConnection does not matter, I think.

@hartwork, I don't insist. Yes, I may be mistaken. Just the migration guide made me think that way. I'm not familiar with the code base. I just provided the link to the migration guide just in case, and wanted to make sure that the VerifiedHTTPSConnection-to-HTTPSConnection part is also taken into consideration (is known about).

Good luck in your fix efforts!

[shifqu]

The thing with changing VerifiedHTTPSConnection to HTTPSConnection is something that we will have to do eventually, since urllib will likely deprecate the VerifiedHTTPSConnection, but to do this, we need to decide which version of urllib3 we want to support. In earlier versions the VerifiedHTTPSConnection is actually the Connection class with SSL enabled, while in later versions VerifiedHTTPSConnection is just an alias.

[kuzminrobin]

Is there any chance that this issue will be resolved by end of month?
(one step of our release uses this feature and is blocked by the issue, I'm thinking whether it makes sense to work around or to wait)

[shifqu]

Hey @kuzminrobin . I hadn’t gotten around to it, but this PR will be closed in favor of #699

[hartwork]

@kuzminrobin we appreciate help with debugging why/how pull request #699 works with Python 3.10/3.11 but not 3.7/3.8/3.9, that's a blocker so far: we cannot go forward before that's better understood. The pull request has more details in its comments.