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 1 commit
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
8 changes: 4 additions & 4 deletions nox/virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,13 @@ def locate_using_path_and_version(version):
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:
version_pattern = "^Python {}".format(version)
version_string = path_python.sysexec("--version").strip()
match = re.match(version_pattern, version_string)
if match:
prefix = "{}.".format(version)
version_string = path_python.sysexec("-c", script).strip()
if version_string.startswith(prefix):
return path_python
except py.process.cmdexec.Error:
return None
Expand Down
121 changes: 76 additions & 45 deletions tests/test_virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@


IS_WINDOWS = nox.virtualenv._SYSTEM == "Windows"
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 @@ -35,6 +36,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 @@ -279,64 +321,53 @@ def test__resolved_interpreter_windows_pyexe_fails(sysfind, make_one):
sysfind.assert_any_call("py")


@pytest.mark.parametrize(
["input_", "path_result", "expected"],
[
("3.7", r"c:\python37-x64\python.exe", r"c:\python37-x64\python.exe"),
("3.7", r"c:\fails\python.exe", None),
("python3.7", r"c:\python37-x64\python.exe", None),
("2.7", r"c:\python37-x64\python.exe", None),
("goofy", r"c:\python37-x64\python.exe", None),
("3.7", None, None),
("python3.7", None, None),
("2.7", None, None),
("goofy", None, None),
],
)
@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, input_, path_result, expected
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=input_)
venv, _ = make_one(interpreter="3.7")

# Trick the system into thinking that it cannot find python3.6
# (it likely will on Unix). Also, we don't give it a dummy
# py launcher. But we give it a dummy python interpreter to find
# 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.
if path_result:
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",
)

def mock_sysexec(_):
if path_result == r"c:\fails\python.exe":
raise py.process.cmdexec.Error(1, 1, "", "", "")
else:
return "Python 3.7.3 | Anaconda\\n"
# Okay, now run the test.
assert str(venv._resolved_interpreter) == correct_path

attrs = {
"sysexec.side_effect": mock_sysexec,
"__str__.return_value": path_result,
}
mock_python = mock.MagicMock()
mock_python.configure_mock(**attrs)

def mock_sysfind(arg):
if arg.lower() == "python" or arg.lower() == "python.exe":
return mock_python
else:
return None
@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_)

sysfind.side_effect = mock_sysfind
else:
sysfind.return_value = None
# 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)

# Okay now run the test.
if expected:
assert str(venv._resolved_interpreter) == expected
else:
with pytest.raises(nox.virtualenv.InterpreterNotFound):
venv._resolved_interpreter
with pytest.raises(nox.virtualenv.InterpreterNotFound):
venv._resolved_interpreter


@mock.patch("nox.virtualenv._SYSTEM", new="Windows")
Expand Down