Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Resolve to interpreter 'python' in PATH if '--version' fits #224

Merged
merged 4 commits into from
Aug 16, 2019
Merged

Resolve to interpreter 'python' in PATH if '--version' fits #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0dcd1d9
Fix #219: Resolve interpreter 'python' in PATH if --version fits
schuderer Jul 24, 2019
1ce4c7b
Reduce test complexity, implement review suggestions
schuderer Aug 3, 2019
a2b37a4
Return str instead of path object
schuderer Aug 3, 2019
f1e93c6
Merge remote-tracking branch 'upstream/master' into resolve-python-ex…
schuderer Aug 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 38 additions & 1 deletion nox/virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,39 @@ def locate_via_py(version):
return None


def locate_using_path_and_version(version):
"""Check the PATH's python interpreter and return it if the version
matches.

On systems without version-named interpreters and with missing
launcher (which is on all Windows Anaconda installations),
we search the PATH for a plain "python" interpreter and accept it
if its --version matches the specified interpreter version.

Args:
version (str): The desired Python version. Of the form ``X.Y``.

Returns:
Optional[str]: The full executable path for the Python ``version``,
if it is found.
"""
if not version:
return None

script = "import platform; print(platform.python_version())"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why lift this out of the if statement but not out of the function as a module global?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming for style consistency with locate_via_py. I can put it inside the if block if it helps (would prefer not to make it module global, as it is the only time this string is used). Should I?

path_python = py.path.local.sysfind("python")
if path_python:
try:
prefix = "{}.".format(version)
version_string = path_python.sysexec("-c", script).strip()
if version_string.startswith(prefix):
return str(path_python)
except py.process.cmdexec.Error:
return None

return None


def _clean_location(self):
"""Deletes any existing path-based environment"""
if os.path.exists(self.location):
Expand Down Expand Up @@ -256,11 +289,15 @@ def _resolved_interpreter(self):
xy_version = cleaned_interpreter

path_from_launcher = locate_via_py(xy_version)

if path_from_launcher:
self._resolved = path_from_launcher
return self._resolved

path_from_version_param = locate_using_path_and_version(xy_version)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to do this before the if _SYSTEM != "Windows": check on line 197? I.e. just checking for python on PATH and matching version against python --version could work fine on non-Windows operating systems.

Copy link
Contributor Author

@schuderer schuderer Jul 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to put the python-in-path-check before the Windows check, because, it being a stop-gap solution, then it would precede the python-launcher-check, which is preferable if it exists. (py can find different python versions, if present, while python-in-path can only check whether the python in the path has the correct version. By putting our python-in-path-check above the py-check we would forfeit the possibility of finding different python versions if any python is in the PATH).

Personally, if something needs to be changed about this, I'd rather just remove the Windows check (as I understand it, it is not doing anything functionally, just skipping some (potentially) useless checks).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may actually be OK, but I'd rather do it in a separate PR. We could do that one in parallel with this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, left it as it is for now in this PR. Before starting another PR, I'll be sorta-kinda waiting first what else might pop up in this PR. 😃

if path_from_version_param:
self._resolved = path_from_version_param
return self._resolved

# If we got this far, then we were unable to resolve the interpreter
# to an actual executable; raise an exception.
self._resolved = InterpreterNotFound(self.interpreter)
Expand Down
95 changes: 93 additions & 2 deletions tests/test_virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

IS_WINDOWS = nox.virtualenv._SYSTEM == "Windows"
HAS_CONDA = shutil.which("conda") is not None
RAISE_ERROR = "RAISE_ERROR"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very srs bznz!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm exceptionally proud of this line. :) Well, I at least hope it's more explicit than some fake path string like "c:\this\breaks\python.exe".



@pytest.fixture
Expand All @@ -47,6 +48,47 @@ def factory(*args, **kwargs):
return factory


@pytest.fixture
def make_mocked_interpreter_path():
Copy link
Contributor Author

@schuderer schuderer Aug 6, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion 1/2 as discussed in #224 (comment)

Suggested change
def make_mocked_interpreter_path():
def make_mocked_interpreter_path():
"""Provides a factory to create a mocked ``path`` to a python interpreter.
This mocked ``path`` provides a ``__str__`` , as well as a ``sysexec`` method
which returns the value of the factory parameter ``sysexec_result``
(which can be a version string or ``RAISE_ERROR``).
"""

def factory(path, sysexec_result):
def mock_sysexec(*_):
if sysexec_result == RAISE_ERROR:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for re-factoring these tests at my request. I know that can be a bit annoying after you've written some useful unit tests.

The core of my re-factoring request was that I was hoping we could ditch if statements for distinct test cases. But now there is an if statement here and on line 78, so my original hesitation is still there.

Copy link
Contributor Author

@schuderer schuderer Aug 6, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not annoying at all! It's an interesting challenge, and, yes, there were absolutely too many if statements in the original test.

But thanks to your review, the number of if statements got cut in half. Now, there are no ifs in the tests themselves, so their logic should be much clearer. There are only mock/patch-related if statements left. Here, I really wanted to avoid copy-pasting the same mock/patch code over several tests. This mock/patch construction unfortunately needs to be a bit complex: a patched sysfind returns a mocked path to the python interpreter, which has a __str__, and whose sysexec method returns the version string.
On every one of these steps, something could go wrong and should be tested. The only way I see to test all these cases without conditionals, would be to mock all variants of these situations separately, which, IMO, would make the tests themselves harder to read.

I am aware that each fixture creates a new potential source for bugs, but for me, the ifs that are left would be clear and primitive enough that it's worth the tradeoff. I just don't see how I could split this whole thing up into separate if-less tests (with or without extra fixtures) that would still be readable and not lead to increasing the amount of lines of test code manyfold. I am open to suggestions on how someone with more experience than me would solve this.

EDIT: To mitigate, I added some documentation to the fixtures, see my two suggestions below. WDYT?

raise py.process.cmdexec.Error(1, 1, "", "", "")
else:
return sysexec_result

attrs = {
"sysexec.side_effect": mock_sysexec,
"__str__": mock.Mock(return_value=path),
}
mock_python = mock.Mock()
mock_python.configure_mock(**attrs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, is there some reason we can't use __spec__?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood that a plain Mock() would be preferable if possible (and MagicMock(spec=("__str__")) if it's not possible). I admit I'm unsure what exactly you are referring to by "Ditto" and "spec". I can change it to something like MagicMock(spec=("__str__"))? I might be confusing things here, also due to my lack of experience with unittest.mock, if in doubt, could you suggest a code example? Thanks!


return mock_python

return factory


@pytest.fixture
def patch_sysfind(make_mocked_interpreter_path):
def patcher(sysfind, only_find, sysfind_result, sysexec_result):
Copy link
Contributor Author

@schuderer schuderer Aug 6, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion 2/2 as discussed in #224 (comment)

Suggested change
def patcher(sysfind, only_find, sysfind_result, sysexec_result):
"""Provides a function to patch ``sysfind`` with parameters for tests related to
locating a Python interpreter in the system ``PATH``.
"""
def patcher(sysfind, only_find, sysfind_result, sysexec_result):
"""Returns an extended ``sysfind`` patch object for tests related to locating a
Python interpreter in the system ``PATH``.
Args:
sysfind: The original sysfind patch object
only_find (Tuple[str]): The strings for which ``sysfind`` should be successful,
e.g. ``("python", "python.exe")``
sysfind_result (Optional[str]): The ``path`` string to create the returned
mocked ``path`` object with which will represent the found Python interpreter,
or ``None``.
This parameter is passed on to ``make_mocked_interpreter_path``.
sysexec_result (str): A string that should be returned when executing the
mocked ``path`` object. Usually a Python version string.
Use the global ``RAISE_ERROR`` to have ``sysexec`` fail.
This parameter is passed on to ``make_mocked_interpreter_path``.
"""

mock_python = make_mocked_interpreter_path(sysfind_result, sysexec_result)

def mock_sysfind(arg):
if sysfind_result is None:
return None
elif arg.lower() in only_find:
return mock_python
else:
return None

sysfind.side_effect = mock_sysfind

return sysfind

return patcher


def test_process_env_constructor():
penv = nox.virtualenv.ProcessEnv()
assert not penv.bin
Expand Down Expand Up @@ -315,7 +357,7 @@ def test__resolved_interpreter_windows_pyexe(sysfind, make_one, input_, expected
attrs = {"sysexec.return_value": expected}
mock_py = mock.Mock()
mock_py.configure_mock(**attrs)
sysfind.side_effect = lambda arg: mock_py if arg == "py" else False
sysfind.side_effect = lambda arg: mock_py if arg == "py" else None

# Okay now run the test.
assert venv._resolved_interpreter == expected
Expand All @@ -338,7 +380,7 @@ def test__resolved_interpreter_windows_pyexe_fails(sysfind, make_one):
attrs = {"sysexec.side_effect": py.process.cmdexec.Error(1, 1, "", "", "")}
mock_py = mock.Mock()
mock_py.configure_mock(**attrs)
sysfind.side_effect = lambda arg: mock_py if arg == "py" else False
sysfind.side_effect = lambda arg: mock_py if arg == "py" else None

# Okay now run the test.
with pytest.raises(nox.virtualenv.InterpreterNotFound):
Expand All @@ -348,6 +390,55 @@ def test__resolved_interpreter_windows_pyexe_fails(sysfind, make_one):
sysfind.assert_any_call("py")


@mock.patch("nox.virtualenv._SYSTEM", new="Windows")
@mock.patch.object(py.path.local, "sysfind")
def test__resolved_interpreter_windows_path_and_version(
sysfind, make_one, patch_sysfind
):
# Establish that if we get a standard pythonX.Y path, we look it
# up via the path on Windows.
venv, _ = make_one(interpreter="3.7")

# Trick the system into thinking that it cannot find
# pythonX.Y up until the python-in-path check at the end.
# Also, we don't give it a mock py launcher.
# But we give it a mock python interpreter to find
# in the system path.
correct_path = r"c:\python37-x64\python.exe"
patch_sysfind(
sysfind,
only_find=("python", "python.exe"),
sysfind_result=correct_path,
sysexec_result="3.7.3\\n",
)

# Okay, now run the test.
assert venv._resolved_interpreter == correct_path


@pytest.mark.parametrize("input_", ["2.7", "python3.7", "goofy"])
@pytest.mark.parametrize("sysfind_result", [r"c:\python37-x64\python.exe", None])
@pytest.mark.parametrize("sysexec_result", ["3.7.3\\n", RAISE_ERROR])
@mock.patch("nox.virtualenv._SYSTEM", new="Windows")
@mock.patch.object(py.path.local, "sysfind")
def test__resolved_interpreter_windows_path_and_version_fails(
sysfind, input_, sysfind_result, sysexec_result, make_one, patch_sysfind
):
# Establish that if we get a standard pythonX.Y path, we look it
# up via the path on Windows.
venv, _ = make_one(interpreter=input_)

# Trick the system into thinking that it cannot find
# pythonX.Y up until the python-in-path check at the end.
# Also, we don't give it a mock py launcher.
# But we give it a mock python interpreter to find
# in the system path.
patch_sysfind(sysfind, ("python", "python.exe"), sysfind_result, sysexec_result)

with pytest.raises(nox.virtualenv.InterpreterNotFound):
venv._resolved_interpreter


@mock.patch("nox.virtualenv._SYSTEM", new="Windows")
@mock.patch.object(py._path.local.LocalPath, "check")
@mock.patch.object(py.path.local, "sysfind")
Expand Down