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

Drop python 3.8 support #12875

Merged
merged 9 commits into from
Nov 28, 2024

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Oct 10, 2024

Closes #12874

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Looks lovely at first glance

.github/workflows/test.yml Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft October 10, 2024 16:31
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 966 to 971
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()
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what to do here, advise welcome

Copy link
Member

@webknjaz webknjaz Oct 10, 2024

Choose a reason for hiding this comment

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

#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:

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:

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 10, 2024
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Oct 10, 2024

Changed the configuration of the repo so build (ubuntu-py38) and build (windows-py38) are not required anymore but 3.13 is. Two readthedoc warnings appeared related to sphinx_issues, not sure how to fix:

WARNING: while setting up extension sphinx_issues: role 'cve' is already registered, it will be overridden [app.add_role]
WARNING: while setting up extension sphinx_issues: role 'cwe' is already registered, it will be overridden [app.add_role]

@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review October 10, 2024 21:43
@Pierre-Sassoulas Pierre-Sassoulas changed the title [Work in progress] Drop python 3.8 support Drop python 3.8 support Oct 10, 2024
@webknjaz
Copy link
Member

@Teri53 @TeriThetha please stop spamming.

@webknjaz
Copy link
Member

@nicoddemus @RonnyPfannschmidt @The-Compiler can anybody ban the spammer, please?

@webknjaz
Copy link
Member

🙄 c'mon...
spam-post-appology

@nicoddemus
Copy link
Member

Blocked.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

i like it !

the messy nature of the github matrix makes me wonder,
if we should investigate a script to generate it as a followup

its so easy to make mistakes in it, its scary

.github/workflows/test.yml Show resolved Hide resolved
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pytest that referenced this pull request Oct 12, 2024
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Oct 12, 2024

The sphinx_issues warning was resolved magically in CI as well as locally, not sure how (I upgraded to latest ubuntu LTS / python 3.12.3), so only #12875 (comment) remains.

src/_pytest/capture.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@Pierre-Sassoulas so I have all the files in this PR marked as reviewed except for src/_pytest/capture.py due to the 3.7 think I noticed. Not sure how far you'd want to go with addressing it, but everything LGTM otherwise.

@Pierre-Sassoulas
Copy link
Member Author

Depends on #12910 (so we can activate pylint's consider-alternative-union-syntax and deprecated-typing-alias).

changelog/12874.breaking.rst Outdated Show resolved Hide resolved
src/_pytest/config/argparsing.py Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

Fixing your other comment and reverting the bad fix, worked:

============================= test session starts =============================
platform win32 -- Python 3.10.11, pytest-8.4.0.dev159+gbd1faf745, pluggy-1.5.0

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks, sorry for the delay!

@Pierre-Sassoulas
Copy link
Member Author

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

@nicoddemus
Copy link
Member

Cleaning up the commit history here seems like a good idea.

@Pierre-Sassoulas
Copy link
Member Author

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)

@nicoddemus
Copy link
Member

LGTM!

@Pierre-Sassoulas
Copy link
Member Author

@RonnyPfannschmidt do you want to do a final review ?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

The changes look good to me, thanks for doing the work!

@@ -10,6 +13,7 @@
import os
from pathlib import Path
import re
from re import Pattern
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

pyproject.toml Outdated
@@ -85,7 +84,7 @@ write_to = "src/_pytest/_version.py"

[tool.black]
target-version = [
'py38',
'py39',
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

e2f17e0

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)

@nicoddemus
Copy link
Member

nicoddemus commented Nov 26, 2024

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

@Pierre-Sassoulas Pierre-Sassoulas merged commit b938e70 into pytest-dev:main Nov 28, 2024
28 checks passed
@Pierre-Sassoulas Pierre-Sassoulas deleted the drop-python3.8 branch November 28, 2024 20:26
@nicoddemus
Copy link
Member

Awesome, thanks @Pierre-Sassoulas for tackling this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop Python 3.8 support
7 participants