-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Upgrade pytest and related plugins. #8177
Conversation
f23b644
to
aad3e55
Compare
Did you talk to Toolchain and John about this? I don't understand super well how Pytest works, but think we need to keep using 4.x so that you can run Pytest with Py2, which we will need to do for a long time as continue to allow users to use Py2 with their own code. |
@Eric-Arellano I kept pytest<5.0 for exactly that reason (allowing pants to run pytest for python 2.7 projects). I did git blame to see if there was any special reason to pinning pytest and didn't notice anything. @jsirois was out this week. |
Ah I see. Read PR too quickly! Looks good to me. |
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.
LGTM, probably worth waiting for John to weigh in, just in case.
3rdparty/python/requirements.txt
Outdated
@@ -17,8 +15,9 @@ psutil==5.4.8 | |||
Pygments==2.3.1 | |||
pyopenssl==17.3.0 | |||
pystache==0.5.3 | |||
pytest-cov>=2.5,<2.6 | |||
pytest>=3.4,<4.0 | |||
pytest-cov>=2.6,<2.8 |
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.
This really needs to happen here as well / more importantly:
pants/src/python/pants/backend/python/subsystems/pytest.py
Lines 13 to 32 in b4e316f
# TODO: This is currently bounded below `3.7` due to #6282. | |
register('--requirements', advanced=True, default='pytest>=3.0.7,<3.7', | |
help='Requirements string for the pytest library.') | |
register('--timeout-requirements', advanced=True, default='pytest-timeout>=1.2,<1.3', | |
help='Requirements string for the pytest-timeout library.') | |
register('--cov-requirements', advanced=True, default='pytest-cov>=2.4,<2.5', | |
help='Requirements string for the pytest-cov library.') | |
register('--unittest2-requirements', advanced=True, default='unittest2>=0.6.0,<=1.9.0', | |
help='Requirements string for the unittest2 library, which some python versions ' | |
'may need.') | |
def get_requirement_strings(self): | |
"""Returns a tuple of requirements-style strings for pytest and related libraries. | |
Make sure the requirements are satisfied in any environment used for running tests. | |
""" | |
opts = self.get_options() | |
# TODO(6282): once we can upgrade Pytest to 4.2.1+, we can remove the more-itertools requirement | |
return ( | |
"more-itertools<6.0.0 ; python_version<'3'", |
I just noticed though that these direct deps are no longer used by pants - yay!:
$ git grep 3rdparty/python:pytest
$
So please just kill these pytest and pytest-cov requirements outright.
When you do fix src/python/pants/backend/python/subsystems/pytest.py
you may hit test failures due to #6282. If not, excellent - please add a "Fixes #6282" to the PR description so that issue closes out. If so, then this is not so simple :/
looks like there are some legit test failures. I will look more into this later this week. |
[Unused](#8177 (comment)) direct dependecies in requirements.txt
I am experimenting with modifying code that breaks when we upgrade pytest. |
Problem
The repo is using a relativly old pytest version, version 5.0 was recently released (which drops support to python 2.7)
So at the very least we should be on the 4.x series.
Solution
Upgrade pytest and related packages/plugins