pip
pip copied to clipboard
pypa
Ignore require-virtualenv in `pip index`
resolves #10657
extremely similar to #8603 and follows same conventions
It would be nice to have a test for this - but I note that #8603 didn't include a test, so I'm inclined to accept this without a test and add tests for both cases in a follow-up PR.
@pfmoore
It would be nice to have a test for this - but I note that #8603 didn't include a test, so I'm inclined to accept this without a test and add tests for both cases in a follow-up PR.
I started taking a look at the tests- as far as I can tell require_venv isn't actually tested anywhere and it's not clear to me that there would be any benefit to testing the behavior of ignore_require_venv for this particular command.
This is the only reference I can find (tests/unit/test_options.py) and it only seems to be asserting that the order (command, option) doesn't matter.
480 def test_require_virtualenv(self) -> None:
481 # FakeCommand intentionally returns the wrong type.
482 options1, args1 = cast(
483 Tuple[Values, List[str]], main(["--require-virtualenv", "fake"])
484 )
485 options2, args2 = cast(
486 Tuple[Values, List[str]], main(["fake", "--require-virtualenv"])
487 )
488 assert options1.require_venv
489 assert options2.require_venv
my first thought is to implement something like:
- mock virtualenv by monkey patching
running_under_virtualenv()incli/base_command.py(with return value of has_venv column in table below) - mock ignore_require_venv by monkeypatching FakeCommand
- test condition:
- if require_venv and not ignore_require_venv and not has_venv:
assert_option_error(msg)
- if require_venv and not ignore_require_venv and not has_venv:
however... that test condition looks an awful lot like a reimplementation of the original implementation...
# cli/base_command.py
if options.require_venv and not self.ignore_require_venv:
# If a venv is required check if it can really be found
if not running_under_virtualenv():
logger.critical("Could not find an activated virtualenv (required).")
sys.exit(VIRTUALENV_NOT_FOUND)
error condition truth matrix:
| "has_venv" | "require_venv" | "ignore_require_venv" | "should_error" |
|---|---|---|---|
| T | T | T | F |
| T | T | F | F |
| T | F | T | F |
| T | F | F | F |
| F | T | T | F |
| F | T | F | T |
| F | F | T | F |
so I guess I'm a little lost on what a test for this should look like...
Maybe also relevant to note I git grepped it and there are like 10 commands that set ignore_require_venv (and none seem to have tests)...
theres another 'meta' problem here that we might test tho...
one idea might be to have an explicit list of commands known to require_venv and fail the test for new commands that require_venv and arent in that list? that would serve as a prompt for future developers to consider whether new commands should ignore_require_venv when it fails. it looks like its been a pretty easy thing to overlook historically
PS: previous CI run failed from missing double-backticks
`requires-virtualenv` ---> ``requires-virtualenv``
i git commit --amend and git push origin +branch, should be good now
require-virtualenv not being tested is a big part of why it wasn't a documented/supported UI.
I think it's fine to kick the can down the road for this, and just merge this as-is.
This has been already done by https://github.com/pypa/pip/commit/62fb64ac9697e36efce3f72193e4bff0a39dbe14. I'll open an issue to track adding tests for this.
