Fix test_https_over_http_error on Windows with OpenSSL 3.1+#828
Fix test_https_over_http_error on Windows with OpenSSL 3.1+#828julianz- wants to merge 1 commit into
Conversation
cc2efd3 to
deeb517
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #828 +/- ##
==========================================
- Coverage 78.19% 78.17% -0.02%
==========================================
Files 41 41
Lines 4788 4793 +5
Branches 547 549 +2
==========================================
+ Hits 3744 3747 +3
- Misses 905 906 +1
- Partials 139 140 +1 |
deeb517 to
f5bb643
Compare
webknjaz
left a comment
There was a problem hiding this comment.
Thanks! I've been meaning to ask you to look into what's changed in the CI env.
See a few notes on the patch inline.
P.S. I wonder how difficult it'd be to have predictable OpenSSL code paths in CI. Is CPython built statically, or can we somehow substitute OpenSSL it uses with something we pre-build? Since we have a few TLS-dependent code paths in runtime, it'd be good to run tests against each of them in CI at some point.
There was a problem hiding this comment.
Since this patch doesn't fix a bug in Cheroot's runtime, it shouldn't be listed among bugfixes in the change log. Fixing a test is infra work. So it could be a contrib note. Perhaps, a packaging note when we declare that we support new things (like a new Python version, a new OS or a new dep in this case).
Though, feel free to opt out of attaching a change note if it doesn't feel important to surface to the change log audience. You can add the bot:chronographer:skip label and the bot will stop flagging a missing note in the PR.
There was a problem hiding this comment.
Good points. I changed to a contrib note.
Regarding CPython, I get your idea but it sounds complicated. I was thinking before the issue was the version of OpenSSL but apparently macOS 15 CI runners use Python 3.14.6 with the same bundled OpenSSL 3.6.3 as Windows, but unlike Windows, macOS was already returning the correct record layer failure error. So I have modified the code the check for Windows in addition to the OpenSSL version.
| assert ( | ||
| 'record layer failure' in actual_error | ||
| or 'wrong version number' in actual_error | ||
| ), actual_error |
There was a problem hiding this comment.
This shouldn't be needed since pytest is invoked w/ CLI options that should reveal the vars in scope:
| ), actual_error | |
| ) |
f5bb643 to
52c9d19
Compare
On Windows, the TCP reset (RST) takes a different code path than Linux, causing OpenSSL 3.1+ to report 'wrong version number' rather than 'record layer failure'. Accept either string when IS_ABOVE_OPENSSL31 is True — the same pattern used in CPython's own test_ssl.py.
52c9d19 to
793a4e2
Compare
test_https_over_http_errorstarted failing in the CI on Windows with Python 3.14 after recent GitHub Actions runner image updates bumped Python 3.14.5 → 3.14.6 and OpenSSL 3.6.2 → 3.6.3 (win25/20260614.167 deployed ~June 19, win22/20260616.203 deployed shortly after).The test expects record layer failure when
IS_ABOVE_OPENSSL31isTrue(OpenSSL > 3.1), but with Python 3.14.6 bundling OpenSSL 3.6.3 on Windows both windows-2025 and windows-2022 now returnwrong version numberinstead. This is a known Windows behavior: the TCP reset is surfaced to userspace before OpenSSL fully processes the server's response, causing a different error code to be reported (documented in CPython's own test_ssl.py).Linux is currently unaffected — Ubuntu 24.04 uses system OpenSSL 3.0.x, so
IS_ABOVE_OPENSSL31isFalsethere and the test falls through to the elifIS_ABOVE_OPENSSL10branch, which already expectswrong version number.❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #
❓ What is the current behavior? (You can also link to an open issue here)
CI errors:
e.g.
https://github.com/cherrypy/cheroot/actions/runs/27889445941/job/82595847403?pr=827
❓ What is the new behavior (if this is a feature change)?
📋 Other information:
📋 Contribution checklist:
(If you're a first-timer, check out
[this guide on making great pull requests][making a lovely PR])
the changes have been approved
and description in grammatically correct, complete sentences