-
Notifications
You must be signed in to change notification settings - Fork 387
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
Start testing with CPython 3.13 #862
Conversation
be5e883
to
884bed4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #862 +/- ##
==========================================
+ Coverage 91.75% 92.19% +0.43%
==========================================
Files 27 27
Lines 1807 1831 +24
Branches 270 346 +76
==========================================
+ Hits 1658 1688 +30
+ Misses 102 94 -8
- Partials 47 49 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graingert please rebase/resquash to ease review in a way where…
- there a no more reverting commits canceling out prior commits (— let's get addition and revert out of history instead —)
- there are no merge commits in here
- the branch targets latest
master
…to ease review — thank you!
c9c5327
to
58ef147
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graingert thanks for the adjustments, much cleaner now. The use of pytest_httpbin.certs.where
is a nice idea but the way it's introduced is probably more complex than necessary (and the tight coupling with Python 3.13 support is not clear); also no longer calling load_cert_chain
could use an explanation why it's expected to not be needed for all environments (I'm curious, happy to understand).
The key question should be: what problem are we trying to solve? The answer seem to be: Invoking the test suite without need for REQUESTS_CA_BUNDLE="$(python3 -m pytest_httpbin.certs)"
hacks and without default failure — anything else?
Probably the simplest and backwords-compatible way forward would be a tiny patch that…
- uses
ssl_ca_location = os.environ.get("REQUESTS_CA_BUNDLE", pytest_httpbin.certs.where())
- starts using
ssl_ca_location
in two places more in the same function - then drop
REQUESTS_CA_BUNDLE
fromruntests.sh
If we need any of the other parts of the suggest diff, please share their motivation so that it becomes apparant and documented why we need them. Thank you!
It's coupled to 3.13 because python 3.13 changed the validation requirements for certificates in the default SSL context, and I updated the pytest-httpbin certs using trustme which names them differently. pytest_httpbin.certs.where() is the public interface to these files, they should not be accessed by filename. load_cert_chain is needed for server contexts and is redundant here.
No, it's because I renamed the certs in pytest-httpbin because they are the names that trustme uses, and otherwise the tests crash here.
Backwards compatibility is not a concern here, it's just in the tests.
I'm not sure what other parts of the diff you're referring to. |
I found your commit kevin1024/pytest-httpbin@7bf62b4 now, so the VCR.py tests will break with
The diff in 58ef147 is quite big, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Formally requesting changes for #862 (comment) above)
58ef147
to
f5597fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graingert that's a very nice story told by commits now — thank you! 👍
investigate #848