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.

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

Use the same interpreter when ptw is called outside of a virtual env #72

Merged
merged 1 commit into from Jun 7, 2017

Conversation

jacebrowning
Copy link
Contributor

My primary use case here is pipenv. I would like to be able to do:

$ pipenv run ptw

But this attempts to find a globally installed pytest:

Running: py.test
Error: [Errno 2] No such file or directory: 'py.test'

With this change, pytest-watch will call pytest as a module if not currently running in an activated virtual environment. This should also make running pytest-watch globally a nicer experience by ensuring the same Python interpreter is used to run both ptw and py.test even if the user's shell is misconfigured.

@joeyespo
Copy link
Owner

joeyespo commented Jun 7, 2017

👍

Thanks for the fix!

@jacebrowning jacebrowning deleted the same-interpreter branch June 7, 2017 19:02
@decentral1se
Copy link

Nice one @jacebrowning 👍

@@ -1,6 +1,7 @@
pytest-watch Changelog
======================

- Enhancement: Use the same Python interpreter for `py.text` when `ptw` is run outside of an activated virtual environment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/text/test/ ?!
Also s/py.test/pytest/ then.

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch, @blueyed! I completely missed that before.

Patched this up a0fff61.

@blueyed
Copy link
Collaborator

blueyed commented Jun 20, 2017

Really?
It uses py.test instead of python -m pytest in case of virtualenvs?!
Sounds like a nasty workaround for something.

@joeyespo joeyespo added the bug label Jun 21, 2017
@joeyespo
Copy link
Owner

@blueyed Looks like it's the other way around. py.test was used before this change. It runs python -m pytest inside a virtualenv to use the same interpreter. I'll bet python -m pytest could be used to cover both cases though.

@jacebrowning Just curious to hear your rationale. Any specific reason you kept py.test instead of using (sys.executable, '-m', 'pytest') in both cases?

return custom.split(' ')
if os.getenv('VIRTUAL_ENV'):
return ('py.test',)
return (sys.executable, '-m', 'pytest')
Copy link
Owner

Choose a reason for hiding this comment

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

Just tested this locally. Got TypeError: can only concatenate tuple (not "list") to tuple because lists were used outside this function.

I just patched this in f4814f9.

@jacebrowning
Copy link
Contributor Author

Any specific reason you kept py.test

@joeyespo Simply to leave the existing behavior unchanged.

I also noticed that pytest-watch prints the full command it's running for pytest, which might introduce some confusing noise to users that were used to seeing a simple "Running py.test" message.

@blueyed
Copy link
Collaborator

blueyed commented Jun 21, 2017

Why would python pick up the virtualenv's python, but py.test/pytest not the virtualenv's pytest?

Maybe it should only fall back to python -m pytest in case pytest is not found as an executable?

@blueyed
Copy link
Collaborator

blueyed commented Jun 21, 2017

@lwm
Did it fix something in this regard for you?

@decentral1se
Copy link

@lwm
Did it fix something in this regard for you?

I just found this pull request through some searching and was happy to see someone put some time in and decided to show some gratitude! In retrospect, I am glad I also provoked your usual high quality code review 😄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants