Turn PipDeprecationWarning into errors when running tests#13947
Turn PipDeprecationWarning into errors when running tests#13947danielhollas wants to merge 3 commits into
Conversation
|
Thanks for this, it was going to be the next thing I did, I played around with this approach and found it doesn't help functional tests that call pip as a subprocess, as the pytest machinery is lost there. I was playing around with this and I think a good approach is to have But open to ideas. |
|
Good point about subprocesses. I likely won't have time to push this further over the next three weeks, feel free to push to this PR or open a new one. (I guess the pytest setting I have here still seems useful even if it doesn't cover everything). |
c8f406e to
808bc14
Compare
@notatallshaw I've implemented this approach, I think it works well. Let me know what you think. I was not sure how to name this env var, happy to adjust it. We could even think about making such a variable public-facing, as I can imagine some users of PIP (e.g. in CI) might want to turn pip warnings into errors as well. But I'd rather keep that discussing for a separate PR if wanted. |
| # If we're running pip test suite, promote the PipDeprecationWarning into errors. | ||
| if os.environ.get("PIP_TEST_ENV", None): | ||
| warnings.simplefilter("error", PipDeprecationWarning) | ||
| else: | ||
| warnings.simplefilter("default", PipDeprecationWarning, append=True) |
There was a problem hiding this comment.
Now that I think about this, we could have a variable PIPWARNINGS and have it behave in the same way as PYTHONWARNINGS?
| # If we're running pip test suite, promote the PipDeprecationWarning into errors. | |
| if os.environ.get("PIP_TEST_ENV", None): | |
| warnings.simplefilter("error", PipDeprecationWarning) | |
| else: | |
| warnings.simplefilter("default", PipDeprecationWarning, append=True) | |
| action = os.environ.get("PIPWARNINGS", "default"): | |
| try: | |
| warnings.simplefilter(action, PipDeprecationWarning, append=True) | |
| except ValueError as e: | |
| warnings.warn("Invalid PIPWARNINGS variable ignored: {e}") | |
| warnings.simplefilter("default", PipDeprecationWarning, append=True) |
There was a problem hiding this comment.
I'd much rather prefer that we keep the envvar private. I don't immediately see why external users would use it, but let's discourage it while we can.
|
Thanks, I won't be able to review anything for at least a couple of weeks, but it's high on my review list once I get some time. |
ichard26
left a comment
There was a problem hiding this comment.
Looks generally good, but I want @notatallshaw's second opinion. I'm not sure about the envvar naming, in particular.
This is motivated by #13912, which added a
PipDeprecationWarningfor unexpected lazy imports during installation. But these warnings could slip through the test suite if some future change in pip introduced such an import. In this PR I propose to turn allPipDeprecationWarningsinto errors using pytest configuration. If this too broad, we can make it more specific by only targetting the warning added in #12912.This is an internal-only change so I don't think needs a NEWS entry.