pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.45k stars 3k forks source link

Add tests for require-virtualenv #12843

Open ichard26 opened 1 month ago

ichard26 commented 1 month ago

As far as I know, the require-virtualenv functionality is essentially entirely untested. This is the main reason why it is not documented (see also https://github.com/pypa/pip/issues/2429).

It would be good to see tests for --require-virtualenv

Below, I've attached some old discussion on this topic.


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:

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

Originally posted by @justin-f-perez in https://github.com/pypa/pip/issues/10658#issuecomment-968286761

matthewhughes934 commented 1 month ago

For the erroring case, could we avoid the monkey patching and just define a custom command and 'pretend' we're not in a virtualenv, something like (the attribute setting/restoring magic should probably be in a fixture/context manager):

    def test_require_virtualenv_errors_when_not_in_venv(self) -> None:
        class Cmd(Command):
            ignore_require_venv: bool = False

        # edit variable set by PEP-405 compliant virtualenvs
        # to pretend we're not in a venv even if we actually are
        has_base_prefix = hasattr(sys, "base_prefix")
        old_base_prefix = getattr(sys, "base_prefix", None)
        if has_base_prefix:
            delattr(sys, "base_prefix")

        try:
            with pytest.raises(SystemExit) as excinfo:
                Cmd(name="fake", summary="").main(["--require-virtualenv"])

            assert excinfo.value.code == VIRTUALENV_NOT_FOUND
        finally:
            # restore our modifications
            if has_base_prefix:
                sys.base_prefix = old_base_prefix