Skip to content
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

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

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Oct 12, 2023

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:

  • No more pinning urllib3 at runtime to make a dev requirement happy
  • No more snarky incompatibility warning from pip that types-requests wants urllib3 2.x but we have 1.x (seen in CI logs)
  • No more weird BrotliDecoderDecompressStream exceptions in my local environment (which didn't happen in CI)

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

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

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 dgw added Tests Build Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Oct 12, 2023
@dgw dgw added this to the 8.0.0 milestone Oct 12, 2023
@dgw dgw requested a review from a team October 12, 2023 05:06
@dgw
Copy link
Member Author

dgw commented Oct 12, 2023

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
Copy link
Contributor

SnoopJ commented Oct 12, 2023

@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
Copy link
Member Author

dgw commented Oct 12, 2023

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
Copy link
Member Author

dgw commented Oct 12, 2023

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 dgw removed the request for review from a team October 12, 2023 22:57
@dgw dgw marked this pull request as draft October 12, 2023 22:58
@dgw
Copy link
Member Author

dgw commented Oct 15, 2023

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
Copy link
Member Author

dgw commented Oct 24, 2023

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).

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. Skipping two whole major versions probably isn't ideal, but I like
this solution because:

* No more pinning urllib3 at runtime to make a dev requirement happy
* No more snarky incompatibility warning from pip that types-requests
  wants urllib3 2.x but we have 1.x (seen in CI logs)
* No more weird BrotliDecoderDecompressStream exceptions in my local
  environment (which didn't happen in CI)

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.

vcrpy 5 is currently installed from a fork that removes the upstream
version cap on urllib3<2 for Python < 3.10, as it seems cassettes are
often incompatible across major urllib3 versions. (Cassettes that would
work when tests ran under urllib3 1.x raised exceptions under urllib3
2.x, and vice versa.)

Upstream, the vcrpy maintainers have been totally silent since I opened
a pull request suggesting the removal of this version cap. After two
weeks without any acknowledgement whatsoever, I chose to create a fork
within Sopel's GitHub organization specifically for use with our CI and
dev-environment setup.

This should be more or less fine, since only CI and maintainers are
likely to bother installing the dev-requirements. Packaged releases
don't include them, so we can move pretty quickly to get current again
whenever the vcrpy maintainers accept the change (or if they do).

The "internal" fork also guards against deleting my personal fork. (When
a patch I submitted is merged, I habitually delete my fork if I don't
foresee making further contributions to that project.)
@dgw dgw marked this pull request as ready for review October 30, 2023 13:51
@dgw dgw requested a review from a team October 30, 2023 13:51
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I'm all for it.

@dgw dgw merged commit eb5e029 into master Oct 30, 2023
11 checks passed
@dgw dgw deleted the upgrade-to-vcrpy-5 branch October 30, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build hacktoberfest-accepted Housekeeping Code cleanup, removal of deprecated stuff, etc. Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants