-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Skip conda and pyenv tests if these tools are not present #17095
PR: Skip conda and pyenv tests if these tools are not present #17095
Conversation
Hello @juliangilbey! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-12-27 10:12:49 UTC |
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 your help with this @juliangilbey!
|
||
|
||
CONDA_MISSING = (len(get_list_conda_envs()) == 0) |
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.
We have a function called is_anaconda
in spyder.config.utils
. So please use that one instead.
Also, there's no need to declare an extra constant here for that.
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.
Ah, I didn't know about that function, thanks!
There's a newer comment on the stackoverflow thread, by the way, which suggests that is_anaconda
should use sys.base_prefix
instead of sys.prefix
for greater robustness.
spyder/utils/tests/test_conda.py
Outdated
@@ -21,6 +21,8 @@ | |||
get_conda_root_prefix, get_list_conda_envs, get_list_conda_envs_cache) | |||
|
|||
|
|||
CONDA_MISSING = (len(get_list_conda_envs()) == 0) |
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.
Please change this line to
if not is_anaconda():
pytest.skip()
and remove the skipif
you added in each test below. That will allow us to skip all tests in this file.
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.
Ah, I didn't know the pytest.skip()
function; that's nice!
spyder/utils/tests/test_pyenv.py
Outdated
@@ -15,6 +15,10 @@ | |||
from spyder.utils.pyenv import get_list_pyenv_envs, get_list_pyenv_envs_cache | |||
|
|||
|
|||
PYENV_MISSING = (len(get_list_pyenv_envs()) == 0) |
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.
Please change this line to
if not find_program('pyenv'):
pytest.skip()
and remove the skipif
you added in each test below.
find_program
is available in spyder.utils.programs
.
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.
All three requests fulfilled!
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.
Looks good to me now, thanks @juliangilbey!
Description of Changes
This patch skips the remaining tests which rely on the presence of pyenv or conda, in the same way that #17067 was fixed in PR #17092.
Issue(s) Resolved
(I never reported this as an issue; I just skipped them on the Debian side.)
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: @juliangilbey