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.
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
Resolve to interpreter 'python' in PATH if '--version' fits #224
Resolve to interpreter 'python' in PATH if '--version' fits #224
Conversation
tests/test_virtualenv.py
Outdated
# up via the path on Windows. | ||
venv, _ = make_one(interpreter=input_) | ||
|
||
# Trick the system into thinking that it cannot find python3.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment refers to 3.6 but none of the test cases do. Then the next comment is about Unix but this test is meant to be Windows only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to review! I was copying some logic from another test and did not fully adapt the comment, will change it.
One difficulty with testing _resolved_parameter
is that it checks for things one after another, so every test has to make sure that the checks above the check-under-test fail (of which most are for Unix, hence the original comment). I am inclined to think that refactoring _resolved_parameter
(separately from this PR, maybe next after) would help reducing the complexity of the tests (factoring out the checks themselves, separating them from the flow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
tests/test_virtualenv.py
Outdated
# (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 | ||
# in the system path. | ||
if path_result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 4 if statements and 2 closures in this test case. I'd much rather see it broken up into distinct test cases with a "single path of execution" to make it clear (and hopefully 🤞 more concise) what is happening in each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not my most elegant test case indeed. 😬 I tried to split it up earlier and found that I would have to repeat a lot preparation code (creating mocks and making sure the checks above the check-under-test fail). I could create yet another fixture to help with this, but I'm not sure whether this makes it even less transparent. I'm unsure, should I choose conciseness or clarity in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to break this up without any if
statements? If it gets too unwieldy we can revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your comments. I pushed my changes just now. Most of the branching actually was part of patching/mocking sysfind, the resulting path object, and python-sysexec the right way for the various parameters. Off-loading the mocking and patching into two pytest fixtures solved the complexity problem IMO (and as a side-effect, created fixtures that are probably useful for other tests in the module as well).
if path_from_launcher: | ||
self._resolved = path_from_launcher | ||
return self._resolved | ||
|
||
path_from_version_param = locate_using_path_and_version(xy_version) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😃
Hope CI runs successfully. Local testing appears to be not quite as smooth and easy as it used to be. Refreshing from master brought some conda-related test sessions which fail both on my macOS system ( This causes |
I've marked this PR as Our funds aren't magnificent, so I'm setting the cap here at $20/participant. |
Likely that's because these packages are being fetched from conda-forge in the CI builds.
|
Can I give my $20 to @schuderer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to just merge this. I'm a bit uneasy about the unit tests in that I don't feel like I fully understand all that's being mocked (it goes a few layers deep), but I'm not sure my unease is reason enough to make this PR drag on any longer (already 12 days in).
WDYT @schuderer @theacodes?
@@ -104,13 +104,13 @@ def locate_using_path_and_version(version): | |||
if not version: | |||
return None | |||
|
|||
script = "import platform; print(platform.python_version())" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
@@ -23,6 +23,7 @@ | |||
|
|||
|
|||
IS_WINDOWS = nox.virtualenv._SYSTEM == "Windows" | |||
RAISE_ERROR = "RAISE_ERROR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very srs bznz!
There was a problem hiding this comment.
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"
.
"__str__": mock.Mock(return_value=path), | ||
} | ||
mock_python = mock.Mock() | ||
mock_python.configure_mock(**attrs) |
There was a problem hiding this comment.
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__
?
There was a problem hiding this comment.
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!
def make_mocked_interpreter_path(): | ||
def factory(path, sysexec_result): | ||
def mock_sysexec(*_): | ||
if sysexec_result == RAISE_ERROR: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 if
s 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 if
s 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?
As long as we understand the intent of the tests we can always replace them later if needed. |
Of course. |
So let's merge? |
You're a maintainer. Go for it. :)
…On Mon, Aug 5, 2019 at 10:44 PM Danny Hermes ***@***.***> wrote:
As long as we understand the *intent* of the tests we can always replace
them later if needed.
So let's merge?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#224>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB5I46LVEX6FDJPSSD2DO3QDEFUTANCNFSM4IGVW3XA>
.
|
Haha, I intended to suggest to give it all to @dhermes 😆 - But seriously, I don't really want the money, as this would essentially be a 4x refund of my donation from a few weeks ago. I'd prefer to keep it in nox's pot/re-donate it to the project. Some stickers would be awesome, though 😁 (if you set up a merch store with the nice art you got, I bet people would buy stickers/mugs/shirts, even more so if it supports the project). |
@@ -47,6 +48,47 @@ def factory(*args, **kwargs): | |||
return factory | |||
|
|||
|
|||
@pytest.fixture | |||
def make_mocked_interpreter_path(): |
There was a problem hiding this comment.
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)
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``). | |
""" |
|
||
@pytest.fixture | ||
def patch_sysfind(make_mocked_interpreter_path): | ||
def patcher(sysfind, only_find, sysfind_result, sysexec_result): |
There was a problem hiding this comment.
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)
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``. | |
""" |
@tswast thanks for your hint, I got further. It still fails with permission error: access is denied on others: please excuse the off-topic discussion |
Hmmm. I think there can be some problems with the virtualenv package that comes with conda. I installed https://github.com/conda-forge/virtualenv-feedstock in my environment. Then again, that's what should be happening already based on https://github.com/theacodes/nox/blob/master/requirements-conda-test.txt I'll try on my local Windows laptop later today and see if I hit the same issues. |
Let's merge this? this is getting in my way for adding windows CI to my project! |
@dhermes Agree with @omry, if I see it correctly, all review questions have been addressed (last point: please accept my documentation suggestions above if they are ok, or if accepting them does not work, tell me whether I should implement them). The conda discussion is off-topic and has been moved to another issue. @omry In the meantime, feel free to use my workaround |
This LGTM, there might still be some improvements we could make to the tests but I don't see that as a reason to hold this any longer. |
awesome, what is the release cadence for nox? when can I expect to try it out from the pip package? |
It's whenever. I can try to cut a new release soon.
…On Fri, Aug 16, 2019, 11:36 AM Omry Yadan ***@***.***> wrote:
awesome, what is the release cadence for nox? when can I expect to try it
out from the pip package?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#224>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB5I457Q7F7KDZ5A5TLYPTQE3XTTANCNFSM4IGVW3XA>
.
|
great, please do. |
@theacodes Um, what about my test-doc suggestions above, wanted to add them if they improve things, but got no feedback. Now they're left out. New PR? |
Sure! Sorry about that.
…On Sat, Aug 17, 2019, 9:53 AM Andreas Schuderer ***@***.***> wrote:
@theacodes <https://github.com/theacodes> Um, what about my test-doc
suggestions above, wanted to add them if they improve things, but got no
feedback. New PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#224>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB5I43DEEKFVAAMX6XIPHDQFAUH5ANCNFSM4IGVW3XA>
.
|
@schuderer, I tried your py.bat hack and it didn't quiet work for me, I also have some slight reservation about the license it comes with (LGPL). I will try out the actual fix in nox as soon as it's released. @theacodes, I am eagerly awaiting the new version. |
@omry You can always install $ pip install git+https://github.com/theacodes/nox@master You can also put And sorry about missing your message! I've been hit by a lot of spam lately, and the filter might have become a little too eager. Unfortunately, I can't find your message in my spam folder; it must have been more than a month ago and already have been cleaned out... The |
I've also cut a new release!
…On Wed, Aug 21, 2019, 1:36 AM Andreas Schuderer ***@***.***> wrote:
@omry <https://github.com/omry> You can always install nox from source,
for example using:
$ pip install ***@***.***
You can also put ***@***.*** in your
project's requirements.txt for the time being, and I think it should work
on CI as well.
And sorry about missing your message! I've been hit by a lot of spam
lately, and the filter might have become a little too eager. Unfortunately,
I can't find your message in my spam folder; it must have been more than a
month ago and already have been cleaned out... The py.bat hack (more
fitting term than "workaround", actually :) ) is indeed kind of brittle. I
would have changed this file's license to MIT if that's a problem.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#224>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB5I42MNI2AUU6ERB43YD3QFT47DANCNFSM4IGVW3XA>
.
|
@schuderer no need to change the license, I will test the new nox version and we can work on fixing it if it's not quiet there yet instead of on focusing on temporary solutions. @theacodes awesome, I will test it soon. |
To fix #219
TL;DR: Trying to implement the suggestion in #219 (comment), this PR adds a final check at the end of the property
nox.virtualenv._resolved_interpreter
which looks for an interpreter called "python" in thePATH
, checks whether its--version
fits the required version, and returns it as the resolved interpreter if it matches.Rationale
As described in #219, sessions specifying the interpreter version (e.g.
@nox.session(python='3.7')
) don't find the existing matching python 3.7 if it is called "python" or "python.exe" and no python launcher ("py.exe") is available.The same sessions work when not specifying python='3.7', even if the used interpreter is actually python 3.7. If this happens, it makes it harder for collaborators on Unix/Windows to work on cross-platform libraries together (like
nox
or my current pet project) without temporarily adapting the noxfile to their own system (or their system to the noxfile).While the mechanisms applied in
nox.virtualenvs
do follow the Python guidelines, in practice, Anaconda installations on Windows, which are quite popular in my scientific/data-wrangling field, provide neither versioned interpreters (like 'python2', 'python27', 'python3', 'python37') nor a python launcher.For Anaconda-specific projects, the recent addition of conda environment support to
nox
is certainly very interesting. However, I am arguing from the perspective of cross-platform library development and would like to be able to provide a noxfile to my project which works on Unix and Windows, be it with Anaconda or with "proper" Python installations.One could argue that this is not
nox
s problem and should be fixed in Anaconda (and one would be right), but it is a pretty small addition and would in one step increase the reach for nox to everyone using Anaconda on Windows (which are quite a few people). And it would make it easier for me (andnox
;)) to find Windows contributors also.Implementation
Added the check (with an extra module-level function for cleanliness), and some parametrised tests. I also fixed two incorrect return value mocks of
py.path.local.sysfind
(which returnsNone
on not found, notFalse
like in the mock return value), which my code initially tripped over because it checked forNone
.As the body of
_resolved_interpreter
is essentially a string of if-else conditionals, creating good tests was a bit of a challenge. I hope they're not too ugly. :)