Drop python 3.8 support #12875
Drop python 3.8 support #12875
Conversation
testing/code/test_excinfo.py
Outdated
| if upframe.f_code.co_name == "_makepath": | ||
| # Only raise with expected calls, but not via e.g. inspect for | ||
| # py38-windows. | ||
| # py38-windows. (?) | ||
| raised += 1 | ||
| raise OSError(2, "custom_oserror") | ||
| return orig_path_cwd() |
There was a problem hiding this comment.
Not sure what to do here, advise welcome
There was a problem hiding this comment.
#6529 does not offer any explanation, however #6463 (comment) links it and implies that there might've been flakiness in the Codecov uploader. But this was like almost 5 years ago, and Codecov has since rewritten their uploader 3 times. We've additionally made it less flaky by hardcoding a plain-text token a few months ago.
Looking into the coverage reports, something weird is happening. The if-line is marked as covered partially, both in this PR and on main:
- https://app.codecov.io/gh/pytest-dev/pytest/pull/12875/blob/testing/code/test_excinfo.py#L966
- https://app.codecov.io/gh/pytest-dev/pytest/blob/main/testing%2Fcode%2Ftest_excinfo.py#L966
However, the return line is reported as 100% covered in the PR (https://app.codecov.io/gh/pytest-dev/pytest/pull/12875/blob/testing/code/test_excinfo.py#L971) while on main it's marked as not covered (https://app.codecov.io/gh/pytest-dev/pytest/blob/main/testing%2Fcode%2Ftest_excinfo.py#L966).
AFAIU, the raise instruction can only be reached on if True and the return statement would be reachable on if False. That should cover both branches of the if-block. So I don't understand why the conditional is marked with partial coverage.
_makepath() is also fully covered in both cases:
- https://app.codecov.io/gh/pytest-dev/pytest/pull/12875/blob/src/_pytest/_code/code.py#L955
- https://app.codecov.io/gh/pytest-dev/pytest/blob/main/src%2F_pytest%2F_code%2Fcode.py#L958
P.S. While looking into Codecov, I realized that it does not show any coverage for src/pytest/ — it's just not there. It looks like we also have Coveragepy misconfigured or something. Something to investigate.
There was a problem hiding this comment.
I investigated a little, it was introduced in 79ae86c so that this function did not raise accidentally when inspecting on some env. So this is still useful as it's not only applicable for windows py38.
psf-chronographer
bot
added
the
bot:chronographer:provided
|
Changed the configuration of the repo so |
Pierre-Sassoulas
force-pushed
the
drop-python3.8
branch
from
14b36ce
to
3f03eda
Compare
Pierre-Sassoulas
changed the title
Pierre-Sassoulas
force-pushed
the
drop-python3.8
branch
2 times, most recently
from
3dd3871
to
62da60e
Compare
|
This is ready for review. I have no idea why I need fe1f3f7 (or why it fixes the issue) but otherwise we get a: (See the commit before the last one's pipeline : https://github.com/pytest-dev/pytest/actions/runs/11775505463). |
There was a problem hiding this comment.
Strange, all jobs are installing Python 3.9 and using that. For example, this is the windows-py312 job:
https://github.com/pytest-dev/pytest/actions/runs/11775818422/job/32797006395?pr=12875
It is clearly installing Python 3.9 ("Set up Python 3.9") and we can see that also in the test suite:
============================= test session starts ==============================
platform linux -- Python 3.9.20, pytest-8.4.0.dev153+gbedb1aecd, pluggy-1.5.0
From what I can tell all jobs are like that.
|
Fixing your other comment and reverting the bad fix, worked: |
|
No problem, it's a lot to review ! There was a conflict that I fixed, I'm going to clean up the history later on. (Or maybe we can squash everything ? Feels wrong though). |
|
Cleaning up the commit history here seems like a good idea. |
Pierre-Sassoulas
force-pushed
the
drop-python3.8
branch
from
1484446
to
3def6df
Compare
|
4d3e7ae is bulky, but it's the only way to have each commit passing the tests suite (fixup of the pre-commit automated fixes and the CI/doc already reviewed changes) |
|
LGTM! |
|
@RonnyPfannschmidt do you want to do a final review ? |
src/_pytest/_code/code.py
Outdated
| @@ -10,6 +13,7 @@ | |||
| import os | |||
| from pathlib import Path | |||
| import re | |||
| from re import Pattern | |||
There was a problem hiding this comment.
Feel free to ignore if it's a hassle: to me match: re.Pattern would be clearer than match: Pattern, so I'd drop the from re import Pattern and use it qualified instead. Same for re.Match.
pyproject.toml
Outdated
| @@ -85,7 +84,7 @@ write_to = "src/_pytest/_version.py" | |||
|
|
|||
| [tool.black] | |||
| target-version = [ | |||
| 'py38', | |||
| 'py39', | |||
There was a problem hiding this comment.
Side note, according to https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#t-target-version, we don't need to configure this because black also looks at requires-python. It also says we're actually supposed to list all supported versions, oh well.
There was a problem hiding this comment.
Feel like I could have removed the option too there. I'm not sure if this option is useful since the example they gave in python 3.5. (Also black is only used in doc for blacken-docs afair)
Pierre-Sassoulas
force-pushed
the
drop-python3.8
branch
from
e2f17e0
to
58e2581
Compare
|
I believe it is OK to merge this, as there is a lot of approvals already, and these changes are a bother to maintain around given they can easily create conflicts. If anybody makes comments after the merge, we can always make a follow up. 👍 |
Closes #12874