sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
951 stars 405 forks source link

build, test: upgrade to vcrpy>=5 & refresh broken cassettes #2519

Closed dgw closed 8 months ago

dgw commented 8 months ago

This is probably the ideal solution to my issues with certain VCR cassettes failing locally, since we were on vcrpy 2.x for quite a long time. The error I saw on my local environment went away after upgrading vcrpy (to 5.x) & urllib3 (to 2.x).

Skipping two whole major versions of vcrpy probably isn't ideal, but I like this solution because:

Note that this commit only updates half a dozen YAML cassettes for two plugins whose tests failed after upgrading vcrpy. Working cassettes have been left untouched.

Checklist

Notes

I'm sort of expecting some new and exciting error to happen in CI that I don't see locally, because that's just what happens when I start messing with anything related to VCR.

All the same, I figured I should probably try to see if updating some dependencies would fix that weird Brotli error I ran into locally while creating new cassettes for #2512 (which worked in CI). Needing to update some cassettes for this patch isn't a huge surprise, since I expect some degree of change in cassette behavior across major versions of the VCR library.

dgw commented 8 months ago

I called it on the "new and exciting error" happening in CI, didn't I? Python 3.10 and 3.11 passed, but got canceled by 3.8 failing with an error about gzip this time… 3.9 didn't get far enough along to tell if the same tests would fail.

It's still related to urllib3 or requests, with zlib this time as the third suspect:

# First exception in zlib code
zlib.error: Error -3 while decompressing data: incorrect header check

# ...leads to a second exception in urllib3
urllib3.exceptions.DecodeError: ('Received response with content-encoding: gzip, but failed to decode it.', error('Error -3 while decompressing data: incorrect header check'))

# ...which finally raises an error in requests-land
requests.exceptions.ContentDecodingError: ('Received response with content-encoding: gzip, but failed to decode it.', error('Error -3 while decompressing data: incorrect header check'))

Couldn't say why this might happen just yet, but this Stack Overflow answer seems relevant. The most common reason for this error would be trying to decode data that has no header (deflate method) as if it has a header, and that's behavior that could've changed in zlib (part of Python's stdlib) between py3.8 and a future release.

@SnoopJ can I ask you to use your tox magic when you've a moment, so I don't hammer everyone subscribed to the repo with (force-)push notification emails in an attempt to debug-by-remote? Not able to easily get py3.8 on my local system.

SnoopJ commented 8 months ago

@SnoopJ can I ask you to use your tox magic when you've a moment, so I don't hammer everyone subscribed to the repo with (force-)push notification emails in an attempt to debug-by-remote? Not able to easily get py3.8 on my local system.

I see this failure locally for test_example_suggest_N (N ∈ {0, 1, 2}) in both Python 3.8 and 3.9. Passes in 3.10 and 3.11. It looks like this can also happen if the remote server claims it's sending gzipped data, but that seems pretty unlikely since 3.10 and 3.11 pass for me as well. Not sure what's going on there.

dgw commented 8 months ago

Interim update from IRC: @SnoopJ has dug into this a bit more (❤️) and ruled out the remote server doing funky stuff. We now suspect that the error is just a symptom of something else.

For example, I noticed in one of the YAML diffs for this PR (test_example_suggest_0) that while a content-encoding: gzip header is stored in the cassette, the actual body was changed from binary to plaintext.

dgw commented 8 months ago

Now 99% certain it's due to this dependency change that's still being shipped with vcrpy 5.x. In fact, the VerifiedHTTPSConnection/HTTPSConnection issue is what kept me from trying to update vcrpy sooner.

I don't believe we can force pip to install urllib3>=2 on older Python releases when vcrpy requires urllib3 <2; python_version <'3.10', but thanks to @SnoopJ's testing environments, we know that urllib3==2.0.6 works on all four currently tested Python branches (3.8 – 3.11) with OpenSSL 1.1.1f. Upstream discussion mentions a compatibility problem with OpenSSL 3.0, though. I have openssl 3.0.2-0ubuntu1.10 locally, but have to make time for creating a Python 3.8 environment to test.

dgw commented 8 months ago

Tested the current HEAD of this PR branch in a fresh Python 3.8.18 installation (hat-tip: pyenv) on Ubuntu 22.04, which is using OpenSSL 3.0. The tests that failed CI here worked with urllib3 2.0.6 and vcrpy 5.1.0:

dgw@ROGAlly:~/github/sopel-irc/sopel$ pytest -v . -k suggest
================================================= test session starts ==================================================
platform linux -- Python 3.8.18, pytest-7.1.3, pluggy-1.3.0 -- /home/dgw/.pyenv/versions/3.8.18/bin/python3.8
cachedir: .pytest_cache
rootdir: /home/dgw/github/sopel-irc/sopel, configfile: pyproject.toml
plugins: vcr-1.0.2, requests-mock-1.9.3, sopel-8.0.0.dev0
collected 1332 items / 1328 deselected / 4 selected

sopel/modules/search.py::test_example_suggest_0 PASSED                                                           [ 25%]
sopel/modules/search.py::test_example_suggest_1 PASSED                                                           [ 50%]
sopel/modules/search.py::test_example_suggest_2 PASSED                                                           [ 75%]
sopel/modules/search.py::test_example_suggest_3 PASSED                                                           [100%]

========================================== 4 passed, 1328 deselected in 0.84s ==========================================
dgw@ROGAlly:~/github/sopel-irc/sopel$ openssl version
OpenSSL 3.0.2 15 Mar 2022 (Library: OpenSSL 3.0.2 15 Mar 2022)
dgw@ROGAlly:~/github/sopel-irc/sopel$ pip list installed | grep urllib3
types-urllib3                 1.26.25.14
urllib3                       2.0.6
dgw@ROGAlly:~/github/sopel-irc/sopel$ pip list installed | grep vcrpy
vcrpy                         5.1.0

From there I ran the full make test, switched to a fresh Python 3.9 environment, and checked there too (after forcing urllib3 to upgrade and double-checking openssl version + the pip list commands above). No problems; it all works.

I think it's time to take this upstream and see if vcrpy will consider making a release that uncaps urllib3 on older Pythons. We'll probably end up with the kludge @SnoopJ suggested on IRC (<+SnoopJ> alternative bunt: YOLO and force-upgrade urllib3 in CI) but I sure hope it'll be temporary if we do and not stick around until py3.9 EOL.

dgw commented 8 months ago

Hoping for some response to kevin1024/vcrpy#777 because oy, the kludge to force-upgrade urllib3 in CI is an ugly one for local dev environments (where the CI scripts are obviously not used).