Loosen pytest reqs to allow upgrade to pytest 7.0.0 + Upgrade to pytest 7 #14380
Conversation
| @@ -63,7 +63,7 @@ class PyTest(PythonToolBase): | |||
| # This should be kept in sync with `requirements.txt`. | |||
| # TODO: To fix this, we should allow using a `target_option` referring to a | |||
| # `python_requirement` to override the version. | |||
| default_version = "pytest>=6.2.4,<6.3" | |||
| default_version = "pytest>=6.2.4,<8" | |||
There was a problem hiding this comment.
I think that if we're expecting / have-a-default-lock-of 7, then we shouldn't mention 6 here, and should instead bump the minimum end of the range to the version that you ended up testing with.
While it's possible that folks using plugins while relying on this default value might need to actually set the version if their plugins are no longer compatible with 7, that seems fine to me.
There was a problem hiding this comment.
why not?
This allows downstream repos that have pants plugins (and thus use pantsbuild.pants.testutil ) use pytest 6.
I don't think keeping tight dependency between pantsbuild.pants.testutil and pytest is a good idea when there is no technical reason I can think of to do so.
There was a problem hiding this comment.
This allows downstream repos that have pants plugins (and thus use
pantsbuild.pants.testutil) use pytest 6.
They can still do that, by explicitly setting the version. This is just the default.
There was a problem hiding this comment.
nope... it doesn't work. see:
this is when trying to upgrade a downstream repo (toolchain) that has plugins (and plugin tests) to pytest 7.
There was a problem hiding this comment.
Hm, we could have our requirements.txt be loose, but have pytest.py be tighter. Note that pytest.py's version only needs to be compatible with requirements.txt, they don't need to be identical. pytest.py is what is used for the default tool lockfile, whereas requirements.txt gets used by python_distribution for the install_requires of pansbuild.pants.testutil
|
I'm getting 500 errors when trying to review (reported to Github), but ditto Stu's comments on not mentioning Pytest 6. Also probably constraint to <7.1, as their changelog says they will be making API changes in 7.1. |
the api changes that are planned are for plugin APIs I think it is not likely that those API changes will break any user facing APIs and thus not those deprecations and changes are not relevant for this repo. |
|
K, then that's fine to use <8. But I don't think we should include 6. |
that will be a bad idea. because it forces downstream repos to upgrade since this line in requirements.txt propagates into dependencies of |
| @@ -10,7 +10,7 @@ | |||
| # ], | |||
| # "generated_with_requirements": [ | |||
| # "pytest-cov!=2.12.1,<3.1,>=2.12", | |||
| # "pytest<6.3,>=6.2.4" | |||
| # "pytest<8,>=6.2.4" | |||
There was a problem hiding this comment.
This needs to be regenerated. build-support/bin/generate_all_lockfiles.sh --tool pytest. No need to regenerate --internal though afaict.
| @@ -63,7 +63,7 @@ class PyTest(PythonToolBase): | |||
| # This should be kept in sync with `requirements.txt`. | |||
There was a problem hiding this comment.
Thank you - could you also please update the comment here to say:
This should be compatible with
requirements.txt, although it can be more precise.
3rdparty/python/requirements.txt
Outdated
| pytest>=6.2.4,<6.3 # This should be kept in sync with `pytest.py`. | ||
| pytest>=6.2.4,<8 # This should be kept in sync with `pytest.py`. |
There was a problem hiding this comment.
Can you please update the comment to say this?
This should be compatible with pytest.py, although it can be looser so that we don't over-constrain pantsbuild.pants.testutil
(Will want to move comment above the entry, rather than inline)
Head branch was pushed to by a user without write access
|
How come the revert for pytest-cov? Is this the final version you want merged? |
it breaks the test, probably because pytest-cov>3 is not compatible with older python versions, so I had to revert it. |
the tight requirements prevents downstream projects from upgrading to pytest 7.0.0
#14383 will need to land first.